Hybrid (G1) GC uses the defeault 200ms MaxGCPauseMillis on JDK-21+ #1706
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It seems that the heuristics have gone a bit sideways in JDK-21 causing degenerate full gc pauses unnecessarily when we configure `-XX:MaxGCPauseMillis=500`, however setting the default 200ms value resolves this behavior. Note that we deployed the increase from 200ms to 500ms at a time when container cpu shares informed the JDKs processor count, thus gc threads/etc, which is no longer the case. As such, it should be safe (and generally more stable) to use the hardened default values from the jdk.
Generate changelog in
|
carterkozak
commented
Sep 25, 2024
| return ImmutableList.of("-XX:+UseG1GC", "-XX:+UseNUMA", "-XX:MaxGCPauseMillis=" + maxGCPauseMillis); | ||
| public final List<String> gcJvmOpts(JavaVersion javaVersion) { | ||
| return ImmutableList.of( | ||
| "-XX:+UseG1GC", "-XX:+UseNUMA", "-XX:MaxGCPauseMillis=" + getMaxGCPauseMillis(javaVersion)); |
There was a problem hiding this comment.
Perhaps it would be best to exclude this parameter entirely when it's not specified? That way if the default changes in the jdk, we follow it. I don't have a strong preference -- we could update this once we drop support for jdk17 to exclude the value unless specified.
mpritham
reviewed
Sep 25, 2024
...g/src/test/groovy/com/palantir/gradle/dist/service/JavaServiceDistributionPluginTests.groovy
Outdated
Show resolved
Hide resolved
bmarcaur
reviewed
Oct 1, 2024
gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/gc/GcProfile.java
Outdated
Show resolved
Hide resolved
schlosna
reviewed
Oct 17, 2024
...g/src/test/groovy/com/palantir/gradle/dist/service/JavaServiceDistributionPluginTests.groovy
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems that the heuristics have gone a bit sideways in JDK-21 causing degenerate full gc pauses unnecessarily when we configure
-XX:MaxGCPauseMillis=500, however setting the default 200ms value resolves this behavior.Note that we deployed the increase from 200ms to 500ms at a time when container cpu shares informed the JDKs processor count, thus gc threads/etc, which is no longer the case. As such, it should be safe (and generally more stable) to use the hardened default values from the jdk.
==COMMIT_MSG==
Hybrid (G1) GC uses the defeault 200ms MaxGCPauseMillis on JDK-21+
==COMMIT_MSG==
Possible downsides?
Performance could change in unexpected ways!
Alternatives:
We could change the default across the board, however it's a bit safer to apply this more precisely to the java version where our previous default value causes problems.
We could slowly ratchet the value down over time, however that makes it more difficult to root cause changes, as they would be less abrupt. We're currently rolling this out explicitly (via the configuration dsl) in a subset of services of varying sizes to validate that metrics look the same or better before rolling this out.