From 3dc69515210897c00d645e323dff23dfd178970c Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 2 Nov 2022 11:20:38 -0700 Subject: [PATCH] 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 --- .../google/devtools/build/lib/profiler/BUILD | 1 + .../build/lib/profiler/MemoryProfiler.java | 89 ++++++++++++------- .../lib/runtime/CommonCommandOptions.java | 8 +- .../com/google/devtools/build/lib/util/BUILD | 15 +++- .../google/devtools/build/lib/profiler/BUILD | 1 + .../lib/profiler/MemoryProfilerTest.java | 72 +++++++++++++++ 6 files changed, 149 insertions(+), 37 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/profiler/BUILD b/src/main/java/com/google/devtools/build/lib/profiler/BUILD index a4e809d91c7b05..17f1f8fe957984 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java b/src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java index 48c05b8754fa4c..4cab893248576b 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java @@ -18,6 +18,7 @@ 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; @@ -25,7 +26,8 @@ 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 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> gcSpecs; - private MemoryProfileStableHeapParameters(int numTimesToDoGc, Duration timeToSleepBetweenGcs) { - this.numTimesToDoGc = numTimesToDoGc; - this.timeToSleepBetweenGcs = timeToSleepBetweenGcs; + private MemoryProfileStableHeapParameters(ArrayList> 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 values = SPLITTER.split(input).iterator(); + List values = SPLITTER.splitToList(input); + + if (values.size() % 2 != 0) { + throw new OptionsParsingException( + "Expected even number of comma-separated integer values"); + } + + ArrayList> 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(); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index 356735cac61f65..bf962d3a360f25 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/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( diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD index b86d8f373c2715..e3cfc44aefdbda 100644 --- a/src/main/java/com/google/devtools/build/lib/util/BUILD +++ b/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", diff --git a/src/test/java/com/google/devtools/build/lib/profiler/BUILD b/src/test/java/com/google/devtools/build/lib/profiler/BUILD index d2ddcec2a9215c..8e46dcce49273a 100644 --- a/src/test/java/com/google/devtools/build/lib/profiler/BUILD +++ b/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", diff --git a/src/test/java/com/google/devtools/build/lib/profiler/MemoryProfilerTest.java b/src/test/java/com/google/devtools/build/lib/profiler/MemoryProfilerTest.java index 57d2ad357f4f1e..4d3f4adba891c2 100644 --- a/src/test/java/com/google/devtools/build/lib/profiler/MemoryProfilerTest.java +++ b/src/test/java/com/google/devtools/build/lib/profiler/MemoryProfilerTest.java @@ -15,6 +15,7 @@ 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; @@ -22,6 +23,7 @@ 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 sleeps = new ArrayList<>();