-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Don't create new sets when checking compatibility #1953
perf: Don't create new sets when checking compatibility #1953
Conversation
Skipping CI for Draft Pull Request. |
a7eb318
to
c2dfcaf
Compare
func (r *Requirement) HasIntersection(requirement *Requirement) bool { | ||
greaterThan := maxIntPtr(r.greaterThan, requirement.greaterThan) | ||
lessThan := minIntPtr(r.lessThan, requirement.lessThan) | ||
if greaterThan != nil && lessThan != nil && *greaterThan >= *lessThan { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking - is this supposed to be *greaterThan >= *lessThan
and not *greaterThan > *lessThan
? Wouldn't there be an intersection if these exist and have overlap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, because you are saying that a requirement is valid if, for instance -- elem > 4 and also elem < 4 -- there is no such value that satisfies both
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonathan-innis, rschalo 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
This change adds the
HasIntersection
method which allows theIntersects
check to validate whether two keys have an intersection with their values without creating new sets under the hood -- greatly reducing the number of allocations that we do for sets and reducing the amount of time that the garbage collector needs to spend cleaning them up.These changes were executed against the scheduling benchmark (which showed negligible performance changes) and a live test that deploys 20,000 pods to the cluster and generates 10,000 nodes (with two pods per node constrained by the size of the instance types on NodePools). Prior to the change, this test took 27m17s. After the change, the same test case takes 21m36s (a 5m41s improvement).
Before PR
Scheduling Benchmark
CPU Flamegraph
Heap Profile
Live Test
Scheduling Time: 27m17s
After PR
Scheduling Benchmark
CPU Flamegraph
Heap Profile
Live Test
Scheduling Time: 21m36s
How was this change tested?
make presubmit
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.