From 132057a61d369e0946b59357bb9dcdc823fbf9db Mon Sep 17 00:00:00 2001
From: Andrew Azores <aazores@redhat.com>
Date: Fri, 1 Oct 2021 13:52:57 -0400
Subject: [PATCH] fix(reports): tune flags for reliability

---
 .../reports/SubprocessReportGenerator.java    | 28 ++++++++--------
 .../SubprocessReportGeneratorTest.java        | 32 +++++++++++++++----
 2 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java b/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java
index 0a9ddf6bfb..687bfa9064 100644
--- a/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java
+++ b/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java
@@ -45,6 +45,7 @@
 import java.nio.file.StandardCopyOption;
 import java.nio.file.StandardOpenOption;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
@@ -135,8 +136,7 @@ CompletableFuture<Path> exec(Path recording, Path saveFile, Duration timeout)
                         // See https://github.com/cryostatio/cryostat/issues/287
                         .jvmArgs(
                                 createJvmArgs(
-                                        Integer.parseInt(
-                                                env.getEnv(SUBPROCESS_MAX_HEAP_ENV, "200"))))
+                                        Integer.parseInt(env.getEnv(SUBPROCESS_MAX_HEAP_ENV, "0"))))
                         .processArgs(createProcessArgs(recording, saveFile))
                         .exec();
         return CompletableFuture.supplyAsync(
@@ -205,18 +205,18 @@ Path copyRecordingToFile(JFRConnection conn, String recordingName, Path path) th
     }
 
     private List<String> createJvmArgs(int maxHeapMegabytes) throws IOException {
-        // These JVM flags must be kept in-sync with the flags set on the parent process in
-        // entrypoint.sh in order to keep the auth and certs setup consistent
-        return List.of(
-                String.format("-Xmx%dM", maxHeapMegabytes),
-                "-XX:+ExitOnOutOfMemoryError",
-                // use EpsilonGC since we're a one-shot process and report generation doesn't
-                // allocate that much memory beyond the initial recording load - no point in
-                // over-complicating memory allocation. Preferable to just complete the request
-                // quickly, or if we're running up against the memory limit, fail early
-                "-XX:+UnlockExperimentalVMOptions",
-                "-XX:+UseEpsilonGC",
-                "-XX:+AlwaysPreTouch");
+        List<String> args = new ArrayList<>();
+        if (maxHeapMegabytes > 0) {
+            args.add(String.format("-Xms%dM", maxHeapMegabytes));
+            args.add(String.format("-Xmx%dM", maxHeapMegabytes));
+        }
+        args.add("-XX:+ExitOnOutOfMemoryError");
+        // use Serial GC since we have a small heap and likely little garbage to clean,
+        // and low GC overhead is more important here than minimizing pause time since the
+        // result will end up cached for subsequent user accesses so long as the process
+        // succeeds in the end
+        args.add("-XX:+UseSerialGC");
+        return args;
     }
 
     private List<String> createProcessArgs(Path recording, Path saveFile) {
diff --git a/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java b/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java
index 3cf57d01ab..c6c8525fca 100644
--- a/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java
+++ b/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java
@@ -118,7 +118,10 @@ void setup() throws Exception {
                 .thenReturn(javaProcessBuilder);
         Mockito.lenient().when(javaProcessBuilder.exec()).thenReturn(proc);
         Mockito.lenient()
-                .when(env.getEnv(SubprocessReportGenerator.SUBPROCESS_MAX_HEAP_ENV, "200"))
+                .when(
+                        env.getEnv(
+                                Mockito.eq(SubprocessReportGenerator.SUBPROCESS_MAX_HEAP_ENV),
+                                Mockito.anyString()))
                 .thenReturn("200");
         this.generator =
                 new SubprocessReportGenerator(
@@ -190,12 +193,27 @@ void shouldSetJvmArgs() throws Exception {
         Mockito.verify(javaProcessBuilder).jvmArgs(captor.capture());
 
         List<String> expected =
-                List.of(
-                        "-Xmx200M",
-                        "-XX:+ExitOnOutOfMemoryError",
-                        "-XX:+UnlockExperimentalVMOptions",
-                        "-XX:+UseEpsilonGC",
-                        "-XX:+AlwaysPreTouch");
+                List.of("-Xms200M", "-Xmx200M", "-XX:+ExitOnOutOfMemoryError", "-XX:+UseSerialGC");
+        MatcherAssert.assertThat(captor.getValue(), Matchers.equalTo(expected));
+    }
+
+    @Test
+    void shouldSetJvmArgsWithoutReportMaxHeapEnvVar() throws Exception {
+        Path dest = Mockito.mock(Path.class);
+        Mockito.when(dest.toAbsolutePath()).thenReturn(dest);
+        Mockito.when(dest.toString()).thenReturn("/dest/somefile.tmp");
+        Mockito.when(
+                        env.getEnv(
+                                Mockito.eq(SubprocessReportGenerator.SUBPROCESS_MAX_HEAP_ENV),
+                                Mockito.anyString()))
+                .thenReturn("0");
+
+        generator.exec(recordingFile, dest, Duration.ofSeconds(10));
+
+        ArgumentCaptor<List<String>> captor = ArgumentCaptor.forClass(List.class);
+        Mockito.verify(javaProcessBuilder).jvmArgs(captor.capture());
+
+        List<String> expected = List.of("-XX:+ExitOnOutOfMemoryError", "-XX:+UseSerialGC");
         MatcherAssert.assertThat(captor.getValue(), Matchers.equalTo(expected));
     }