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

Add automaxprocs. #4301

Merged
merged 2 commits into from
May 10, 2023
Merged

Add automaxprocs. #4301

merged 2 commits into from
May 10, 2023

Conversation

robholland
Copy link
Contributor

@robholland robholland commented May 9, 2023

This will automate the setting of GOMAXPROCS in Kubernetes/docker environments where it is not already set as part of the deployment.

What changed?

automaxprocs library was added to set GOMAXPROCS to match the CPU limits set on a container. This is a no-op if GOMAXPROCS environment variable is already set, or if not running in a container.

Why?

Setting GOMAXPROCS to match resource limits (rather than the total core count of the node) allows Go to more efficiently use the available cores and reduces CPU throttling (eliminating it entirely if limits are set to an integer number of cores).

This issue was highlighted during benchmarking but probably effects a large number of real world Kubernetes deployments where CPU limits are set but GOMAXPROCS is not.

How did you test it?

Potential risks

Is hotfix candidate?

No.

This will automate the setting of GOMAXPROCS in Kubernetes/docker environments
where it is not already set as part of the deployment.

Setting GOMAXPROCS to match resource limits (rather than the total core count
of the node) allows Go to more efficiently use the available cores and
reduces CPU throttling (eliminating it if limits are set to an integer number
of cores).

This issue was highlighted during benchmarking but probably effects a large
number of real world Kubernetes deployments where CPU limits are set but
GOMAXPROCS is not.
@robholland robholland requested a review from a team as a code owner May 9, 2023 10:07
@robholland
Copy link
Contributor Author

robholland commented May 9, 2023

6-mh

Example of correcting GOMAXPROCS on an 8-core node to match the CPU limit of 1 core. See that throttling is gone and that CPU usage in general has improved (reduced).

Note: This is was not done using this PR as docker builds are not automatically published for PRs. This was done by manually setting GOMAXPROCS, but that is same effect as adding this library.

Copy link
Contributor

@MichaelSnowden MichaelSnowden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue was highlighted during benchmarking

Do you have any benchmark results to compare before and after this change? I see the throttling disappear from the screenshot, but it looks like this change increased average latency in the example that the library has in their docs. It'd be good to know how it would affect our latency profile.

@robholland
Copy link
Contributor Author

I don't have the cluster now but I can recreate, which metrics would you like comparing before/after?

@MichaelSnowden
Copy link
Contributor

I don't have the cluster now but I can recreate, which metrics would you like comparing before/after?

I think task processing p50 and p99 is good enough

@robholland
Copy link
Contributor Author

task_latency_processing ?

@robholland
Copy link
Contributor Author

Soak Test - Pods - Temporal - Dashboards - Grafana-mh

@robholland
Copy link
Contributor Author

The setup is 8 core node, CPU limit for history pods set to 1.

@robholland robholland merged commit 807b791 into master May 10, 2023
@robholland robholland deleted the rh-automaxprocs branch May 10, 2023 07:27
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.

2 participants