Post the first 5 action rewind events instead of the top 5.
A max of 5 action rewind events are posted as a sample, but we currently store every action rewind event in the whole build, then when the build ends, we sort them by number of lost inputs and take the top 5. In order to limit memory use when there is a lot of rewinding, just take the first 5 such events and do away with sorting by number of lost inputs. Note that we still post the total number of lost inputs in the build.
Removed assertions in a couple tests about exactly which events are posted since it can now be nondeterministic. I think these tests still offer excellent coverage.
PiperOrigin-RevId: 611566530
Change-Id: I190e6fc53d17945848fb2cfa7ae9bf6052450921
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java
index 4f09d36..0e6a990 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java
@@ -17,9 +17,8 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.collect.Comparators.greatest;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static java.util.Comparator.comparing;
+import static java.lang.Math.min;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
@@ -61,11 +60,12 @@
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.atomic.AtomicInteger;
import javax.annotation.Nullable;
/**
@@ -83,11 +83,11 @@
private final SkyframeActionExecutor skyframeActionExecutor;
private final BugReporter bugReporter;
- // Note that these references are mutated only outside of Skyframe evaluations, and accessed only
- // inside of them. Their visibility piggybacks on Skyframe evaluation synchronizations.
private ConcurrentHashMultiset<LostInputRecord> lostInputRecords =
ConcurrentHashMultiset.create();
- private ConcurrentLinkedQueue<RewindPlanStats> rewindPlansStats = new ConcurrentLinkedQueue<>();
+ private final List<RewindPlanStats> rewindPlansStats =
+ Collections.synchronizedList(new ArrayList<>(MAX_ACTION_REWIND_EVENTS));
+ private final AtomicInteger rewindPlanStatsCounter = new AtomicInteger(0);
public ActionRewindStrategy(
SkyframeActionExecutor skyframeActionExecutor, BugReporter bugReporter) {
@@ -156,14 +156,16 @@
prepareRewindPlan(
failedKey, failedActionDeps, lostInputsByDigest, inputDepOwners, env, depsToRewind);
- int lostInputRecordsCount = lostInputRecordsThisAction.size();
- rewindPlansStats.add(
- RewindPlanStats.create(
- failedAction,
- rewindPlan.rewindGraph().nodes().size(),
- lostInputRecordsCount,
- lostInputRecordsThisAction.subList(
- 0, Math.min(lostInputRecordsCount, MAX_LOST_INPUTS_RECORDED))));
+ if (rewindPlanStatsCounter.getAndIncrement() < MAX_ACTION_REWIND_EVENTS) {
+ int lostInputRecordsCount = lostInputRecordsThisAction.size();
+ rewindPlansStats.add(
+ RewindPlanStats.create(
+ failedAction,
+ rewindPlan.rewindGraph().nodes().size(),
+ lostInputRecordsCount,
+ lostInputRecordsThisAction.subList(
+ 0, min(lostInputRecordsCount, MAX_LOST_INPUTS_RECORDED))));
+ }
if (lostInputsException.isActionStartedEventAlreadyEmitted()) {
env.getListener()
@@ -277,23 +279,20 @@
}
/**
- * Log the top N action rewind events and clear the history of failed actions' lost inputs and
+ * Logs the first N action rewind events and clears the history of failed actions' lost inputs and
* rewind plans.
*/
public void reset(ExtendedEventHandler eventHandler) {
ImmutableList<ActionRewindEvent> topActionRewindEvents =
rewindPlansStats.stream()
- .collect(
- greatest(
- MAX_ACTION_REWIND_EVENTS, comparing(RewindPlanStats::invalidatedNodesCount)))
- .stream()
.map(ActionRewindingStats::toActionRewindEventProto)
.collect(toImmutableList());
ActionRewindingStats rewindingStats =
new ActionRewindingStats(lostInputRecords.size(), topActionRewindEvents);
eventHandler.post(rewindingStats);
lostInputRecords = ConcurrentHashMultiset.create();
- rewindPlansStats = new ConcurrentLinkedQueue<>();
+ rewindPlansStats.clear();
+ rewindPlanStatsCounter.set(0);
}
private void checkIfTopLevelOutputLostTooManyTimes(
@@ -707,12 +706,7 @@
}
}
- /**
- * Lite statistics about graph computed by {@link #prepareRewindPlan}.
- *
- * <p>This object persists across the build. It is more memory efficient than saving the entire
- * rewind graph for each rewind plan.
- */
+ /** Lite statistics about graph computed by {@link #prepareRewindPlan}. */
@AutoValue
abstract static class RewindPlanStats {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java
index 9b22ef5..b7d1146 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java
@@ -70,7 +70,6 @@
import com.google.devtools.build.lib.skyframe.ActionExecutionValue;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.testutil.ActionEventRecorder;
-import com.google.devtools.build.lib.testutil.ActionEventRecorder.ActionRewindEventAsserter;
import com.google.devtools.build.lib.testutil.ControllableActionStrategyModule;
import com.google.devtools.build.lib.testutil.SpawnController;
import com.google.devtools.build.lib.testutil.SpawnController.ExecResult;
@@ -598,16 +597,7 @@
/* exactlyOneMiddlemanEventChecks= */ ImmutableList.of(),
/* expectResultReceivedForFailedRewound= */ false,
/* actionRewindingPostLostInputCounts= */ ImmutableList.of(
- ActionRewindStrategy.MAX_REPEATED_LOST_INPUTS + 1),
- /* lostInputAndActionsPosts= */ ImmutableList.of(
- ImmutableList.copyOf(
- Collections.nCopies(
- // 3 invalidated nodes:
- // 1. The failed action itself.
- // 2. The ArtifactNestedSet node for [srcs] in
- // action.getInputs() = {genrule-setup.sh, [srcs]}.
- // 3. The lost input's generating action.
- 5, ActionRewindEventAsserter.create("Genrule", "//test:rule2", 1, 3)))));
+ ActionRewindStrategy.MAX_REPEATED_LOST_INPUTS + 1));
assertOnlyActionsRewound(rewoundKeys);
assertThat(Iterables.frequency(rewoundArtifactOwnerLabels(rewoundKeys), "//test:rule1"))
@@ -701,18 +691,7 @@
"//test:consume_6");
assertOnlyActionsRewound(rewoundKeys);
verifyAllSpawnShimsConsumed();
- recorder.assertRewindActionStats(
- /* totalLostInputCounts= */ ImmutableList.of(21),
- /* lostInputAndActionsPosts= */ ImmutableList.of(
- ImmutableList.of(
- // All invalidated node counts are len(srcs) + 2 (+1 for the failed action itself,
- // and +1 for the ArtifactNestedSet node for [srcs] in
- // action.getInputs() = {genrule-setup.sh, [srcs]}.
- ActionRewindEventAsserter.create("Genrule", "//test:consume_6", 6, 8),
- ActionRewindEventAsserter.create("Genrule", "//test:consume_5", 5, 7),
- ActionRewindEventAsserter.create("Genrule", "//test:consume_4", 4, 6),
- ActionRewindEventAsserter.create("Genrule", "//test:consume_3", 3, 5),
- ActionRewindEventAsserter.create("Genrule", "//test:consume_2", 2, 4))));
+ recorder.assertTotalLostInputCountsFromStats(ImmutableList.of(21));
}
public final void runInterruptedDuringRewindStopsNormally() throws Exception {
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/ActionEventRecorder.java b/src/test/java/com/google/devtools/build/lib/testutil/ActionEventRecorder.java
index c449e3e..d0c4440 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/ActionEventRecorder.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/ActionEventRecorder.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
-import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.AllowConcurrentEvents;
import com.google.common.eventbus.Subscribe;
@@ -28,7 +27,6 @@
import com.google.devtools.build.lib.actions.ActionResultReceivedEvent;
import com.google.devtools.build.lib.actions.ActionStartedEvent;
import com.google.devtools.build.lib.actions.CachedActionEvent;
-import com.google.devtools.build.lib.skyframe.proto.ActionRewind.ActionRewindEvent;
import com.google.devtools.build.lib.skyframe.rewinding.ActionRewindingStats;
import com.google.devtools.build.lib.skyframe.rewinding.ActionRewoundEvent;
import java.util.ArrayList;
@@ -177,9 +175,8 @@
completedRewound,
failedRewound,
exactlyOneMiddlemanEventChecks,
- /*expectResultReceivedForFailedRewound=*/ true,
- actionRewindingPostLostInputCounts,
- /*lostInputAndActionsPosts=*/ ImmutableList.of());
+ /* expectResultReceivedForFailedRewound= */ true,
+ actionRewindingPostLostInputCounts);
}
/**
@@ -197,18 +194,17 @@
ImmutableList<String> failedRewound,
ImmutableList<Predicate<ActionMiddlemanEvent>> exactlyOneMiddlemanEventChecks,
boolean expectResultReceivedForFailedRewound,
- ImmutableList<Integer> actionRewindingPostLostInputCounts,
- ImmutableList<ImmutableList<ActionRewindEventAsserter>> lostInputAndActionsPosts) {
+ ImmutableList<Integer> actionRewindingPostLostInputCounts) {
EventCountAsserter eventCountAsserter =
new EventCountAsserter(runOnce, completedRewound, failedRewound);
eventCountAsserter.assertEventCounts(
- /*events=*/ actionStartedEvents,
- /*eventsName=*/ "actionStartedEvents",
- /*converter=*/ e -> progressMessageOrPrettyPrint(e.getAction()),
- /*expectedRunOnceEventCount=*/ 1,
- /*expectedCompletedRewoundEventCount=*/ 1,
- /*expectedFailedRewoundEventCount=*/ 2);
+ /* events= */ actionStartedEvents,
+ /* eventsName= */ "actionStartedEvents",
+ /* converter= */ e -> progressMessageOrPrettyPrint(e.getAction()),
+ /* expectedRunOnceEventCount= */ 1,
+ /* expectedCompletedRewoundEventCount= */ 1,
+ /* expectedFailedRewoundEventCount= */ 2);
eventCountAsserter.assertEventCounts(
/*events=*/ actionCompletionEvents,
@@ -242,7 +238,7 @@
/*expectedCompletedRewoundEventCount=*/ 0,
/*expectedFailedRewoundEventCount=*/ 1);
- assertRewindActionStats(actionRewindingPostLostInputCounts, lostInputAndActionsPosts);
+ assertTotalLostInputCountsFromStats(actionRewindingPostLostInputCounts);
for (Predicate<ActionMiddlemanEvent> check : exactlyOneMiddlemanEventChecks) {
assertThat(actionMiddlemanEvents.stream().filter(check)).hasSize(1);
@@ -251,67 +247,21 @@
}
/**
- * Assert that the contents of the {@link ActionRewindingStats} event matches expected results.
+ * Asserts that the total lost input counts from posted {@link ActionRewindingStats} matches
+ * expected results.
*
* @param totalLostInputCounts - The list of the counts of all lost inputs logged with each
* ActionRewindingStats post.
- * @param lostInputAndActionsPosts - The list of all expected values in all ActionRewindingStats
- * posts. The outer list represents the ActionRewindingStats posts, the inner list contains
- * {@link ActionRewindEventAsserter} that has the expected values of each action rewind event
- * within each corresponding post.
*/
- public void assertRewindActionStats(
- ImmutableList<Integer> totalLostInputCounts,
- ImmutableList<ImmutableList<ActionRewindEventAsserter>> lostInputAndActionsPosts) {
+ public void assertTotalLostInputCountsFromStats(ImmutableList<Integer> totalLostInputCounts) {
assertThat(actionRewindingStatsPosts).hasSize(totalLostInputCounts.size());
- for (int postIndex = 0; postIndex < lostInputAndActionsPosts.size(); postIndex++) {
+ for (int postIndex = 0; postIndex < totalLostInputCounts.size(); postIndex++) {
ActionRewindingStats actionRewindingStats = actionRewindingStatsPosts.get(postIndex);
assertThat(actionRewindingStats.lostInputsCount())
.isEqualTo(totalLostInputCounts.get(postIndex));
- ImmutableList<ActionRewindEventAsserter> expectedLostInputPost =
- lostInputAndActionsPosts.get(postIndex);
-
- for (int r = 0; r < actionRewindingStats.actionRewindEvents().size(); r++) {
- ActionRewindEvent actualActionRewind = actionRewindingStats.actionRewindEvents().get(r);
- expectedLostInputPost.get(r).assertActionRewindEvent(actualActionRewind);
- }
}
}
- @AutoValue
- public abstract static class ActionRewindEventAsserter {
-
- public static ActionRewindEventAsserter create(
- String expectedActionMnemonic,
- String expectedActionOwnerLabel,
- int expectedLostInputsCount,
- int expectedInvalidatedNodesCount) {
- return new AutoValue_ActionEventRecorder_ActionRewindEventAsserter(
- expectedActionMnemonic,
- expectedActionOwnerLabel,
- expectedLostInputsCount,
- expectedInvalidatedNodesCount);
- }
-
- ActionRewindEventAsserter() {}
-
- private void assertActionRewindEvent(ActionRewindEvent actualEvent) {
- assertThat(actualEvent.getActionDescription().getType()).isEqualTo(expectedActionMnemonic());
- assertThat(actualEvent.getActionDescription().getRuleLabel())
- .isEqualTo(expectedActionOwnerLabel());
- assertThat(actualEvent.getTotalLostInputsCount()).isEqualTo(expectedLostInputsCount());
- assertThat(actualEvent.getInvalidatedNodesCount()).isEqualTo(expectedInvalidatedNodesCount());
- }
-
- abstract String expectedActionMnemonic();
-
- abstract String expectedActionOwnerLabel();
-
- abstract int expectedLostInputsCount();
-
- abstract int expectedInvalidatedNodesCount();
- }
-
private static final class EventCountAsserter {
private final ImmutableList<String> runOnce;
private final ImmutableList<String> completedRewound;
