-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8281181: Do not use CPU Shares to compute active processor count #7666
8281181: Do not use CPU Shares to compute active processor count #7666
Conversation
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
/label remove hotspot-runtime |
@iklam |
@iklam |
Webrevs
|
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.
Hi Ioi,
Generally looks good. A couple of nits below.
The deprecated flags should be added to the VMDeprecatedOptions test.
Thanks,
David
int share = cpu_shares(); | ||
|
||
// It's not a good idea to use cpu_shares() to limit the number | ||
// of CPUs used by the JVM. See JDK-8281571. |
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.
I suggest using JDK-8281181 rather than the CSR issue.
@@ -100,6 +100,11 @@ private static void testComboWithCpuSets() throws Exception { | |||
String cpuSetStr = CPUSetsReader.readFromProcStatus("Cpus_allowed_list"); | |||
System.out.println("cpuSetStr = " + cpuSetStr); | |||
|
|||
// OLD = use the deprecated -XX:+UseContainerCpuShares flag, which | |||
// will be removed in the next JDK release. See JDK-8281571. |
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.
I suggest using JDK-8281181 rather than the CSR issue.
Hi David, thanks for the review. I've updated the patch as you suggested. |
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.
LGTM
@iklam This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 35 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
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.
Updates look good.
Please file an issue to obsolete these flags in JDK 20 (they will be expired en-masse in JDK 21).
Thanks,
David
I filed https://bugs.openjdk.java.net/browse/JDK-8282684 Thanks |
Thanks to @dholmes-ora and @jerboaa for the review. |
Going to push as commit e07fd39.
Your commit was automatically rebased without conflicts. |
This is a simple change (Linux-only) that removes the consideration of Cgroups CPU Shares from the active process count calculation. Of note, this fixes CPU underutilization when Java is executed by Kubernetes without CPU resources limits.
Please see the CSR JDK-8281571 for a detailed discussion of the reasons to make this change.
To err on the side of caution, we added a temporary (and deprecated) VM flag
-XX:+UseContainerCpuShares
to enable the old behavior. We believe the old behavior is wrong and unnecessary. The plan is to remove the old behavior in JDK 20.The associated flag,
PreferContainerQuotaForCPUCount
is also deprecated. Both flags will be obsoleted in JDK 20.Testing with tiers 1-4, as well as container tests in tier5.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7666/head:pull/7666
$ git checkout pull/7666
Update a local copy of the PR:
$ git checkout pull/7666
$ git pull https://git.openjdk.java.net/jdk pull/7666/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7666
View PR using the GUI difftool:
$ git pr show -t 7666
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7666.diff