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
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 b00f029..831e3ba 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 @@
}
@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 4e69bed..d9ce4f5 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 @@
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 @@
+ " 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 69c242f..4d94f86 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 @@
}
@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 21bf208..ec3c67e 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -630,8 +630,9 @@
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 9ba856d..06d0cb2 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 @@
}
@Test
- public void underThreshold_noOom() throws Exception {
+ public void underThreshold_noOom() {
options.oomMoreEagerlyThreshold = 99;
underTest.setOptions(options);
@@ -76,7 +72,7 @@
}
@Test
- public void overThreshold_oom() throws Exception {
+ public void overThreshold_oom() {
options.oomMoreEagerlyThreshold = 90;
underTest.setOptions(options);
@@ -95,7 +91,7 @@
}
@Test
- public void inactiveAfterOom() throws Exception {
+ public void inactiveAfterOom() {
options.oomMoreEagerlyThreshold = 90;
options.minTimeBetweenTriggeredGc = Duration.ZERO;
underTest.setOptions(options);
@@ -116,7 +112,7 @@
}
@Test
- public void externalGcNoTrigger() throws Exception {
+ public void externalGcNoTrigger() {
options.oomMoreEagerlyThreshold = 90;
underTest.setOptions(options);
@@ -131,7 +127,7 @@
}
@Test
- public void triggerReset() throws Exception {
+ public void triggerReset() {
options.oomMoreEagerlyThreshold = 90;
underTest.setOptions(options);
@@ -146,7 +142,7 @@
}
@Test
- public void triggerRaceWithOtherGc() throws Exception {
+ public void triggerRaceWithOtherGc() {
options.oomMoreEagerlyThreshold = 90;
underTest.setOptions(options);
@@ -160,7 +156,7 @@
}
@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 @@
}
@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 @@
}
@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 @@
}
@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 @@
}
@Test
- public void reportsMaxConsecutiveIgnored() throws Exception {
+ public void reportsMaxConsecutiveIgnored() {
options.oomMoreEagerlyThreshold = 90;
options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
underTest.setOptions(options);
@@ -269,7 +265,7 @@
}
@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 @@
}
@Test
- public void statsReset() throws Exception {
+ public void statsReset() {
options.oomMoreEagerlyThreshold = 90;
underTest.setOptions(options);
@@ -297,15 +293,6 @@
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();
}
