Use PercentageConverter to automatically validate the value of `--e…

…xperimental_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

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<Class<? extends OptionsBase>> getCommandOptions(Command com
   }
 
   @Override
-  public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
+  public void beforeCommand(CommandEnvironment env) {
     eventBus = env.getEventBus();
     memoryPressureListener.setEventBus(eventBus);
 

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);
     }
   }

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) {

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];

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();
   }