Delete `RetainedHeapLimiter`.
It has been superseded by `GcThrashingDetector`, which is simpler and has proven to be more effective.
Rename the flags without the `experimental_` prefix, keeping the old names to aid in migration. A default value for `--gc_thrashing_limits` is applied in order to keep the existing behavior that specifying a value < 100 for the threshold activates detection.
PiperOrigin-RevId: 552809809
Change-Id: Iee53eaa28632da0750ccc09e815eebc479d13890
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 4412969..9657df3 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
@@ -360,18 +360,16 @@
+ " by 4 GCs with zero second pause")
public MemoryProfileStableHeapParameters memoryProfileStableHeapParameters;
-
@Option(
name = "heap_dump_on_oom",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.LOGGING,
effectTags = {OptionEffectTag.BAZEL_MONITORING},
help =
- "Whether to manually output a heap dump if an OOM is thrown (including OOMs due to"
- + " --experimental_oom_more_eagerly_threshold). The dump will be written to"
+ "Whether to manually output a heap dump if an OOM is thrown (including manual OOMs due to"
+ + " reaching --gc_thrashing_limits). The dump will be written to"
+ " <output_base>/<invocation_id>.heapdump.hprof. This option effectively replaces"
- + " -XX:+HeapDumpOnOutOfMemoryError, which has no effect because OOMs are caught and"
- + " redirected to Runtime#halt.")
+ + " -XX:+HeapDumpOnOutOfMemoryError, which has no effect for manual OOMs.")
public boolean heapDumpOnOom;
@Option(
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java b/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java
index 5c64839..68d6989 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java
@@ -66,12 +66,12 @@
/** If enabled in {@link MemoryPressureOptions}, creates a {@link GcThrashingDetector}. */
@Nullable
static GcThrashingDetector createForCommand(MemoryPressureOptions options) {
- if (options.gcThrashingLimits.isEmpty() || options.oomMoreEagerlyThreshold == 100) {
+ if (options.gcThrashingLimits.isEmpty() || options.gcThrashingThreshold == 100) {
return null;
}
return new GcThrashingDetector(
- options.oomMoreEagerlyThreshold,
+ options.gcThrashingThreshold,
options.gcThrashingLimits,
BlazeClock.instance(),
BugReporter.defaultInstance());
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java
index b85eeff..5003978 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java
@@ -42,19 +42,16 @@
final class MemoryPressureListener implements NotificationListener {
private final AtomicReference<EventBus> eventBus = new AtomicReference<>();
- private final RetainedHeapLimiter retainedHeapLimiter;
private final AtomicReference<GcThrashingDetector> gcThrashingDetector = new AtomicReference<>();
private final Executor executor;
- private MemoryPressureListener(RetainedHeapLimiter retainedHeapLimiter, Executor executor) {
- this.retainedHeapLimiter = retainedHeapLimiter;
+ private MemoryPressureListener(Executor executor) {
this.executor = executor;
}
- static MemoryPressureListener create(RetainedHeapLimiter retainedHeapLimiter) {
+ static MemoryPressureListener create() {
return createFromBeans(
ManagementFactory.getGarbageCollectorMXBeans(),
- retainedHeapLimiter,
// Use a dedicated thread to broadcast memory pressure events. The service thread that calls
// handleNotification for GC events is not a typical Java thread - it doesn't work with
// debugger breakpoints and may not show up in thread dumps.
@@ -65,7 +62,6 @@
@VisibleForTesting
static MemoryPressureListener createFromBeans(
List<GarbageCollectorMXBean> gcBeans,
- RetainedHeapLimiter retainedHeapLimiter,
Executor executor) {
ImmutableList<NotificationEmitter> tenuredGcEmitters = findTenuredCollectorBeans(gcBeans);
if (tenuredGcEmitters.isEmpty()) {
@@ -79,8 +75,7 @@
"Unable to find tenured collector from %s: names were %s.", gcBeans, names));
}
- MemoryPressureListener memoryPressureListener =
- new MemoryPressureListener(retainedHeapLimiter, executor);
+ MemoryPressureListener memoryPressureListener = new MemoryPressureListener(executor);
tenuredGcEmitters.forEach(e -> e.addNotificationListener(memoryPressureListener, null, null));
return memoryPressureListener;
}
@@ -150,14 +145,10 @@
}
// A null EventBus implies memory pressure event between commands with no active EventBus.
- // In such cases, notify RetainedHeapLimiter but do not publish event.
EventBus eventBus = this.eventBus.get();
if (eventBus != null) {
eventBus.post(event);
}
- // Post to EventBus first so memory pressure subscribers have a chance to make things
- // eligible for GC before RetainedHeapLimiter would trigger a full GC.
- this.retainedHeapLimiter.handle(event);
}
void setEventBus(@Nullable EventBus eventBus) {
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 0ba6a8b..d1850ca 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
@@ -17,7 +17,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.EventBus;
import com.google.common.eventbus.Subscribe;
-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.common.options.OptionsBase;
@@ -28,19 +27,11 @@
* pressure events.
*/
public final class MemoryPressureModule extends BlazeModule {
- private RetainedHeapLimiter retainedHeapLimiter;
- private MemoryPressureListener memoryPressureListener;
+ private final MemoryPressureListener memoryPressureListener = MemoryPressureListener.create();
private HighWaterMarkLimiter highWaterMarkLimiter;
private EventBus eventBus;
@Override
- public void workspaceInit(
- BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) {
- retainedHeapLimiter = RetainedHeapLimiter.create(runtime.getBugReporter());
- memoryPressureListener = MemoryPressureListener.create(retainedHeapLimiter);
- }
-
- @Override
public ImmutableList<Class<? extends OptionsBase>> getCommandOptions(Command command) {
return ImmutableList.of(MemoryPressureOptions.class);
}
@@ -54,7 +45,6 @@
highWaterMarkLimiter =
new HighWaterMarkLimiter(env.getSkyframeExecutor(), env.getSyscallCache(), options);
memoryPressureListener.setGcThrashingDetector(GcThrashingDetector.createForCommand(options));
- retainedHeapLimiter.setOptions(options);
eventBus.register(this);
eventBus.register(highWaterMarkLimiter);
@@ -77,7 +67,6 @@
private void postStats() {
MemoryPressureStats.Builder stats = MemoryPressureStats.newBuilder();
- retainedHeapLimiter.addStatsAndReset(stats);
highWaterMarkLimiter.addStatsAndReset(stats);
eventBus.post(stats.build());
}
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 aa7f138..d9222f7 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
@@ -30,25 +30,6 @@
public final class MemoryPressureOptions extends OptionsBase {
@Option(
- name = "experimental_oom_more_eagerly_threshold",
- 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.")
- public int oomMoreEagerlyThreshold;
-
- @Option(
- name = "min_time_between_triggered_gc",
- defaultValue = "1m",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
- help = "The minimum amount of time between GCs triggered by RetainedHeapLimiter.")
- public Duration minTimeBetweenTriggeredGc;
-
- @Option(
name = "skyframe_high_water_mark_threshold",
defaultValue = "85",
documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
@@ -97,30 +78,35 @@
public int skyframeHighWaterMarkFullGcDropsPerInvocation;
@Option(
- name = "experimental_gc_thrashing_limits",
- defaultValue = "",
+ name = "gc_thrashing_limits",
+ oldName = "experimental_gc_thrashing_limits",
+ oldNameWarning = false,
+ defaultValue = "1s:2,20s:3,1m:5",
documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS},
converter = GcThrashingLimitsConverter.class,
help =
"Limits which, if reached, cause GcThrashingDetector to crash Bazel with an OOM. Each"
+ " limit is specified as <period>:<count> where period is a duration and count is a"
- + " positive integer. If more than --experimental_oom_more_eagerly_threshold percent"
- + " of tenured space (old gen heap) remains occupied after <count> consecutive full"
- + " GCs within <period>, an OOM is triggered. Multiple limits can be specified"
- + " separated by commas.")
+ + " positive integer. If more than --gc_thrashing_threshold percent of tenured space"
+ + " (old gen heap) remains occupied after <count> consecutive full GCs within"
+ + " <period>, an OOM is triggered. Multiple limits can be specified separated by"
+ + " commas.")
public ImmutableList<GcThrashingDetector.Limit> gcThrashingLimits;
@Option(
- name = "gc_thrashing_limits_retained_heap_limiter_mutually_exclusive",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
+ name = "gc_thrashing_threshold",
+ oldName = "experimental_oom_more_eagerly_threshold",
+ oldNameWarning = false,
+ defaultValue = "100",
+ documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS},
+ converter = PercentageConverter.class,
help =
- "If true, specifying non-empty --experimental_gc_thrashing_limits deactivates"
- + " RetainedHeapLimiter to make it mutually exclusive with GcThrashingDetector."
- + " Setting to false permits both to be active for the same command.")
- public boolean gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive;
+ "The percent of tenured space occupied (0-100) above which GcThrashingDetector considers"
+ + " memory pressure events against its limits (--gc_thrashing_limits). If set to 100,"
+ + " GcThrashingDetector is disabled.")
+ public int gcThrashingThreshold;
static final class NonNegativeIntegerConverter extends RangeConverter {
NonNegativeIntegerConverter() {
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
deleted file mode 100644
index a59b54e..0000000
--- a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java
+++ /dev/null
@@ -1,183 +0,0 @@
-// Copyright 2016 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.devtools.build.lib.runtime;
-
-import static com.google.common.base.Preconditions.checkNotNull;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.bugreport.BugReporter;
-import com.google.devtools.build.lib.bugreport.Crash;
-import com.google.devtools.build.lib.bugreport.CrashContext;
-import com.google.devtools.build.lib.clock.BlazeClock;
-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.common.options.Options;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicLong;
-
-/**
- * Monitors the size of the retained heap and exit promptly if it grows too large.
- *
- * <p>Specifically, checks the size of the tenured space after each major GC; if it exceeds {@link
- * MemoryPressureOptions#oomMoreEagerlyThreshold}%, call {@link System#gc()} to trigger a
- * stop-the-world collection; if it's still more than {@link
- * MemoryPressureOptions#oomMoreEagerlyThreshold}% full, exit with an {@link OutOfMemoryError}.
- */
-final class RetainedHeapLimiter implements MemoryPressureStatCollector {
-
- private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
-
- private final BugReporter bugReporter;
- private final Clock clock;
-
- private volatile MemoryPressureOptions options = inactiveOptions();
-
- private final AtomicBoolean throwingOom = new AtomicBoolean(false);
- private final AtomicBoolean heapLimiterTriggeredGc = new AtomicBoolean(false);
- private final AtomicInteger consecutiveIgnoredFullGcsOverThreshold = new AtomicInteger(0);
- private final AtomicBoolean loggedIgnoreWarningSinceLastGc = new AtomicBoolean(false);
- private final AtomicLong lastTriggeredGcMillis = new AtomicLong();
- private final AtomicInteger gcsTriggered = new AtomicInteger(0);
- private final AtomicInteger maxConsecutiveIgnoredFullGcsOverThreshold = new AtomicInteger(0);
-
- static RetainedHeapLimiter create(BugReporter bugReporter) {
- return new RetainedHeapLimiter(bugReporter, BlazeClock.instance());
- }
-
- @VisibleForTesting
- static RetainedHeapLimiter createForTest(BugReporter bugReporter, Clock clock) {
- return new RetainedHeapLimiter(bugReporter, clock);
- }
-
- private RetainedHeapLimiter(BugReporter bugReporter, Clock clock) {
- this.bugReporter = checkNotNull(bugReporter);
- this.clock = checkNotNull(clock);
- }
-
- @ThreadSafety.ThreadCompatible // Can only be called on the logical main Bazel thread.
- void setOptions(MemoryPressureOptions options) {
- if (options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive
- && !options.gcThrashingLimits.isEmpty()) {
- this.options = inactiveOptions();
- } else {
- this.options = options;
- }
- }
-
- // Can be called concurrently, handles concurrent calls with #setThreshold gracefully.
- @ThreadSafety.ThreadSafe
- public void handle(MemoryPressureEvent event) {
- if (throwingOom.get()) {
- return; // Do nothing if a crash is already in progress.
- }
-
- boolean wasHeapLimiterTriggeredGc = false;
- boolean wasGcLockerDeferredHeapLimiterTriggeredGc = false;
- if (event.wasManualGc()) {
- wasHeapLimiterTriggeredGc = heapLimiterTriggeredGc.getAndSet(false);
- if (!wasHeapLimiterTriggeredGc) {
- // This was a manually triggered GC, but not from us earlier: short-circuit.
- logger.atInfo().log("Ignoring manual GC from other source");
- return;
- }
- } else if (event.wasGcLockerInitiatedGc() && heapLimiterTriggeredGc.getAndSet(false)) {
- // If System.gc() is called was while there are JNI thread(s) in the critical region, GCLocker
- // defers the GC until those threads exit the critical region. However, all GCLocker initiated
- // GCs are minor evacuation pauses, so we won't get the full GC we requested. Cancel the
- // timeout so we can attempt System.gc() again if we're still over the threshold. See full
- // explanation in b/263405096#comment14.
- logger.atWarning().log(
- "Observed a GCLocker initiated GC without observing a manual GC since the last call to"
- + " System.gc(), cancelling timeout to permit a retry");
- wasGcLockerDeferredHeapLimiterTriggeredGc = true;
- lastTriggeredGcMillis.set(0);
- }
-
- // Get a local reference to guard against concurrent modifications.
- MemoryPressureOptions options = this.options;
- int threshold = options.oomMoreEagerlyThreshold;
-
- if (threshold == 100) {
- return; // Inactive.
- }
-
- int actual = event.percentTenuredSpaceUsed();
- if (actual < threshold) {
- if (wasHeapLimiterTriggeredGc || wasGcLockerDeferredHeapLimiterTriggeredGc) {
- logger.atInfo().log("Back under threshold (%s%% of tenured space)", actual);
- }
- consecutiveIgnoredFullGcsOverThreshold.set(0);
- return;
- }
-
- if (wasHeapLimiterTriggeredGc) {
- if (!throwingOom.getAndSet(true)) {
- // We got here from a GC initiated by the other branch.
- OutOfMemoryError oom =
- new OutOfMemoryError(
- String.format(
- "RetainedHeapLimiter forcing exit due to GC thrashing: After back-to-back full"
- + " GCs, the tenured space is more than %s%% occupied (%s out of a tenured"
- + " space size of %s).",
- threshold, event.tenuredSpaceUsedBytes(), event.tenuredSpaceMaxBytes()));
- logger.atInfo().log("Calling handleCrash");
- // Exits the runtime.
- bugReporter.handleCrash(Crash.from(oom), CrashContext.halt());
- }
- } else if (clock.currentTimeMillis() - lastTriggeredGcMillis.get()
- > options.minTimeBetweenTriggeredGc.toMillis()) {
- logger.atInfo().log(
- "Triggering a full GC (%s%% of tenured space after %s GC)",
- actual, event.wasFullGc() ? "full" : "minor");
- heapLimiterTriggeredGc.set(true);
- gcsTriggered.incrementAndGet();
- // Force a full stop-the-world GC and see if it can get us below the threshold.
- System.gc();
- lastTriggeredGcMillis.set(clock.currentTimeMillis());
- consecutiveIgnoredFullGcsOverThreshold.set(0);
- loggedIgnoreWarningSinceLastGc.set(false);
- } else if (event.wasFullGc()) {
- int consecutiveIgnored = consecutiveIgnoredFullGcsOverThreshold.incrementAndGet();
- maxConsecutiveIgnoredFullGcsOverThreshold.accumulateAndGet(consecutiveIgnored, Math::max);
- logger.atWarning().log(
- "Ignoring possible GC thrashing x%s (%s%% of tenured space after full GC) because of"
- + " recently triggered GC",
- consecutiveIgnored, actual);
- } else if (!loggedIgnoreWarningSinceLastGc.getAndSet(true)) {
- logger.atWarning().log(
- "Ignoring possible GC thrashing (%s%% of tenured space after minor GC) because of"
- + " recently triggered GC",
- actual);
- }
- }
-
- @Override
- public void addStatsAndReset(MemoryPressureStats.Builder stats) {
- stats
- .setManuallyTriggeredGcs(gcsTriggered.getAndSet(0))
- .setMaxConsecutiveIgnoredGcsOverThreshold(
- maxConsecutiveIgnoredFullGcsOverThreshold.getAndSet(0));
- consecutiveIgnoredFullGcsOverThreshold.set(0);
- }
-
- private static MemoryPressureOptions inactiveOptions() {
- var options = Options.getDefaults(MemoryPressureOptions.class);
- options.oomMoreEagerlyThreshold = 100;
- return options;
- }
-}
diff --git a/src/main/protobuf/memory_pressure.proto b/src/main/protobuf/memory_pressure.proto
index 97a8d20..63ec9b7 100644
--- a/src/main/protobuf/memory_pressure.proto
+++ b/src/main/protobuf/memory_pressure.proto
@@ -20,11 +20,12 @@
// Statistics about memory pressure handling.
message MemoryPressureStats {
- // The number of calls to System.gc() made by RetainedHeapLimiter.
- int32 manually_triggered_gcs = 1;
- // The maximum number of consecutive full GC events over the threshold ignored
- // by RetainedHeapLimiter because it recently triggered a GC.
- int32 max_consecutive_ignored_gcs_over_threshold = 2;
+ // Deprecated: An artifact of RetainedHeapLimiter. GcThrashingDetector (its
+ // replacement) does not manually trigger GC.
+ int32 manually_triggered_gcs = 1 [deprecated = true];
+ // Deprecated: An artifact of RetainedHeapLimiter. GcThrashingDetector (its
+ // replacement) does not ignore any non-manual full GC.
+ int32 max_consecutive_ignored_gcs_over_threshold = 2 [deprecated = true];
// Number of times HighWaterMarkLimiter dropped caches after minor GCs.
int32 minor_gc_drops = 3;
// Number of times HighWaterMarkLimiter dropped caches after full GCs.
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
index c34ff74..dfea5f7 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
@@ -729,7 +729,7 @@
testOom(
() -> {
OutOfMemoryError oom = new OutOfMemoryError();
- // Simulates an OOM coming from RetainedHeapLimiter, which reports the error by calling
+ // Simulates an OOM coming from GcThrashingDetector, which reports the error by calling
// handleCrash. Uses keepAlive() to avoid exiting the JVM and aborting the test, then
// throw the original oom to ensure control flow terminates.
BugReport.handleCrash(Crash.from(oom), CrashContext.keepAlive());
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java
index 4340537..eb50b46 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java
@@ -28,11 +28,12 @@
import com.google.common.eventbus.EventBus;
import com.google.common.eventbus.Subscribe;
import com.google.devtools.build.lib.bugreport.BugReporter;
-import com.google.devtools.common.options.Options;
+import com.google.devtools.build.lib.clock.BlazeClock;
import com.sun.management.GarbageCollectionNotificationInfo;
import com.sun.management.GcInfo;
import java.lang.management.GarbageCollectorMXBean;
import java.lang.management.MemoryUsage;
+import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import javax.management.Notification;
@@ -52,7 +53,6 @@
private final NotificationBean mockUselessBean = mock(NotificationBean.class);
private final BugReporter bugReporter = mock(BugReporter.class);
private final EventBus eventBus = new EventBus();
- private final RetainedHeapLimiter retainedHeapLimiter = RetainedHeapLimiter.create(bugReporter);
private final List<MemoryPressureEvent> events = new ArrayList<>();
@Before
@@ -87,14 +87,14 @@
IllegalStateException.class,
() ->
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean), retainedHeapLimiter, directExecutor()));
+ ImmutableList.of(mockUselessBean), directExecutor()));
}
@Test
public void simple() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor());
+ ImmutableList.of(mockUselessBean, mockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
verify(mockUselessBean, never()).addNotificationListener(any(), any(), any());
@@ -134,7 +134,7 @@
public void nullEventBus_doNotPublishEvent() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor());
+ ImmutableList.of(mockUselessBean, mockBean), directExecutor());
verify(mockBean).addNotificationListener(underTest, null, null);
verify(mockUselessBean, never()).addNotificationListener(any(), any(), any());
@@ -166,8 +166,7 @@
@Test
public void manualGc() {
MemoryPressureListener underTest =
- MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockBean), retainedHeapLimiter, directExecutor());
+ MemoryPressureListener.createFromBeans(ImmutableList.of(mockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
@@ -198,8 +197,7 @@
@Test
public void doesntInvokeHandlerWhenTenuredSpaceMaxSizeIsZero() {
MemoryPressureListener underTest =
- MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockBean), retainedHeapLimiter, directExecutor());
+ MemoryPressureListener.createFromBeans(ImmutableList.of(mockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
@@ -229,7 +227,7 @@
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockBean, anotherMockBean), retainedHeapLimiter, directExecutor());
+ ImmutableList.of(mockBean, anotherMockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
verify(anotherMockBean).addNotificationListener(underTest, null, null);
@@ -267,18 +265,21 @@
}
@Test
- public void retainedHeapLimiter_aboveThreshold_handleCrash() {
- MemoryPressureOptions options = Options.getDefaults(MemoryPressureOptions.class);
- options.oomMoreEagerlyThreshold = 90;
- retainedHeapLimiter.setOptions(options);
-
+ public void gcThrashingDetector_limitMet_handleCrash() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor());
- underTest.setEventBus(eventBus);
+ ImmutableList.of(mockUselessBean, mockBean), directExecutor());
verify(mockBean).addNotificationListener(underTest, null, null);
verify(mockUselessBean, never()).addNotificationListener(any(), any(), any());
+ underTest.setEventBus(eventBus);
+ underTest.setGcThrashingDetector(
+ new GcThrashingDetector(
+ /* threshold= */ 90,
+ ImmutableList.of(GcThrashingDetector.Limit.of(Duration.ofMinutes(1), 1)),
+ BlazeClock.instance(),
+ bugReporter));
+
GcInfo mockGcInfo = mock(GcInfo.class);
String nonTenuredSpaceName = "nope";
MemoryUsage mockMemoryUsageForNonTenuredSpace = mock(MemoryUsage.class);
@@ -296,6 +297,7 @@
MemoryPressureEvent event =
MemoryPressureEvent.newBuilder()
.setWasManualGc(false)
+ .setWasFullGc(true)
.setTenuredSpaceUsedBytes(99)
.setTenuredSpaceMaxBytes(100)
.build();
@@ -303,27 +305,10 @@
new Notification(
GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION, "test", 123);
notification.setUserData(
- new GarbageCollectionNotificationInfo("gcName", "gcAction", "non-manual", mockGcInfo)
+ new GarbageCollectionNotificationInfo("gcName", "end of major GC", "non-manual", mockGcInfo)
.toCompositeData(null));
underTest.handleNotification(notification, null);
assertThat(events).containsExactly(event);
-
- MemoryPressureEvent manualGcEvent =
- MemoryPressureEvent.newBuilder()
- .setWasManualGc(true)
- .setTenuredSpaceUsedBytes(99)
- .setTenuredSpaceMaxBytes(100)
- .build();
- Notification manualGCNotification =
- new Notification(
- GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION, "test", 123);
- manualGCNotification.setUserData(
- new GarbageCollectionNotificationInfo("gcName", "gcAction", "System.gc()", mockGcInfo)
- .toCompositeData(null));
- underTest.handleNotification(manualGCNotification, null);
-
- verify(bugReporter).handleCrash(any(), any());
- assertThat(events).containsExactly(event, manualGcEvent);
}
}
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
deleted file mode 100644
index 632b309..0000000
--- a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java
+++ /dev/null
@@ -1,346 +0,0 @@
-// Copyright 2019 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.devtools.build.lib.runtime;
-
-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.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoInteractions;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
-
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.bugreport.BugReporter;
-import com.google.devtools.build.lib.bugreport.Crash;
-import com.google.devtools.build.lib.runtime.GcThrashingDetector.Limit;
-import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats;
-import com.google.devtools.build.lib.testutil.ManualClock;
-import com.google.devtools.common.options.Options;
-import com.google.testing.junit.testparameterinjector.TestParameterInjector;
-import java.lang.ref.WeakReference;
-import java.time.Duration;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.ArgumentCaptor;
-import org.mockito.ArgumentMatchers;
-
-/** Tests for {@link RetainedHeapLimiter}. */
-@RunWith(TestParameterInjector.class)
-public final class RetainedHeapLimiterTest {
-
- private final BugReporter bugReporter = mock(BugReporter.class);
- private final ManualClock clock = new ManualClock();
- private final MemoryPressureOptions options = Options.getDefaults(MemoryPressureOptions.class);
-
- private final RetainedHeapLimiter underTest =
- RetainedHeapLimiter.createForTest(bugReporter, clock);
-
- @Before
- public void setClock() {
- clock.advanceMillis(100000);
- }
-
- @After
- public void verifyNoMoreBugReports() {
- verifyNoMoreInteractions(bugReporter);
- }
-
- @Test
- public void underThreshold_noOom() {
- options.oomMoreEagerlyThreshold = 99;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(100));
- underTest.handle(percentUsedAfterManualGc(89));
-
- verifyNoInteractions(bugReporter);
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void overThreshold_oom() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- // Triggers GC, and tells RetainedHeapLimiter to OOM if too much memory used next time.
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- underTest.handle(percentUsedAfterManualGc(91));
-
- ArgumentCaptor<Crash> crashArgument = ArgumentCaptor.forClass(Crash.class);
- verify(bugReporter).handleCrash(crashArgument.capture(), ArgumentMatchers.any());
- OutOfMemoryError oom = (OutOfMemoryError) crashArgument.getValue().getThrowable();
- assertThat(oom).hasMessageThat().contains("forcing exit due to GC thrashing");
- assertThat(oom).hasMessageThat().contains("tenured space is more than 90% occupied");
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void inactiveAfterOom() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ZERO;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(91));
- verify(bugReporter).handleCrash(any(), any());
-
- // No more GC or bug reports even if notifications come in after an OOM is in progress.
- WeakReference<?> ref = new WeakReference<>(new Object());
- clock.advanceMillis(Duration.ofMinutes(1).toMillis());
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(91));
- assertThat(ref.get()).isNotNull();
- verifyNoMoreBugReports();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void externalGcNoTrigger() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- // No trigger because cause was "System.gc()".
- underTest.handle(percentUsedAfterManualGc(91));
-
- // Proof: no OOM.
- underTest.handle(percentUsedAfterManualGc(91));
- verifyNoInteractions(bugReporter);
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void triggerReset() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- // Got under the threshold, so no OOM.
- underTest.handle(percentUsedAfterManualGc(89));
-
- // No OOM this time since wasn't triggered.
- underTest.handle(percentUsedAfterManualGc(91));
- verifyNoInteractions(bugReporter);
- }
-
- @Test
- public void triggerRaceWithOtherGc() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(91));
-
- ArgumentCaptor<Crash> crashArgument = ArgumentCaptor.forClass(Crash.class);
- verify(bugReporter).handleCrash(crashArgument.capture(), ArgumentMatchers.any());
- assertThat(crashArgument.getValue().getThrowable()).isInstanceOf(OutOfMemoryError.class);
- }
-
- @Test
- public void minTimeBetweenGc_lessThan_noGc() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
- WeakReference<?> ref = new WeakReference<>(new Object());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNull();
- underTest.handle(percentUsedAfterManualGc(89));
-
- ref = new WeakReference<>(new Object());
- clock.advanceMillis(Duration.ofSeconds(59).toMillis());
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNotNull();
-
- assertStats(
- MemoryPressureStats.newBuilder()
- .setManuallyTriggeredGcs(1)
- .setMaxConsecutiveIgnoredGcsOverThreshold(1));
- }
-
- @Test
- public void minTimeBetweenGc_greaterThan_gc() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
- WeakReference<?> ref = new WeakReference<>(new Object());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNull();
- underTest.handle(percentUsedAfterManualGc(89));
-
- ref = new WeakReference<>(new Object());
- clock.advanceMillis(Duration.ofSeconds(61).toMillis());
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNull();
-
- assertStats(
- MemoryPressureStats.newBuilder()
- .setManuallyTriggeredGcs(2)
- .setMaxConsecutiveIgnoredGcsOverThreshold(0));
- }
-
- @Test
- public void gcLockerDefersManualGc_timeoutCancelled() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- WeakReference<?> ref = new WeakReference<>(new Object());
- underTest.handle(percentUsedAfterGcLockerGc(91));
- assertThat(ref.get()).isNull();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(2));
- }
-
- @Test
- public void gcLockerAfterSuccessfulManualGc_timeoutPreserved() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(89));
- WeakReference<?> ref = new WeakReference<>(new Object());
- underTest.handle(percentUsedAfterGcLockerGc(91));
- assertThat(ref.get()).isNotNull();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void reportsMaxConsecutiveIgnored() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(89));
- for (int i = 0; i < 6; i++) {
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- }
-
- clock.advanceMillis(Duration.ofMinutes(2).toMillis());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(89));
- for (int i = 0; i < 8; i++) {
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- }
- underTest.handle(percentUsedAfterOrganicFullGc(89)); // Breaks the streak of over threshold GCs.
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- clock.advanceMillis(Duration.ofMinutes(2).toMillis());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterOrganicFullGc(89));
- for (int i = 0; i < 7; i++) {
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- }
-
- assertStats(
- MemoryPressureStats.newBuilder()
- .setManuallyTriggeredGcs(3)
- .setMaxConsecutiveIgnoredGcsOverThreshold(8));
- }
-
- @Test
- public void threshold100_noGcTriggeredEvenWithNonsenseStats() {
- options.oomMoreEagerlyThreshold = 100;
- underTest.setOptions(options);
- WeakReference<?> ref = new WeakReference<>(new Object());
-
- underTest.handle(percentUsedAfterOrganicFullGc(101));
- assertThat(ref.get()).isNotNull();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void optionsNotSet_disabled() {
- underTest.handle(percentUsedAfterOrganicFullGc(99));
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void gcThrashingLimitsSet_mutuallyExclusive_disabled() {
- options.oomMoreEagerlyThreshold = 90;
- options.gcThrashingLimits = ImmutableList.of(Limit.of(Duration.ofMinutes(1), 2));
- options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive = true;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(99));
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void gcThrashingLimitsSet_mutuallyInclusive_enabled() {
- options.oomMoreEagerlyThreshold = 90;
- options.gcThrashingLimits = ImmutableList.of(Limit.of(Duration.ofMinutes(1), 2));
- options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive = false;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(99));
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void statsReset() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- private static MemoryPressureEvent percentUsedAfterManualGc(int percentUsed) {
- return percentUsedAfterGc(percentUsed).setWasManualGc(true).setWasFullGc(true).build();
- }
-
- private static MemoryPressureEvent percentUsedAfterOrganicFullGc(int percentUsed) {
- return percentUsedAfterGc(percentUsed).setWasFullGc(true).build();
- }
-
- private static MemoryPressureEvent percentUsedAfterGcLockerGc(int percentUsed) {
- return percentUsedAfterGc(percentUsed).setWasGcLockerInitiatedGc(true).build();
- }
-
- private static MemoryPressureEvent.Builder percentUsedAfterGc(int percentUsed) {
- checkArgument(percentUsed >= 0, percentUsed);
- return MemoryPressureEvent.newBuilder()
- .setTenuredSpaceUsedBytes(percentUsed)
- .setTenuredSpaceMaxBytes(100L);
- }
-
- private void assertStats(MemoryPressureStats.Builder expected) {
- MemoryPressureStats.Builder stats = MemoryPressureStats.newBuilder();
- underTest.addStatsAndReset(stats);
- assertThat(stats.build()).isEqualTo(expected.build());
- }
-}
