diff --git a/changelog/@unreleased/pr-1706.v2.yml b/changelog/@unreleased/pr-1706.v2.yml new file mode 100644 index 000000000..821617c0a --- /dev/null +++ b/changelog/@unreleased/pr-1706.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Hybrid (G1) GC uses the defeault 200ms MaxGCPauseMillis on JDK-21+ + links: + - https://github.com/palantir/sls-packaging/pull/1706 diff --git a/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/gc/GcProfile.java b/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/gc/GcProfile.java index f4dd24f93..b958d9625 100644 --- a/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/gc/GcProfile.java +++ b/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/gc/GcProfile.java @@ -22,6 +22,7 @@ import java.io.Serializable; import java.util.Collections; import java.util.List; +import java.util.OptionalInt; import org.gradle.api.JavaVersion; public interface GcProfile extends Serializable { @@ -102,21 +103,42 @@ public final void newRatio(int newerRatio) { // Match the MaxGCPauseMillis case @SuppressWarnings("AbbreviationAsWordInName") class Hybrid implements GcProfile { - // We use 500ms by default, up from the JDK default value of 200ms. Using G1, eden space is dynamically - // chosen based on the amount of memory which can be collected within the pause time target. - // Higher pause target values allow for more eden space, resulting in more stable old generation - // in high-garbage or low-gc-thread scenarios. In the happy case, increasing the pause target increases - // both throughput and latency. In degenerate cases, a low target can cause the garbage collector to - // thrash and reduce throughput while increasing latency. - private int maxGCPauseMillis = 500; + // For Java versions older than 21 we use 500ms by default, up from the JDK default value of 200ms for + // historical reasons. Java 21 builds seem to interact poorly with this value, causing frequent full + // GC cycles. The 500ms value was chosen at a time when GC thread counts were based on cpu shares + // rather than the underlying physical cores (prior to https://bugs.openjdk.org/browse/JDK-8281181), + // so services tended to thrash in GC to meet the 200ms threshold, promoting data too quickly to old gen. + // This is no longer the case, however we are leaving the Java 17 values where they've been, and making + // changes targeting Java 21 and beyond where we've observed significant improvements. + private static final int LEGACY_MAX_GC_PAUSE_MILLIS = 500; + + private OptionalInt maxGCPauseMillis = OptionalInt.empty(); @Override - public final List gcJvmOpts(JavaVersion _javaVersion) { - return ImmutableList.of("-XX:+UseG1GC", "-XX:+UseNUMA", "-XX:MaxGCPauseMillis=" + maxGCPauseMillis); + public final List gcJvmOpts(JavaVersion javaVersion) { + ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(3) + .add("-XX:+UseG1GC") + .add("-XX:+UseNUMA"); + OptionalInt maxGCPauseMillisValue = getMaxGCPauseMillis(javaVersion); + if (maxGCPauseMillisValue.isPresent()) { + builder.add("-XX:MaxGCPauseMillis=" + maxGCPauseMillisValue.getAsInt()); + } + return builder.build(); + } + + private OptionalInt getMaxGCPauseMillis(JavaVersion javaVersion) { + if (maxGCPauseMillis.isPresent()) { + return maxGCPauseMillis; + } + if (javaVersion.compareTo(JavaVersion.toVersion("21")) >= 0) { + return OptionalInt.empty(); + } else { + return OptionalInt.of(LEGACY_MAX_GC_PAUSE_MILLIS); + } } public final void maxGCPauseMillis(int value) { - this.maxGCPauseMillis = value; + this.maxGCPauseMillis = OptionalInt.of(value); } } diff --git a/gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/service/JavaServiceDistributionPluginTests.groovy b/gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/service/JavaServiceDistributionPluginTests.groovy index 0af2d8ca3..a933400f5 100644 --- a/gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/service/JavaServiceDistributionPluginTests.groovy +++ b/gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/service/JavaServiceDistributionPluginTests.groovy @@ -1285,6 +1285,7 @@ class JavaServiceDistributionPluginTests extends GradleIntegrationSpec { distribution { serviceName 'service-name' mainClass 'test.Test' + javaVersion 17 gc 'hybrid' } '''.stripIndent() @@ -1300,6 +1301,76 @@ class JavaServiceDistributionPluginTests extends GradleIntegrationSpec { actualStaticConfig.jvmOpts.containsAll(['-XX:+UseG1GC', '-XX:+UseNUMA', "-XX:MaxGCPauseMillis=500"]) } + def 'adds maxGCPauseMillis gc profile jvm settings on java 21'() { + given: + buildFile << ''' + plugins { + id 'java' + id 'com.palantir.sls-java-service-distribution' + } + + repositories { + mavenCentral() + } + + version '0.0.1' + + distribution { + serviceName 'service-name' + mainClass 'test.Test' + javaVersion 21 + gc 'hybrid', { + maxGCPauseMillis 1234 + } + } + '''.stripIndent() + + createUntarTask(buildFile) + + when: + runTasks(':untar') + + then: + def actualStaticConfig = OBJECT_MAPPER.readValue( + new File(projectDir, 'dist/service-name-0.0.1/service/bin/launcher-static.yml'), LaunchConfig.LaunchConfigInfo) + actualStaticConfig.jvmOpts.containsAll(['-XX:+UseG1GC', '-XX:+UseNUMA', '-XX:MaxGCPauseMillis=1234']) + } + + def 'gc profile null configuration closure on java 21'() { + // We do not declare MaxGCPauseMillis in JDK-21 unless a value is explicitly configured. + given: + buildFile << ''' + plugins { + id 'java' + id 'com.palantir.sls-java-service-distribution' + } + + repositories { + mavenCentral() + } + + version '0.0.1' + + distribution { + serviceName 'service-name' + mainClass 'test.Test' + javaVersion 21 + gc 'hybrid' + } + '''.stripIndent() + + createUntarTask(buildFile) + + when: + runTasks(':untar') + + then: + def actualStaticConfig = OBJECT_MAPPER.readValue( + new File(projectDir, 'dist/service-name-0.0.1/service/bin/launcher-static.yml'), LaunchConfig.LaunchConfigInfo) + actualStaticConfig.jvmOpts.containsAll(['-XX:+UseG1GC', '-XX:+UseNUMA']) + actualStaticConfig.jvmOpts().stream().noneMatch { it.contains("MaxGCPauseMillis") } + } + def 'applies java agents'() { createUntarBuildFile(buildFile) buildFile << """