From 4097d9c5e81413cdacb30d628340e15e06024402 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 29 Mar 2023 12:18:15 -0700 Subject: [PATCH] Use `PercentageConverter` to automatically validate the value of `--experimental_oom_more_eagerly_threshold`. There's no need to validate it manually. Also, this makes validation happen even if we disable `RetainedHeapLimiter` in favor of `GcThrashingDetector` (the flag is used by both classes). PiperOrigin-RevId: 520407887 Change-Id: I5a13d32b3207febc622cd55dbcc1b62a6f670d3c --- .../lib/runtime/MemoryPressureModule.java | 3 +- .../lib/runtime/MemoryPressureOptions.java | 8 ++-- .../lib/runtime/RetainedHeapLimiter.java | 21 +--------- src/main/protobuf/failure_details.proto | 5 ++- .../lib/runtime/RetainedHeapLimiterTest.java | 39 +++++++------------ 5 files changed, 23 insertions(+), 53 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java index b00f029c779d78..831e3badd60a79 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats; import com.google.devtools.build.lib.skyframe.HighWaterMarkLimiter; -import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.common.options.OptionsBase; import com.google.errorprone.annotations.Keep; @@ -47,7 +46,7 @@ public ImmutableList> getCommandOptions(Command com } @Override - public void beforeCommand(CommandEnvironment env) throws AbruptExitException { + public void beforeCommand(CommandEnvironment env) { eventBus = env.getEventBus(); memoryPressureListener.setEventBus(eventBus); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java index 4e69bed60a720c..d9ce4f50ff7a18 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java @@ -13,7 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.runtime; -import com.google.devtools.common.options.Converters; +import com.google.devtools.common.options.Converters.PercentageConverter; +import com.google.devtools.common.options.Converters.RangeConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -28,6 +29,7 @@ public final class MemoryPressureOptions extends OptionsBase { defaultValue = "100", documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, + converter = PercentageConverter.class, help = "If this flag is set to a value less than 100, Bazel will OOM if, after two full GC's, " + "more than this percentage of the (old gen) heap is still occupied.") @@ -89,8 +91,8 @@ public final class MemoryPressureOptions extends OptionsBase { + " threshold is exceeded.") public int skyframeHighWaterMarkFullGcDropsPerInvocation; - static class NonNegativeIntegerConverter extends Converters.RangeConverter { - public NonNegativeIntegerConverter() { + static final class NonNegativeIntegerConverter extends RangeConverter { + NonNegativeIntegerConverter() { super(0, Integer.MAX_VALUE); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java b/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java index 69c242ff43efb3..4d94f86604c254 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java @@ -25,10 +25,6 @@ import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.concurrent.ThreadSafety; import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats; -import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; -import com.google.devtools.build.lib.server.FailureDetails.MemoryOptions; -import com.google.devtools.build.lib.util.AbruptExitException; -import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.common.options.Options; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -74,25 +70,10 @@ private RetainedHeapLimiter(BugReporter bugReporter, Clock clock) { } @ThreadSafety.ThreadCompatible // Can only be called on the logical main Bazel thread. - void setOptions(MemoryPressureOptions options) throws AbruptExitException { - if (options.oomMoreEagerlyThreshold < 0 || options.oomMoreEagerlyThreshold > 100) { - throw createExitException( - "--experimental_oom_more_eagerly_threshold must be a percent between 0 and 100 but was " - + options.oomMoreEagerlyThreshold, - MemoryOptions.Code.EXPERIMENTAL_OOM_MORE_EAGERLY_THRESHOLD_INVALID_VALUE); - } + void setOptions(MemoryPressureOptions options) { this.options = options; } - private static AbruptExitException createExitException(String message, MemoryOptions.Code code) { - return new AbruptExitException( - DetailedExitCode.of( - FailureDetail.newBuilder() - .setMessage(message) - .setMemoryOptions(MemoryOptions.newBuilder().setCode(code)) - .build())); - } - // Can be called concurrently, handles concurrent calls with #setThreshold gracefully. @ThreadSafety.ThreadSafe public void handle(MemoryPressureEvent event) { diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 21bf20873867ee..ec3c67ea41e940 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -630,8 +630,9 @@ message InfoCommand { message MemoryOptions { enum Code { MEMORY_OPTIONS_UNKNOWN = 0 [(metadata) = { exit_code: 37 }]; - EXPERIMENTAL_OOM_MORE_EAGERLY_THRESHOLD_INVALID_VALUE = 1 - [(metadata) = { exit_code: 2 }]; + // Deprecated: validation is now implemented by the option converter. + DEPRECATED_EXPERIMENTAL_OOM_MORE_EAGERLY_THRESHOLD_INVALID_VALUE = 1 + [(metadata) = { exit_code: 2 }, deprecated = true]; // Deprecated: no tenured collectors found is now a crash on startup. DEPRECATED_EXPERIMENTAL_OOM_MORE_EAGERLY_NO_TENURED_COLLECTORS_FOUND = 2 [(metadata) = { exit_code: 2 }, deprecated = true]; diff --git a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java b/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java index 9ba856d97eb7c7..06d0cb2be12fb3 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java @@ -17,7 +17,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; -import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -27,11 +26,8 @@ import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.bugreport.Crash; import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats; -import com.google.devtools.build.lib.server.FailureDetails.MemoryOptions.Code; import com.google.devtools.build.lib.testutil.ManualClock; -import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.common.options.Options; -import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.lang.ref.WeakReference; import java.time.Duration; @@ -64,7 +60,7 @@ public void verifyNoMoreBugReports() { } @Test - public void underThreshold_noOom() throws Exception { + public void underThreshold_noOom() { options.oomMoreEagerlyThreshold = 99; underTest.setOptions(options); @@ -76,7 +72,7 @@ public void underThreshold_noOom() throws Exception { } @Test - public void overThreshold_oom() throws Exception { + public void overThreshold_oom() { options.oomMoreEagerlyThreshold = 90; underTest.setOptions(options); @@ -95,7 +91,7 @@ public void overThreshold_oom() throws Exception { } @Test - public void inactiveAfterOom() throws Exception { + public void inactiveAfterOom() { options.oomMoreEagerlyThreshold = 90; options.minTimeBetweenTriggeredGc = Duration.ZERO; underTest.setOptions(options); @@ -116,7 +112,7 @@ public void inactiveAfterOom() throws Exception { } @Test - public void externalGcNoTrigger() throws Exception { + public void externalGcNoTrigger() { options.oomMoreEagerlyThreshold = 90; underTest.setOptions(options); @@ -131,7 +127,7 @@ public void externalGcNoTrigger() throws Exception { } @Test - public void triggerReset() throws Exception { + public void triggerReset() { options.oomMoreEagerlyThreshold = 90; underTest.setOptions(options); @@ -146,7 +142,7 @@ public void triggerReset() throws Exception { } @Test - public void triggerRaceWithOtherGc() throws Exception { + public void triggerRaceWithOtherGc() { options.oomMoreEagerlyThreshold = 90; underTest.setOptions(options); @@ -160,7 +156,7 @@ public void triggerRaceWithOtherGc() throws Exception { } @Test - public void minTimeBetweenGc_lessThan_noGc() throws Exception { + public void minTimeBetweenGc_lessThan_noGc() { options.oomMoreEagerlyThreshold = 90; options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); underTest.setOptions(options); @@ -182,7 +178,7 @@ public void minTimeBetweenGc_lessThan_noGc() throws Exception { } @Test - public void minTimeBetweenGc_greaterThan_gc() throws Exception { + public void minTimeBetweenGc_greaterThan_gc() { options.oomMoreEagerlyThreshold = 90; options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); underTest.setOptions(options); @@ -204,7 +200,7 @@ public void minTimeBetweenGc_greaterThan_gc() throws Exception { } @Test - public void gcLockerDefersManualGc_timeoutCancelled() throws Exception { + public void gcLockerDefersManualGc_timeoutCancelled() { options.oomMoreEagerlyThreshold = 90; options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); underTest.setOptions(options); @@ -218,7 +214,7 @@ public void gcLockerDefersManualGc_timeoutCancelled() throws Exception { } @Test - public void gcLockerAfterSuccessfulManualGc_timeoutPreserved() throws Exception { + public void gcLockerAfterSuccessfulManualGc_timeoutPreserved() { options.oomMoreEagerlyThreshold = 90; options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); underTest.setOptions(options); @@ -233,7 +229,7 @@ public void gcLockerAfterSuccessfulManualGc_timeoutPreserved() throws Exception } @Test - public void reportsMaxConsecutiveIgnored() throws Exception { + public void reportsMaxConsecutiveIgnored() { options.oomMoreEagerlyThreshold = 90; options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); underTest.setOptions(options); @@ -269,7 +265,7 @@ public void reportsMaxConsecutiveIgnored() throws Exception { } @Test - public void threshold100_noGcTriggeredEvenWithNonsenseStats() throws Exception { + public void threshold100_noGcTriggeredEvenWithNonsenseStats() { options.oomMoreEagerlyThreshold = 100; underTest.setOptions(options); WeakReference ref = new WeakReference<>(new Object()); @@ -287,7 +283,7 @@ public void worksWithoutSettingOptions() { } @Test - public void statsReset() throws Exception { + public void statsReset() { options.oomMoreEagerlyThreshold = 90; underTest.setOptions(options); @@ -297,15 +293,6 @@ public void statsReset() throws Exception { assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0)); } - @Test - public void invalidThreshold_throws(@TestParameter({"-1", "101"}) int threshold) { - options.oomMoreEagerlyThreshold = threshold; - AbruptExitException e = - assertThrows(AbruptExitException.class, () -> underTest.setOptions(options)); - assertThat(e.getDetailedExitCode().getFailureDetail().getMemoryOptions().getCode()) - .isEqualTo(Code.EXPERIMENTAL_OOM_MORE_EAGERLY_THRESHOLD_INVALID_VALUE); - } - private static MemoryPressureEvent percentUsedAfterManualGc(int percentUsed) { return percentUsedAfterGc(percentUsed).setWasManualGc(true).setWasFullGc(true).build(); }