Skip to content
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: Make list calls more consistent #239

Closed
wants to merge 1 commit into from
Closed

perf: Make list calls more consistent #239

wants to merge 1 commit into from

Conversation

stijndehaes
Copy link
Contributor

This has the added benefit that the list object
is created on the stack and not on the heap

In support of #722

Description

I made no pointers to list objects were created before calling the kubeclient list call.

How was this change tested?
Unit tests, also deployed on a live environment to test out the performance.

This change reduces times spent in the UpdateDaemonSet function significantly. It went from 8s spent to not being in the tree anymore.
To create these images I took CPU profiles for 120s

Before:
daemonset-before

After:
daemonset-after

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This has the added benefit that the list object
is created on the stack and not on the heap
@stijndehaes stijndehaes requested a review from a team as a code owner March 13, 2023 12:55
@jonathan-innis
Copy link
Member

Can you share the scheduling benchmarks to see if there were any changes on timing performance here? Sharing manual testing and observed performance would also be useful.

@jonathan-innis
Copy link
Member

jonathan-innis commented Mar 13, 2023

Also, do you have any references to client-go best practices around list calls that shows it's preferable to use the real object vs. utilizing the pointer to the object? It's interesting to me that provisioning the same objects, but just referencing it by pointer in a scope of code affects CPU clock cycles so significantly

The reason I was curious is mainly around this callout in the Go FAQ on heap vs. stack allocations:

In the current compilers, if a variable has its address taken, that variable is a candidate for allocation on the heap. However, a basic escape analysis recognizes some cases when such variables will not live past the return from the function and can reside on the stack.

@coveralls
Copy link

coveralls commented Mar 13, 2023

Pull Request Test Coverage Report for Build 4405121557

  • 14 of 16 (87.5%) changed or added relevant lines in 7 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 80.828%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/node/controller.go 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/provisioning/scheduling/preferences.go 7 86.67%
Totals Coverage Status
Change from base Build 4377678707: -0.06%
Covered Lines: 6539
Relevant Lines: 8090

💛 - Coveralls

@stijndehaes
Copy link
Contributor Author

@jonathan-innis I did some thinking yesterday and I am not sure anymore if what I observed was true. Maybe it is just hidden because not all nodes of the profile are shown in these images. I will do some more tests again this week.

@stijndehaes
Copy link
Contributor Author

@jonathan-innis as I suspected the nodes were hidden and this has no performance impact, will close the PR

@jonathan-innis
Copy link
Member

@stijndehaes Thanks for the follow-up. This is really good work though! If you find other performance improvements please don't hesitate to open PRs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants