Skip to content

✨ Simplify and speedup priority queue#3471

Open
inteon wants to merge 1 commit intokubernetes-sigs:mainfrom
inteon:priority_queue
Open

✨ Simplify and speedup priority queue#3471
inteon wants to merge 1 commit intokubernetes-sigs:mainfrom
inteon:priority_queue

Conversation

@inteon
Copy link
Member

@inteon inteon commented Mar 7, 2026

I reviewed the priority queue source code, simplified it and tried to improve the benchmarks.

$ go test -run=^$ -benchmem -bench ^Benchmark sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue -count 10 > main.out
$ go test -run=^$ -benchmem -bench ^Benchmark sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue -count 10 > pr.out
$ benchstat main.out pr.out 
goos: linux
goarch: amd64
pkg: sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue
cpu: Intel(R) Core(TM) Ultra 7 165H
                   │   main.out    │                pr.out                │
                   │    sec/op     │    sec/op     vs base                │
AddGetDone-8         1048.9µ ±  2%   300.1µ ±  2%  -71.39% (p=0.000 n=10)
AddOnly-8             91.11µ ± 22%   26.98µ ±  4%  -70.39% (p=0.000 n=10)
AddLockContended-8   121.87µ ± 28%   45.08µ ± 22%  -63.01% (p=0.000 n=10)
geomean               226.7µ         71.46µ        -68.47%

                   │     main.out     │                 pr.out                 │
                   │       B/op       │     B/op       vs base                 │
AddGetDone-8           273.64Ki ±  0%   31.30Ki ±  0%   -88.56% (p=0.000 n=10)
AddOnly-8            193930.000 ± 15%     2.000 ±  0%  -100.00% (p=0.000 n=10)
AddLockContended-8    221.532Ki ± 17%   3.563Ki ± 29%   -98.39% (p=0.000 n=10)
geomean                 225.6Ki           616.1         -99.73%

                   │   main.out    │                  pr.out                   │
                   │   allocs/op   │  allocs/op    vs base                     │
AddGetDone-8           5.014k ± 0%   1.000k ±  0%   -80.06% (p=0.000 n=10)
AddOnly-8              1.000k ± 0%   0.000k ±  0%  -100.00% (p=0.000 n=10)
AddLockContended-8   1071.000 ± 2%    2.000 ± 50%   -99.81% (p=0.000 n=10)
geomean                1.751k                      ?                       ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: inteon
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 7, 2026
@inteon inteon force-pushed the priority_queue branch 2 times, most recently from c2a810e to 1c8ad6a Compare March 8, 2026 08:37
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is doing way too many many things at once without explanation other than a benchmark result in the cover letter. If you want a chance at getting something merged, please do small and targeted changes with a description and demonstration of their impact

addBuffer: make(map[T]*item[T]),
addBufferOld: make(map[T]*item[T]),
items: map[T]trackingItem[T]{},
ready: btree.NewWithFreeList(32, lessReady[T], freeList),
Copy link
Member

@alvaroaleman alvaroaleman Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This aproach actually results in a smaller freelist, since btree.New is just NewWithFreeList(degree, less, NewFreeList[T](DefaultFreeListSize)) under the hood

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants