From ecfb4caaddd20e008335da10ad0ee9ea91de1129 Mon Sep 17 00:00:00 2001
From: Carter Kozak <ckozak@palantir.com>
Date: Tue, 1 Aug 2023 14:05:19 -0400
Subject: [PATCH] Increase G1 default pause target from 200ms to 500ms (#1505)

Increase G1 default pause target from 200ms to 500ms
---
 changelog/@unreleased/pr-1505.v2.yml          | 13 +++++++
 .../gradle/dist/service/gc/GcProfile.java     | 16 +++++++-
 .../JavaServiceDistributionPluginTests.groovy | 39 ++++++++++++++++++-
 3 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100644 changelog/@unreleased/pr-1505.v2.yml

diff --git a/changelog/@unreleased/pr-1505.v2.yml b/changelog/@unreleased/pr-1505.v2.yml
new file mode 100644
index 000000000..9225b57a4
--- /dev/null
+++ b/changelog/@unreleased/pr-1505.v2.yml
@@ -0,0 +1,13 @@
+type: improvement
+improvement:
+  description: |-
+    Increase G1 default pause target from 200ms to 500ms
+
+    The G1 pause target can be modified thusly:
+    ```groovy
+    gc 'hybrid', {
+        maxGCPauseMillis 250
+    }
+    ```
+  links:
+  - https://github.com/palantir/sls-packaging/pull/1505
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 cbb39c6cc..ab3778a33 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
@@ -89,10 +89,24 @@ 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;
+
         @Override
         public final List<String> gcJvmOpts(JavaVersion _javaVersion) {
-            return ImmutableList.of("-XX:+UseG1GC", "-XX:+UseNUMA");
+            return ImmutableList.of("-XX:+UseG1GC", "-XX:+UseNUMA", "-XX:MaxGCPauseMillis=" + maxGCPauseMillis);
+        }
+
+        public final void maxGCPauseMillis(int value) {
+            this.maxGCPauseMillis = 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 ef7bc9c09..75d2609a6 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
@@ -1022,7 +1022,7 @@ class JavaServiceDistributionPluginTests extends GradleIntegrationSpec {
         !new File(projectDir, 'dist/service-name-0.0.1/service/lib/com/test/Test.class').exists()
     }
 
-    def 'adds gc profile jvm settings'() {
+    def 'adds initiatingOccupancyFraction gc profile jvm settings'() {
         given:
         buildFile << '''
             plugins {
@@ -1057,6 +1057,41 @@ class JavaServiceDistributionPluginTests extends GradleIntegrationSpec {
         actualStaticConfig.jvmOpts.containsAll(['-XX:+UseParNewGC', '-XX:+UseConcMarkSweepGC', '-XX:CMSInitiatingOccupancyFraction=75'])
     }
 
+    def 'adds maxGCPauseMillis gc profile jvm settings'() {
+        given:
+        buildFile << '''
+            plugins {
+                id 'java'
+                id 'com.palantir.sls-java-service-distribution'
+            }
+
+            repositories {
+                jcenter()
+                mavenCentral()
+            }
+
+            version '0.0.1'
+
+            distribution {
+                serviceName 'service-name'
+                mainClass 'test.Test'
+                gc 'hybrid', {
+                    maxGCPauseMillis 1234
+                }
+            }
+        '''.stripIndent()
+
+        createUntarTask(buildFile)
+
+        when:
+        runTasks(':untar')
+
+        then:
+        def actualStaticConfig = OBJECT_MAPPER.readValue(
+                file('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'() {
         given:
         buildFile << '''
@@ -1087,7 +1122,7 @@ class JavaServiceDistributionPluginTests extends GradleIntegrationSpec {
         then:
         def actualStaticConfig = OBJECT_MAPPER.readValue(
                 file('dist/service-name-0.0.1/service/bin/launcher-static.yml'), LaunchConfig.LaunchConfigInfo)
-        actualStaticConfig.jvmOpts.containsAll(['-XX:+UseG1GC', '-XX:+UseNUMA'])
+        actualStaticConfig.jvmOpts.containsAll(['-XX:+UseG1GC', '-XX:+UseNUMA', "-XX:MaxGCPauseMillis=500"])
     }
 
     def 'applies java agents'() {