Optimize EmptySorting by caching isNodeEmpty result #9149
Conversation
k8s-ci-robot
added
kind/cleanup
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: x13n The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
This change optimizes the EmptySorting scale down candidate processor by implementing a caching mechanism for the isNodeEmpty check. Summary of changes: 1. Extended CandidatesComparer interface with ResetState() method to allow clearing caches before each sorting pass. 2. Updated NodeSorter.Sort() to invoke ResetState() on all processors. 3. Implemented caching in EmptySorting, ensuring simulator.GetPodsToMove is called at most once per node during a sorting cycle. 4. Added no-op ResetState() to other CandidatesComparer implementations. Benchmark results for sorting 5000 nodes with EmptySorting: - Before: 26,403,629 ns/op (~26.4 ms) - After: 5,120,497 ns/op (~5.1 ms) Performance improvement: ~5x. Testing: Added micro benchmark BenchmarkEmptySorting and updated existing unit tests.
| return n.nodes | ||
| } | ||
| for _, p := range n.processors { | ||
| p.ResetState() |
There was a problem hiding this comment.
The optimization makes sense, however it's not clear how the Sort() method actuates the reset in a sane way (perhaps these optimizations are consumed downstream somewhere and not obvious in the OSS code?)
There was a problem hiding this comment.
Yeah, we do have another comparer downstream that will also benefit from this, but it is not clear to me why the current approach is not sane - state is reset before every sorting, so any local optimizations can be aware of that. Can you clarify what do you think is a problem?
3 participants
This change optimizes the EmptySorting scale down candidate processor by implementing a caching mechanism for the isNodeEmpty check.
Summary of changes:
Benchmark results for sorting 5000 nodes with EmptySorting:
Performance improvement: ~5x.
Testing: Added micro benchmark BenchmarkEmptySorting and updated existing unit tests.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: