Skip to content

Enable 'UseContainerCpuShares' for java distributions #1414

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

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

carterkozak
Copy link
Contributor

https://bugs.openjdk.org/browse/JDK-8281181 was introduced in the October patch.
==COMMIT_MSG==
Enable 'UseContainerCpuShares' for java distributions
==COMMIT_MSG==

@changelog-app
Copy link

changelog-app bot commented Dec 8, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Enable 'UseContainerCpuShares' for java distributions

Check the box to generate changelog(s)

  • Generate changelog entry

@carterkozak carterkozak marked this pull request as draft December 8, 2022 20:44
@carterkozak
Copy link
Contributor Author

Leaving this as a draft until I have data from an internal test.

// is deprecated in jdk19 and expired in jdk21: https://bugs.openjdk.org/browse/JDK-8282684
.addAllJvmOpts(
javaVersion.get().compareTo(JavaVersion.toVersion("11")) >= 0
&& javaVersion.get().compareTo(JavaVersion.toVersion("20")) <= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we only add -XX:+UseContainerCpuShares for JDK [11, 19] as JDK 20.x obsoletes the flag and will accept it as an argument without throwing, but it won't have any effect as it has already been removed from jdk-20+12 from the code outside https://github.com/openjdk/jdk/blob/jdk-20+26/src/hotspot/share/runtime/arguments.cpp#L552

Suggested change
&& javaVersion.get().compareTo(JavaVersion.toVersion("20")) <= 0
&& javaVersion.get().compareTo(JavaVersion.toVersion("19")) <= 0
  { "UseContainerCpuShares",        JDK_Version::jdk(19), JDK_Version::jdk(20), JDK_Version::jdk(21) },
 DEPRECATED: An option that is supported, but a warning is printed to let the user know that
             support may be removed in the future. Both regular and aliased options may be
             deprecated.

             Add a deprecation warning for an option (or alias) by adding an entry in the
             "special_jvm_flags" table and setting the "deprecated_in" field.
             Often an option "deprecated" in one major release will
             be made "obsolete" in the next. In this case the entry should also have its
             "obsolete_in" field set.

   OBSOLETE: An option that has been removed (and deleted from globals.hpp), but is still accepted
             on the command line. A warning is printed to let the user know that option might not
             be accepted in the future.

             Add an obsolete warning for an option by adding an entry in the "special_jvm_flags"
             table and setting the "obsolete_in" field.

    EXPIRED: A deprecated or obsolete option that has an "accept_until" version less than or equal
             to the current JDK version. The system will flatly refuse to admit the existence of
             the flag. This allows a flag to die automatically over JDK releases.

             Note that manual cleanup of expired options should be done at major JDK version upgrades:
                - Newly expired options should be removed from the special_jvm_flags and aliased_jvm_flags tables.
                - Newly obsolete or expired deprecated options should have their global variable
                  definitions removed (from globals.hpp, etc) and related implementations removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

// UseContainerCpuShares was added in a patch release, thus IgnoreUnrecognizedVMOptions is required to avoid
// breaking distributions running with older JDKs.
private static final ImmutableList<String> forceUseContainerCpuShares =
ImmutableList.of("-XX:+IgnoreUnrecognizedVMOptions", "-XX:+UseContainerCpuShares");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with IgnoreUnrecognizedVMOptions because UseContainerCpuShares was added in the patch that changed behavior. It pains me a bit, but it's likely our best option.

oss latest-jdks is a bit behind, and didn't have the flag. Resolved that piece by merging palantir/gradle-jdks-latest#59 but we need to be careful about implicit dependencies on specific jdk builds.

Copy link
Contributor

@schlosna schlosna left a comment

Choose a reason for hiding this comment

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

Feel free to add the merge back when you're ready

@carterkozak
Copy link
Contributor Author

Internal results look great, pushing this through!

@carterkozak carterkozak marked this pull request as ready for review December 9, 2022 16:48
@bulldozer-bot bulldozer-bot bot merged commit 3f6811e into develop Dec 9, 2022
@bulldozer-bot bulldozer-bot bot deleted the ckozak/forceUseContainerCpuShares branch December 9, 2022 16:51
@svc-autorelease
Copy link
Collaborator

Released 7.25.0

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

Successfully merging this pull request may close these issues.

4 participants