Change --memory_profile_stable_heap_parameters to accept more than one GC specification

Currently memory_profile_stable_heap_parameters expects 2 ints and runs that
many GCs with pauses between them 2nd param.

This CL doesn't change that, but allows any arbitrary number of pairs to be
provided that will run the same logic for each pair.  This allows experimenting
with forcing longer pauses on that thread before doing the quick GCs that allow
for cleaner memory measurement.

PiperOrigin-RevId: 485646588
Change-Id: Iff4f17cdaae409854f99397b4271bb5f87c4c404

Author: kkress

src/main/java/com/google/devtools/build/lib/profiler/BUILD

@@ -33,6 +33,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/unix:procmeminfo_parser",
         "//src/main/java/com/google/devtools/build/lib/util:os",
+        "//src/main/java/com/google/devtools/build/lib/util:pair",
         "//src/main/java/com/google/devtools/build/lib/worker:worker_metric",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:auto_value",

src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java

@@ -18,14 +18,16 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Splitter;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.io.OutputStream;
 import java.io.PrintStream;
 import java.lang.management.ManagementFactory;
 import java.lang.management.MemoryMXBean;
 import java.lang.management.MemoryUsage;
 import java.time.Duration;
-import java.util.Iterator;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.NoSuchElementException;
 import javax.annotation.Nullable;
 
