-
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: Capture InstanceTypeFilterErrors in error structs and don't print them until the end #1948
perf: Capture InstanceTypeFilterErrors in error structs and don't print them until the end #1948
Conversation
Skipping CI for Draft Pull Request. |
Pull Request Test Coverage Report for Build 13082607588Details
💛 - Coveralls |
ecce7df
to
0edfada
Compare
0edfada
to
06f044f
Compare
06f044f
to
f91a411
Compare
if !r.requirementsMet && !r.fits && !r.hasOffering { | ||
return "no instance type met the scheduling requirements or had enough resources or had a required offering" | ||
if !e.requirementsMet && !e.fits && !e.hasOffering { | ||
return fmt.Sprintf("no instance type met the scheduling requirements or had enough resources or had a required offering, requirements=%s, resources=%s", e.requirements, resources.String(resources.Merge(e.daemonRequests, e.podRequests))) |
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.
Does the improvement require that e.requirements, resources.String(resources.Merge(e.daemonRequests, e.podRequests)
be called in every branch as opposed to once at the top of .Error()
?
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.
No, it doesn't -- I was going back and forth on whether to add the code duplication or not -- it seemed fine to me to do it this way for the short-circuiting and error returns -- otherwise, I have to create a string and then append to it
/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 ensures that we don't perform expensive operations that show-up in the CPU profile and show-up in our heap analysis to print the nodeClaimRequirements and performing resources.Merge.
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 31m49s. After the change, the same test case takes 26m34s (a 5m15s improvement).
Before PR
Scheduling Benchmark
Live Test
Scheduling Time: 31m49s
Heap Profile
After PR
Scheduling Benchmark
Live Test
Scheduling Time: 26m34s
Heap Profile
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.