perf: increase scheduler performance #982
Conversation
A few changes that overall increase performance 10-60% depending on the number of pods/nodes. Its around a ~34% overall improvement: name old pods/sec new pods/sec delta Scheduling1-12 3.89k ± 2% 4.36k ± 4% +12.10% (p=0.000 n=10+9) Scheduling50-12 1.44k ±11% 1.70k ± 5% +18.29% (p=0.000 n=9+9) Scheduling100-12 1.30k ±10% 2.09k ± 3% +60.63% (p=0.000 n=9+9) Scheduling500-12 1.47k ± 3% 2.06k ± 1% +40.65% (p=0.000 n=8+9) Scheduling1000-12 1.42k ± 5% 1.99k ± 1% +39.32% (p=0.000 n=10+10) Scheduling2000-12 1.43k ± 1% 1.83k ± 2% +28.36% (p=0.000 n=9+9) Scheduling5000-12 1.09k ± 6% 1.67k ± 2% +53.75% (p=0.000 n=9+10)
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
Pull Request Test Coverage Report for Build 7715418157
💛 - Coveralls |
| @@ -56,10 +56,22 @@ const PrintStats = false | |||
| //nolint:gosec | |||
| var r = rand.New(rand.NewSource(42)) | |||
|
|
|||
| // To run the benchmarks use: | |||
| @@ -237,9 +237,38 @@ func labelHint(r Requirements, key string, allowedUndefined sets.Set[string]) st | |||
| return "" | |||
| } | |||
|
|
|||
| // badKeyError allows lazily generating the error string in the case of a bad key error. When requirements fail | |||
| // to match, we are most often interested in the failure and not why it fails. | |||
| type badKeyError struct { | |||
There was a problem hiding this comment.
It's surprising to me that templating out those error values into a string is so expensive. What's the performance hit that we take from not taking this change in and just returning errors like normal?
There was a problem hiding this comment.
Changing the errors is a large part of the performance increase for anti-affinity pods in particular due to the repeated failures to match topologies. String interpolation is expensive and it happens a lot in these cases.
There was a problem hiding this comment.
Got it. This is good to know 👍🏼
| return options | ||
| } | ||
| for domain := range t.domains { | ||
| for domain := range t.emptyDomains { |
|
Worth sharing the CPU profile that you generated from pprof as part of the PR just for posterity? |
I don't think it would be that helpful. We should instead set some minimum benchmarks and enable the CI based tests for performance. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonathan-innis, tzneal The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #N/A
Description
A few changes that overall increase performance 10-60% depending on the number of pods/nodes. Its around a ~34% overall improvement:
How was this change tested?
Benchmarking and unit testing/
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.