@@ -111,14 +113,29 @@ synchronized HeapAndNonHeap prepareBeanAndGetLocalMinUsage(
     bean.gc();
     MemoryUsage minHeapUsed = bean.getHeapMemoryUsage();
     MemoryUsage minNonHeapUsed = bean.getNonHeapMemoryUsage();
+
     if (nextPhase == ProfilePhase.FINISH && memoryProfileStableHeapParameters != null) {
-      for (int i = 1; i < memoryProfileStableHeapParameters.numTimesToDoGc; i++) {
-        sleeper.sleep(memoryProfileStableHeapParameters.timeToSleepBetweenGcs);
-        bean.gc();
-        MemoryUsage currentHeapUsed = bean.getHeapMemoryUsage();
-        if (currentHeapUsed.getUsed() < minHeapUsed.getUsed()) {
-          minHeapUsed = currentHeapUsed;
-          minNonHeapUsed = bean.getNonHeapMemoryUsage();
+      for (int j = 0; j < memoryProfileStableHeapParameters.gcSpecs.size(); j++) {
+        Pair<Integer, Duration> spec = memoryProfileStableHeapParameters.gcSpecs.get(j);
+
+        int numTimesToDoGc = spec.first;
+        Duration timeToSleepBetweenGcs = spec.second;
+
+        for (int i = 0; i < numTimesToDoGc; i++) {
+          // We want to skip the first cycle for the first spec, since we ran a
+          // GC at the top of this function, but all the rest should get their
+          // proper runs
+          if (j == 0 && i == 0) {
+            continue;
+          }
+
+          sleeper.sleep(timeToSleepBetweenGcs);
+          bean.gc();
+          MemoryUsage currentHeapUsed = bean.getHeapMemoryUsage();
+          if (currentHeapUsed.getUsed() < minHeapUsed.getUsed()) {
+            minHeapUsed = currentHeapUsed;
+            minNonHeapUsed = bean.getNonHeapMemoryUsage();
+          }
         }
       }
     }
@@ -130,12 +147,10 @@ synchronized HeapAndNonHeap prepareBeanAndGetLocalMinUsage(
    * build.
    */
   public static class MemoryProfileStableHeapParameters {
-    private final int numTimesToDoGc;
-    private final Duration timeToSleepBetweenGcs;
+    private final ArrayList<Pair<Integer, Duration>> gcSpecs;
 
-    private MemoryProfileStableHeapParameters(int numTimesToDoGc, Duration timeToSleepBetweenGcs) {
-      this.numTimesToDoGc = numTimesToDoGc;
-      this.timeToSleepBetweenGcs = timeToSleepBetweenGcs;
+    private MemoryProfileStableHeapParameters(ArrayList<Pair<Integer, Duration>> gcSpecs) {
+      this.gcSpecs = gcSpecs;
     }
 
     /** Converter for {@code MemoryProfileStableHeapParameters} option. */
@@ -147,40 +162,48 @@ public static class Converter
       @Override
       public MemoryProfileStableHeapParameters convert(String input)
           throws OptionsParsingException {
-        Iterator<String> values = SPLITTER.split(input).iterator();
+        List<String> values = SPLITTER.splitToList(input);
+
+        if (values.size() % 2 != 0) {
+          throw new OptionsParsingException(
+              "Expected even number of comma-separated integer values");
+        }
+
+        ArrayList<Pair<Integer, Duration>> gcSpecs = new ArrayList<>(values.size() / 2);
+
         try {
-          int numTimesToDoGc = Integer.parseInt(values.next());
-          int numSecondsToSleepBetweenGcs = Integer.parseInt(values.next());
-          if (values.hasNext()) {
-            throw new OptionsParsingException("Expected exactly 2 comma-separated integer values");
+          for (int i = 0; i < values.size(); i += 2) {
+            int numTimesToDoGc = Integer.parseInt(values.get(i));
+            int numSecondsToSleepBetweenGcs = Integer.parseInt(values.get(i + 1));
+
+            if (numTimesToDoGc <= 0) {
+              throw new OptionsParsingException("Number of times to GC must be positive");
+            }
+            if (numSecondsToSleepBetweenGcs < 0) {
+              throw new OptionsParsingException(
+                  "Number of seconds to sleep between GC's must be non-negative");
+            }
+            gcSpecs.add(Pair.of(numTimesToDoGc, Duration.ofSeconds(numSecondsToSleepBetweenGcs)));
           }
-          if (numTimesToDoGc <= 0) {
-            throw new OptionsParsingException("Number of times to GC must be positive");
-          }
-          if (numSecondsToSleepBetweenGcs < 0) {
-            throw new OptionsParsingException(
-                "Number of seconds to sleep between GC's must be non-negative");
-          }
-          return new MemoryProfileStableHeapParameters(
-              numTimesToDoGc, Duration.ofSeconds(numSecondsToSleepBetweenGcs));
+
+          return new MemoryProfileStableHeapParameters(gcSpecs);
         } catch (NumberFormatException | NoSuchElementException nfe) {
           throw new OptionsParsingException(
-              "Expected exactly 2 comma-separated integer values", nfe);
+              "Expected even number of comma-separated integer values, could not parse integer in"
+                  + " list",
+              nfe);
         }
       }
 
       @Override
       public String getTypeDescription() {
-        return "two integers, separated by a comma";
+        return "integers, separated by a comma expected in pairs";
       }
     }
 
     @Override
     public String toString() {
-      return MoreObjects.toStringHelper(this)
-          .add("numTimesToDoGc", numTimesToDoGc)
-          .add("timeToSleepBetweenGcs", timeToSleepBetweenGcs)
-          .toString();
+      return MoreObjects.toStringHelper(this).add("gcSpecs", gcSpecs).toString();
     }
   }
 

src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java

@@ -337,9 +337,11 @@ public String getTypeDescription() {
       effectTags = {OptionEffectTag.BAZEL_MONITORING},
       converter = MemoryProfileStableHeapParameters.Converter.class,
       help =
-          "Tune memory profile's computation of stable heap at end of build. Should be two"
-              + " integers separated by a comma. First parameter is the number of GCs to perform."
-              + " Second parameter is the number of seconds to wait between GCs.")
+          "Tune memory profile's computation of stable heap at end of build. Should be and even"
+              + " number of  integers separated by commas. In each pair the first integer is the"
+              + " number of GCs to perform. The second integer in each pair is the number of"
+              + " seconds to wait between GCs. Ex: 2,4,4,0 would 2 GCs with a 4sec pause, followed"
+              + " by 4 GCs with zero second pause")
   public MemoryProfileStableHeapParameters memoryProfileStableHeapParameters;
 
   @Option(

src/main/java/com/google/devtools/build/lib/util/BUILD

@@ -142,6 +142,16 @@ java_library(
     ],
 )
 
+java_library(
+    name = "pair",
+    srcs = [
+        "Pair.java",
+    ],
+    deps = [
+        "//third_party:jsr305",
+    ],
+)
+
 java_library(
     name = "util",
     srcs = [
@@ -165,7 +175,6 @@ java_library(
         "OptionsUtils.java",
         "OrderedSetMultimap.java",
         "OsUtils.java",
-        "Pair.java",
         "PathFragmentFilter.java",
         "PersistentMap.java",
         "RegexFilter.java",
@@ -177,6 +186,10 @@ java_library(
         "TimeUtilities.java",
         "UserUtils.java",
     ],
+    exports = [
+        # vfs depends on the profiler and creates a cycle since we use Pair in profiler
+        ":pair",
+    ],
     deps = [
         ":os",
         ":shell_escaper",

src/test/java/com/google/devtools/build/lib/profiler/BUILD

@@ -28,6 +28,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/profiler:profiler-output",
         "//src/main/java/com/google/devtools/build/lib/profiler:system_network_stats",
         "//src/main/java/com/google/devtools/build/lib/worker:worker_metric",
+        "//src/main/java/com/google/devtools/common/options",
         "//src/test/java/com/google/devtools/build/lib/testutil",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
         "//third_party:guava",

src/test/java/com/google/devtools/build/lib/profiler/MemoryProfilerTest.java

@@ -15,13 +15,15 @@
 package com.google.devtools.build.lib.profiler;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.profiler.MemoryProfiler.MemoryProfileStableHeapParameters;
 import com.google.devtools.build.lib.profiler.MemoryProfiler.Sleeper;
+import com.google.devtools.common.options.OptionsParsingException;
 import java.lang.management.MemoryMXBean;
 import java.lang.management.MemoryUsage;
 import java.time.Duration;
@@ -96,6 +98,76 @@ public void profilerDoesOneGcAndNoSleepExceptInFinish() throws Exception {
     verify(bean, times(3)).getNonHeapMemoryUsage();
   }
 
+  @Test
+  public void profilerHasMultiplePairs() throws Exception {
+    MemoryProfiler profiler = MemoryProfiler.instance();
+    profiler.setStableMemoryParameters(
+        new MemoryProfileStableHeapParameters.Converter().convert("2,1,3,4,5,6"));
+    profiler.start(ByteStreams.nullOutputStream());
+    MemoryMXBean bean = Mockito.mock(MemoryMXBean.class);
+
+    MemoryUsage heapUsage = new MemoryUsage(0, 0, 0, 0);
+    MemoryUsage nonHeapUsage = new MemoryUsage(5, 5, 5, 5);
+    when(bean.getHeapMemoryUsage()).thenReturn(heapUsage);
+    when(bean.getNonHeapMemoryUsage()).thenReturn(nonHeapUsage);
+
+    RecordingSleeper sleeper = new RecordingSleeper();
+    MemoryProfiler.HeapAndNonHeap result =
+        profiler.prepareBeanAndGetLocalMinUsage(ProfilePhase.ANALYZE, bean, sleeper);
+    assertThat(result.getHeap()).isSameInstanceAs(heapUsage);
+    assertThat(result.getNonHeap()).isSameInstanceAs(nonHeapUsage);
+    assertThat(sleeper.sleeps).isEmpty();
+
+    verify(bean, times(1)).gc();
+    profiler.prepareBeanAndGetLocalMinUsage(ProfilePhase.FINISH, bean, sleeper);
+    // 1 for call to ANALYZE + spec'd runs
+    verify(bean, times(1 + 2 + 3 + 5)).gc();
+
+    assertThat(sleeper.sleeps)
+        .containsExactly(
+            Duration.ofSeconds(1), // 2 * 1s, but we skip the first sleep
+            Duration.ofSeconds(4), // 3 * 4s
+            Duration.ofSeconds(4),
+            Duration.ofSeconds(4),
+            Duration.ofSeconds(6), // 5 * 6s
+            Duration.ofSeconds(6),
+            Duration.ofSeconds(6),
+            Duration.ofSeconds(6),
+            Duration.ofSeconds(6))
+        .inOrder();
+  }
+
+  @Test
+  public void profilerHasBadInputOddValues() throws Exception {
+    MemoryProfiler profiler = MemoryProfiler.instance();
+    OptionsParsingException e =
+        assertThrows(
+            OptionsParsingException.class,
+            () ->
+                profiler.setStableMemoryParameters(
+                    new MemoryProfileStableHeapParameters.Converter().convert("1,10,7")));
+    assertThat(e)
+        .hasMessageThat()
+        .contains("Expected even number of comma-separated integer values");
+  }
+
+  @Test
+  public void profilerHasBadInputNotInts() throws Exception {
+    MemoryProfiler profiler = MemoryProfiler.instance();
+    OptionsParsingException e =
+        assertThrows(
+            OptionsParsingException.class,
+            () ->
+                profiler.setStableMemoryParameters(
+                    new MemoryProfileStableHeapParameters.Converter()
+                        .convert("1,10,74,22,horse,goat")));
+    assertThat(e)
+        .hasMessageThat()
+        .contains(
+            "Expected even number of comma-separated integer values, could not parse integer in"
+                + " list");
+  }
+
   private static class RecordingSleeper implements Sleeper {
     private final List<Duration> sleeps = new ArrayList<>();