From 7242b0e2db061f81c3d0c3220baeaa8ec95db40b Mon Sep 17 00:00:00 2001 From: mschaller Date: Sun, 7 Mar 2021 16:13:40 -0800 Subject: [PATCH] Replace NullEvaluationProgressReceiver with default methods Removes unnecessary clutter. RELNOTES: None. PiperOrigin-RevId: 361458286 --- .../buildtool/ExecutionProgressReceiver.java | 8 +- .../build/lib/skyframe/SkyframeBuildView.java | 3 +- .../build/lib/skyframe/SkyframeExecutor.java | 5 +- .../TrimmedConfigurationProgressReceiver.java | 7 +- .../packages/AbstractPackageLoader.java | 2 +- .../skyframe/EvaluationProgressReceiver.java | 59 +++------- ...ursiveFilesystemTraversalFunctionTest.java | 2 +- .../lib/skyframe/SkyframeAwareActionTest.java | 2 +- .../build/skyframe/EagerInvalidatorTest.java | 107 +++++++++--------- .../build/skyframe/ParallelEvaluatorTest.java | 4 +- .../skyframe/TrackingProgressReceiver.java | 7 +- 11 files changed, 86 insertions(+), 120 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java index ef0dd8a93fd50f..e7935a729af38b 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionProgressReceiver.java @@ -39,13 +39,13 @@ import javax.annotation.Nullable; /** - * Listener for executed actions and built artifacts. We use a listener so that we have an - * accurate set of successfully run actions and built artifacts, even if the build is interrupted. + * Listener for executed actions and built artifacts. We use a listener so that we have an accurate + * set of successfully run actions and built artifacts, even if the build is interrupted. */ public final class ExecutionProgressReceiver - extends EvaluationProgressReceiver.NullEvaluationProgressReceiver implements SkyframeActionExecutor.ProgressSupplier, - SkyframeActionExecutor.ActionCompletedReceiver { + SkyframeActionExecutor.ActionCompletedReceiver, + EvaluationProgressReceiver { private static final ThreadLocal PROGRESS_MESSAGE_NUMBER_FORMATTER = ThreadLocal.withInitial( () -> { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index f914c51179338f..a44dbb8fc385cf 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -1058,8 +1058,7 @@ public ActionKeyContext getActionKeyContext() { return skyframeExecutor.getActionKeyContext(); } - private final class ActionLookupValueProgressReceiver - extends EvaluationProgressReceiver.NullEvaluationProgressReceiver { + private final class ActionLookupValueProgressReceiver implements EvaluationProgressReceiver { private final AtomicInteger actionLookupValueCount = new AtomicInteger(); private final AtomicInteger actionCount = new AtomicInteger(); private final AtomicInteger configuredTargetCount = new AtomicInteger(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index c32759954ab41e..175b1eaee54636 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -3005,9 +3005,8 @@ public PrepareAnalysisPhaseValue prepareAnalysisPhase( return prepareAnalysisPhaseValue; } - /** A progress received to track analysis invalidation and update progress messages. */ - protected class SkyframeProgressReceiver - extends EvaluationProgressReceiver.NullEvaluationProgressReceiver { + /** A progress receiver to track analysis invalidation and update progress messages. */ + protected class SkyframeProgressReceiver implements EvaluationProgressReceiver { /** * This flag is needed in order to avoid invalidating legacy data when we clear the analysis * cache because of --discard_analysis_cache flag. For that case we want to keep the legacy data diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java index 9e1eb0e4ba1f33..0480cf7864817a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java @@ -19,10 +19,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.skyframe.trimming.TrimmedConfigurationCache; import com.google.devtools.build.skyframe.ErrorInfo; -import com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationState; -import com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationSuccessState; -import com.google.devtools.build.skyframe.EvaluationProgressReceiver.InvalidationState; -import com.google.devtools.build.skyframe.EvaluationProgressReceiver.NullEvaluationProgressReceiver; +import com.google.devtools.build.skyframe.EvaluationProgressReceiver; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.function.Supplier; @@ -32,7 +29,7 @@ * Skyframe progress receiver which keeps a {@link TrimmedConfigurationCache} in sync with Skyframe * invalidations and revalidations. */ -public final class TrimmedConfigurationProgressReceiver extends NullEvaluationProgressReceiver { +public final class TrimmedConfigurationProgressReceiver implements EvaluationProgressReceiver { private final TrimmedConfigurationCache cache; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index 4576b100cba5a8..99b4d6624739d8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -420,7 +420,7 @@ private BuildDriver makeFreshDriver() { InMemoryMemoizingEvaluator.SUPPLIER.create( makeFreshSkyFunctions(), preinjectedDifferencer, - new EvaluationProgressReceiver.NullEvaluationProgressReceiver(), + EvaluationProgressReceiver.NULL, GraphInconsistencyReceiver.THROWING, InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER, new MemoizingEvaluator.EmittedEventState(), diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java index c845737c0cfdde..88842059cb1bec 100644 --- a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java +++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java @@ -20,9 +20,11 @@ /** Receiver for various stages of the lifetime of a skyframe node evaluation. */ @ThreadSafety.ThreadSafe public interface EvaluationProgressReceiver { - /** - * New state of the value entry after evaluation. - */ + + /** A no-op {@link EvaluationProgressReceiver}. */ + EvaluationProgressReceiver NULL = new EvaluationProgressReceiver() {}; + + /** New state of the value entry after evaluation. */ enum EvaluationState { /** The value was successfully re-evaluated. */ BUILT, @@ -50,9 +52,7 @@ public Supplier supplier() { } } - /** - * New state of the value entry after invalidation. - */ + /** New state of the value entry after invalidation. */ enum InvalidationState { /** The value is dirty, although it might get re-validated again. */ DIRTY, @@ -60,9 +60,7 @@ enum InvalidationState { DELETED, } - /** - * Overall state of the node while it is being evaluated. - */ + /** Overall state of the node while it is being evaluated. */ enum NodeState { /** The node is undergoing a dirtiness check and may be re-validated. */ CHECK_DIRTY, @@ -84,17 +82,17 @@ enum NodeState { *

If {@code state} is {@link InvalidationState#DIRTY}, should only be called after a * successful {@link ThinNodeEntry#markDirty} call: a call that returns a non-null value. */ - void invalidated(SkyKey skyKey, InvalidationState state); + default void invalidated(SkyKey skyKey, InvalidationState state) {} /** * Notifies that {@code skyKey} is about to get queued for evaluation. * - *

Note that we don't guarantee that it actually got enqueued or will, only that if - * everything "goes well" (e.g. no interrupts happen) it will. + *

Note that we don't guarantee that it actually got enqueued or will, only that if everything + * "goes well" (e.g. no interrupts happen) it will. * *

This guarantee is intentionally vague to encourage writing robust implementations. */ - void enqueueing(SkyKey skyKey); + default void enqueueing(SkyKey skyKey) {} /** * Notifies that the node for {@code skyKey} is about to enter the given {@code nodeState}. @@ -102,7 +100,7 @@ enum NodeState { *

Notably, this includes {@link SkyFunction#compute} calls due to Skyframe restarts, but also * dirtiness checking and node completion. */ - void stateStarting(SkyKey skyKey, NodeState nodeState); + default void stateStarting(SkyKey skyKey, NodeState nodeState) {} /** * Notifies that the node for {@code skyKey} is about to complete the given {@code nodeState}. @@ -112,7 +110,7 @@ enum NodeState { *

{@code elapsedTimeNanos} is either the elapsed time in the {@code nodeState} or -1 if the * timing was not recorded. */ - void stateEnding(SkyKey skyKey, NodeState nodeState, long elapsedTimeNanos); + default void stateEnding(SkyKey skyKey, NodeState nodeState, long elapsedTimeNanos) {} /** * Notifies that the node for {@code skyKey} has been evaluated, or found to not need @@ -128,37 +126,10 @@ enum NodeState { * value or error (i.e., {@code EvaluationState.BUILT} if and only if at least one of newValue * and newError is non-null) */ - void evaluated( + default void evaluated( SkyKey skyKey, @Nullable SkyValue newValue, @Nullable ErrorInfo newError, Supplier evaluationSuccessState, - EvaluationState state); - - /** An {@link EvaluationProgressReceiver} that does nothing. */ - class NullEvaluationProgressReceiver implements EvaluationProgressReceiver { - @Override - public void invalidated(SkyKey skyKey, InvalidationState state) { - } - - @Override - public void enqueueing(SkyKey skyKey) { - } - - @Override - public void stateStarting(SkyKey skyKey, NodeState nodeState) { - } - - @Override - public void stateEnding(SkyKey skyKey, NodeState nodeState, long elapsedTimeNanos) { - } - - @Override - public void evaluated( - SkyKey skyKey, - @Nullable SkyValue newValue, - @Nullable ErrorInfo newError, - Supplier evaluationSuccessState, - EvaluationState state) {} - } + EvaluationState state) {} } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index a820a617febb69..8d653baec00b96 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -415,7 +415,7 @@ private void invalidateOutputArtifact(Artifact output) { } private static final class RecordingEvaluationProgressReceiver - extends EvaluationProgressReceiver.NullEvaluationProgressReceiver { + implements EvaluationProgressReceiver { Set invalidations; Set evaluations; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java index d61f6608eb1638..ed3cf587b066c1 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java @@ -89,7 +89,7 @@ public final void createExecutor() throws Exception { } private static final class TrackingEvaluationProgressReceiver - extends EvaluationProgressReceiver.NullEvaluationProgressReceiver { + implements EvaluationProgressReceiver { public static final class InvalidatedKey { public final SkyKey skyKey; diff --git a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java index 762559a0f06de7..644f67b3569762 100644 --- a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java @@ -32,7 +32,6 @@ import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.DeletingNodeVisitor; import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.DirtyingInvalidationState; import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.DirtyingNodeVisitor; -import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.InvalidationState; import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.InvalidationType; import com.google.devtools.build.skyframe.QueryableGraph.Reason; import java.lang.ref.WeakReference; @@ -56,7 +55,7 @@ public class EagerInvalidatorTest { protected InMemoryGraphImpl graph; protected GraphTester tester = new GraphTester(); - protected InvalidationState state = newInvalidationState(); + protected InvalidatingNodeVisitor.InvalidationState state = newInvalidationState(); protected AtomicReference> visitor = new AtomicReference<>(); protected DirtyTrackingProgressReceiver progressReceiver; private IntVersion graphVersion = IntVersion.of(0); @@ -111,7 +110,7 @@ private void assertDirtyAndNotChanged(SkyKey key) { } - protected InvalidationState newInvalidationState() { + protected InvalidatingNodeVisitor.InvalidationState newInvalidationState() { throw new UnsupportedOperationException("Sublcasses must override"); } @@ -178,14 +177,16 @@ public void setUp() throws Exception { @Test public void receiverWorks() throws Exception { final Set invalidated = Sets.newConcurrentHashSet(); - DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { - @Override - public void invalidated(SkyKey skyKey, InvalidationState state) { - Preconditions.checkState(state == expectedState()); - invalidated.add(skyKey); - } - }); + DirtyTrackingProgressReceiver receiver = + new DirtyTrackingProgressReceiver( + new EvaluationProgressReceiver() { + @Override + public void invalidated( + SkyKey skyKey, EvaluationProgressReceiver.InvalidationState state) { + Preconditions.checkState(state == expectedState()); + invalidated.add(skyKey); + } + }); graph = new InMemoryGraphImpl(); SkyKey aKey = GraphTester.nonHermeticKey("a"); SkyKey bKey = GraphTester.nonHermeticKey("b"); @@ -206,14 +207,15 @@ public void invalidated(SkyKey skyKey, InvalidationState state) { @Test public void receiverIsNotifiedAboutNodesInError() throws Exception { final Set invalidated = Sets.newConcurrentHashSet(); - DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { - @Override - public void invalidated(SkyKey skyKey, InvalidationState state) { - Preconditions.checkState(state == expectedState()); - invalidated.add(skyKey); - } - }); + DirtyTrackingProgressReceiver receiver = + new DirtyTrackingProgressReceiver( + new EvaluationProgressReceiver() { + @Override + public void invalidated(SkyKey skyKey, InvalidationState state) { + Preconditions.checkState(state == expectedState()); + invalidated.add(skyKey); + } + }); // Given a graph consisting of two nodes, "a" and "ab" such that "ab" depends on "a", // And given "ab" is in error, @@ -236,14 +238,15 @@ public void invalidated(SkyKey skyKey, InvalidationState state) { @Test public void invalidateValuesNotInGraph() throws Exception { final Set invalidated = Sets.newConcurrentHashSet(); - DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { - @Override - public void invalidated(SkyKey skyKey, InvalidationState state) { - Preconditions.checkState(state == InvalidationState.DIRTY); - invalidated.add(skyKey); - } - }); + DirtyTrackingProgressReceiver receiver = + new DirtyTrackingProgressReceiver( + new EvaluationProgressReceiver() { + @Override + public void invalidated(SkyKey skyKey, InvalidationState state) { + Preconditions.checkState(state == InvalidationState.DIRTY); + invalidated.add(skyKey); + } + }); graph = new InMemoryGraphImpl(); SkyKey aKey = GraphTester.nonHermeticKey("a"); invalidateWithoutError(receiver, aKey); @@ -348,7 +351,7 @@ public void interruptChild() throws Exception { AtomicReference badKey = new AtomicReference<>(); DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { + new EvaluationProgressReceiver() { @Override public void invalidated(SkyKey skyKey, InvalidationState state) { if (skyKey.equals(child)) { @@ -382,7 +385,7 @@ public void invalidated(SkyKey skyKey, InvalidationState state) { assertThat(graph.get(null, Reason.OTHER, parent).getValue()).isNotNull(); final DirtyTrackingProgressReceiver receiver2 = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { + new EvaluationProgressReceiver() { @Override public void invalidated(SkyKey skyKey, InvalidationState state) { invalidated.add(skyKey); @@ -459,22 +462,22 @@ public void interruptThreadInReceiver() throws Exception { final CountDownLatch countDownToInterrupt = new CountDownLatch(countDownStart); final DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { - @Override - public void invalidated(SkyKey skyKey, InvalidationState state) { - countDownToInterrupt.countDown(); - if (countDownToInterrupt.getCount() == 0) { - mainThread.interrupt(); - // Wait for the main thread to be interrupted uninterruptibly, because the main - // thread is going to interrupt us, and we don't want to get into an interrupt - // fight. Only if we get interrupted without the main thread also being interrupted - // will this throw an InterruptedException. - TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions( - visitor.get().getInterruptionLatchForTestingOnly(), - "Main thread was not interrupted"); - } - } - }); + new EvaluationProgressReceiver() { + @Override + public void invalidated(SkyKey skyKey, InvalidationState state) { + countDownToInterrupt.countDown(); + if (countDownToInterrupt.getCount() == 0) { + mainThread.interrupt(); + // Wait for the main thread to be interrupted uninterruptibly, because the main + // thread is going to interrupt us, and we don't want to get into an interrupt + // fight. Only if we get interrupted without the main thread also being + // interrupted will this throw an InterruptedException. + TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions( + visitor.get().getInterruptionLatchForTestingOnly(), + "Main thread was not interrupted"); + } + } + }); try { invalidate( graph, @@ -537,7 +540,7 @@ boolean gcExpected() { } @Override - protected InvalidationState newInvalidationState() { + protected InvalidatingNodeVisitor.InvalidationState newInvalidationState() { return new InvalidatingNodeVisitor.DeletingInvalidationState(); } @@ -554,12 +557,12 @@ protected boolean reverseDepsPresent() { @Test public void dirtyTrackingProgressReceiverWorksWithDeletingInvalidator() throws Exception { setupInvalidatableGraph(); - DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver()); + DirtyTrackingProgressReceiver receiver = + new DirtyTrackingProgressReceiver(/*progressReceiver=*/ null); // Dirty the node, and ensure that the tracker is aware of it: ImmutableList diff = ImmutableList.of(GraphTester.nonHermeticKey("a")); - InvalidationState state1 = new DirtyingInvalidationState(); + InvalidatingNodeVisitor.InvalidationState state1 = new DirtyingInvalidationState(); Preconditions.checkNotNull( EagerInvalidator.createInvalidatingVisitorIfNeeded(graph, diff, receiver, state1)) .run(); @@ -601,7 +604,7 @@ boolean gcExpected() { } @Override - protected InvalidationState newInvalidationState() { + protected InvalidatingNodeVisitor.InvalidationState newInvalidationState() { return new DirtyingInvalidationState(); } @@ -618,8 +621,8 @@ protected boolean reverseDepsPresent() { @Test public void dirtyTrackingProgressReceiverWorksWithDirtyingInvalidator() throws Exception { setupInvalidatableGraph(); - DirtyTrackingProgressReceiver receiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver()); + DirtyTrackingProgressReceiver receiver = + new DirtyTrackingProgressReceiver(/*progressReceiver=*/ null); // Dirty the node, and ensure that the tracker is aware of it: invalidate(graph, receiver, GraphTester.nonHermeticKey("a")); diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java index 2b7dcc58c37a9b..fe2f50f7a6eb18 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java @@ -507,7 +507,7 @@ public String extractTag(SkyKey skyKey) { final Set receivedValues = Sets.newConcurrentHashSet(); revalidationReceiver = new DirtyTrackingProgressReceiver( - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { + new EvaluationProgressReceiver() { @Override public void evaluated( SkyKey skyKey, @@ -2176,7 +2176,7 @@ public void signalValueEnqueuedAndEvaluated() throws Exception { final Set enqueuedValues = Sets.newConcurrentHashSet(); final Set evaluatedValues = Sets.newConcurrentHashSet(); EvaluationProgressReceiver progressReceiver = - new EvaluationProgressReceiver.NullEvaluationProgressReceiver() { + new EvaluationProgressReceiver() { @Override public void enqueueing(SkyKey skyKey) { enqueuedValues.add(skyKey); diff --git a/src/test/java/com/google/devtools/build/skyframe/TrackingProgressReceiver.java b/src/test/java/com/google/devtools/build/skyframe/TrackingProgressReceiver.java index 0d401348508d3b..941e9648621d6b 100644 --- a/src/test/java/com/google/devtools/build/skyframe/TrackingProgressReceiver.java +++ b/src/test/java/com/google/devtools/build/skyframe/TrackingProgressReceiver.java @@ -20,11 +20,8 @@ import java.util.function.Supplier; import javax.annotation.Nullable; -/** - * A testing utility to keep track of evaluation. - */ -public class TrackingProgressReceiver - extends EvaluationProgressReceiver.NullEvaluationProgressReceiver { +/** A testing utility to keep track of evaluation. */ +public class TrackingProgressReceiver implements EvaluationProgressReceiver { private final boolean checkEvaluationResults; /** * Callback to be executed on a next {@link #invalidated} call. It will be run once and is