From 29a7e0579aa7ff3633e92c3c9de06c878b1d14c7 Mon Sep 17 00:00:00 2001 From: mschaller Date: Mon, 18 Jun 2018 09:15:42 -0700 Subject: [PATCH] Permit marking dirty/changed a node more than once This functionality will be useful for node restarting. More than one parent node may request the restart of a shared child node, and this should not fail. Instead, the returned MarkedDirtyResult indicates whether the dirtying was redundant, and the calling code can assert on that. RELNOTES: None. PiperOrigin-RevId: 201005663 --- .../build/skyframe/InMemoryNodeEntry.java | 22 ++- .../skyframe/InvalidatingNodeVisitor.java | 18 ++- .../build/skyframe/ThinNodeEntry.java | 113 +++++++++++---- .../devtools/build/skyframe/GraphTest.java | 4 +- .../build/skyframe/InMemoryNodeEntryTest.java | 130 +++++++++++++----- 5 files changed, 204 insertions(+), 83 deletions(-) diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java index 00f52dd73db0be..bb6a2d1d4bd16a 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java @@ -475,21 +475,19 @@ public synchronized MarkedDirtyResult markDirty(boolean isChanged) { DirtyBuildingState.create(isChanged, GroupedList.create(directDeps), value); value = null; directDeps = null; - return new MarkedDirtyResult(ReverseDepsUtility.getReverseDeps(this)); + return new FromCleanMarkedDirtyResult(ReverseDepsUtility.getReverseDeps(this)); } - // The caller may be simultaneously trying to mark this node dirty and changed, and the dirty - // thread may have lost the race, but it is the caller's responsibility not to try to mark - // this node changed twice. The end result of racing markers must be a changed node, since one - // of the markers is trying to mark the node changed. - Preconditions.checkState(isChanged != isChanged(), - "Cannot mark node dirty twice or changed twice: %s", this); + Preconditions.checkState(value == null, "Value should have been reset already %s", this); - if (isChanged) { - // If the changed marker lost the race, we just need to mark changed in this method -- all - // other work was done by the dirty marker. - getDirtyBuildingState().markChanged(); + if (isChanged != isChanged()) { + if (isChanged) { + getDirtyBuildingState().markChanged(); + } + // If !isChanged, then this call made no changes to the node, but redundancy is a property of + // the sequence of markDirty calls, not their effects. + return FromDirtyMarkedDirtyResult.NOT_REDUNDANT; } - return null; + return FromDirtyMarkedDirtyResult.REDUNDANT; } @Override diff --git a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java index ceae771e88401c..9d2d81159461f5 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java +++ b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java @@ -207,7 +207,7 @@ Set> getInvalidationsForTesting() { } } - public static class DirtyingInvalidationState extends InvalidationState { + static class DirtyingInvalidationState extends InvalidationState { public DirtyingInvalidationState() { super(InvalidationType.CHANGED); } @@ -477,7 +477,7 @@ public void run() { // method. // Any exception thrown should be unrecoverable. // This entry remains in the graph in this dirty state until it is re-evaluated. - MarkedDirtyResult markedDirtyResult = null; + MarkedDirtyResult markedDirtyResult; try { markedDirtyResult = entry.markDirty(isChanged); } catch (InterruptedException e) { @@ -487,8 +487,13 @@ public void run() { // visitation, so we can resume next time. return; } - if (markedDirtyResult == null) { - // Another thread has already dirtied this node. Don't do anything in this thread. + Preconditions.checkState( + !markedDirtyResult.wasCallRedundant(), + "Node unexpectedly marked dirty or changed twice: %s", + entry); + if (!markedDirtyResult.wasClean()) { + // Another thread has already handled this node's rdeps. Don't do anything in this + // thread. if (supportInterruptions) { pendingVisitations.remove(Pair.of(key, invalidationType)); } @@ -496,7 +501,10 @@ public void run() { } // Propagate dirtiness upwards and mark this node dirty/changed. Reverse deps should // only be marked dirty (because only a dependency of theirs has changed). - visit(markedDirtyResult.getReverseDepsUnsafe(), InvalidationType.DIRTIED, key); + visit( + markedDirtyResult.getReverseDepsUnsafeIfWasClean(), + InvalidationType.DIRTIED, + key); progressReceiver.invalidated(key, EvaluationProgressReceiver.InvalidationState.DIRTY); diff --git a/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java index eef54874362fde..c063ce6f67c44b 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java @@ -15,11 +15,10 @@ import com.google.common.base.Preconditions; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; -import javax.annotation.Nullable; /** * A node in the graph without the means to access its value. All operations on this class are - * thread-safe. Note, however, the warning on the return value of {@link #markDirty}. + * thread-safe (note, however, the warning on the return value of {@link #markDirty}). * *

This interface is public only for the benefit of alternative graph implementations outside of * the package. @@ -45,45 +44,101 @@ public interface ThinNodeEntry { boolean isChanged(); /** - * Marks this node dirty, or changed if {@code isChanged} is true. The node is put in the - * just-created state. It will be re-evaluated if necessary during the evaluation phase, but if it - * has not changed, it will not force a re-evaluation of its parents. + * Marks this node dirty, or changed if {@code isChanged} is true. * - *

{@code markDirty(b)} must not be called on an undone node if {@code isChanged() == b}. It is - * the caller's responsibility to ensure that this does not happen. Calling {@code - * markDirty(false)} when {@code isChanged() == true} has no effect. The idea here is that the - * caller will only ever want to call {@code markDirty()} a second time if a transition from a - * dirty-unchanged state to a dirty-changed state is required. + *

A dirty node P is re-evaluated during the evaluation phase if it's requested and directly + * depends on some node C whose value changed since the last evaluation of P. If it's requested + * and there is no such node C, P is marked clean. * - * @return A {@link ThinNodeEntry.MarkedDirtyResult} if the node was previously clean, and {@code - * null} if it was already dirty. If it was already dirty, the caller should abort its - * handling of this node, since another thread is already dealing with it. Note the warning on - * {@link ThinNodeEntry.MarkedDirtyResult} regarding the collection it provides. + *

A changed node is re-evaluated during the evaluation phase if it's requested (regardless of + * the state of its dependencies). + * + * @return a {@link MarkedDirtyResult} indicating whether the call was redundant and which may + * include the node's reverse deps */ - @Nullable @ThreadSafe MarkedDirtyResult markDirty(boolean isChanged) throws InterruptedException; - /** - * Returned by {@link #markDirty} if that call changed the node from clean to dirty. Contains an - * iterable of the node's reverse deps for efficiency, because the single use case for {@link - * #markDirty} is during invalidation, and if such an invalidation call wins, the invalidator - * must immediately afterwards schedule the invalidation of the node's reverse deps. - * - *

Warning: {@link #getReverseDepsUnsafe()} may return a live view of the reverse deps - * collection of the marked-dirty node. The consumer of this data must be careful only to - * iterate over and consume its values while that collection is guaranteed not to change. This - * is true during invalidation, because reverse deps don't change during invalidation. - */ - class MarkedDirtyResult { + /** Returned by {@link #markDirty}. */ + interface MarkedDirtyResult { + + /** Returns true iff the node was clean prior to the {@link #markDirty} call. */ + boolean wasClean(); + + /** + * Returns true iff the call to {@link #markDirty} was the same as some previous call to {@link + * #markDirty} (i.e., sharing the same {@code isChanged} parameter value) since the last time + * the node was clean. + * + *

More specifically, this returns true iff the call was {@code n.markDirty(b)} and prior to + * the call {@code n.isDirty() && n.isChanged() == b}). + */ + boolean wasCallRedundant(); + + /** + * If {@code wasClean()}, this returns an iterable of the node's reverse deps for efficiency, + * because the {@link #markDirty} caller may be doing graph invalidation, and after dirtying a + * node, the invalidation process may want to dirty the node's reverse deps. + * + *

If {@code !wasClean()}, this must not be called. It will throw {@link + * IllegalStateException}. + * + *

Warning: the returned iterable may be a live view of the reverse deps collection of the + * marked-dirty node. The consumer of this data must be careful only to iterate over and consume + * its values while that collection is guaranteed not to change. This is true during + * invalidation, because reverse deps don't change during invalidation. + */ + Iterable getReverseDepsUnsafeIfWasClean(); + } + + /** A {@link MarkedDirtyResult} returned when {@link #markDirty} is called on a clean node. */ + class FromCleanMarkedDirtyResult implements MarkedDirtyResult { private final Iterable reverseDepsUnsafe; - public MarkedDirtyResult(Iterable reverseDepsUnsafe) { + public FromCleanMarkedDirtyResult(Iterable reverseDepsUnsafe) { this.reverseDepsUnsafe = Preconditions.checkNotNull(reverseDepsUnsafe); } - public Iterable getReverseDepsUnsafe() { + @Override + public boolean wasClean() { + return true; + } + + @Override + public boolean wasCallRedundant() { + return false; + } + + @Override + public Iterable getReverseDepsUnsafeIfWasClean() { return reverseDepsUnsafe; } } + + /** A {@link MarkedDirtyResult} returned when {@link #markDirty} is called on a dirty node. */ + class FromDirtyMarkedDirtyResult implements MarkedDirtyResult { + static final FromDirtyMarkedDirtyResult REDUNDANT = new FromDirtyMarkedDirtyResult(true); + static final FromDirtyMarkedDirtyResult NOT_REDUNDANT = new FromDirtyMarkedDirtyResult(false); + + private final boolean redundant; + + private FromDirtyMarkedDirtyResult(boolean redundant) { + this.redundant = redundant; + } + + @Override + public boolean wasClean() { + return false; + } + + @Override + public boolean wasCallRedundant() { + return redundant; + } + + @Override + public Iterable getReverseDepsUnsafeIfWasClean() { + throw new IllegalStateException(); + } + } } diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTest.java b/src/test/java/com/google/devtools/build/skyframe/GraphTest.java index 378f58c81e3595..1f843a782a3248 100644 --- a/src/test/java/com/google/devtools/build/skyframe/GraphTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/GraphTest.java @@ -222,7 +222,7 @@ public void run() { graph = getGraph(getNextVersion(startingVersion)); NodeEntry sameEntry = Preconditions.checkNotNull(graph.get(null, Reason.OTHER, key)); // Mark the node as dirty again and check that the reverse deps have been preserved. - sameEntry.markDirty(true); + assertThat(sameEntry.markDirty(true).wasCallRedundant()).isFalse(); startEvaluation(sameEntry); sameEntry.markRebuilding(); sameEntry.setValue(new StringValue("foo2"), getNextVersion(startingVersion)); @@ -368,7 +368,7 @@ public void run() { throw new IllegalStateException(e); } try { - entry.markDirty(true); + assertThat(entry.markDirty(true).wasCallRedundant()).isFalse(); // Make some changes, like adding a dep and rdep. entry.addReverseDepAndCheckIfDone(key("rdep")); diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java index b16206cd6b6c85..4712cfd10e86bf 100644 --- a/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.skyframe.NodeEntry.DependencyState; import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; +import com.google.devtools.build.skyframe.ThinNodeEntry.MarkedDirtyResult; import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -183,7 +184,9 @@ public void dirtyLifecycle() throws InterruptedException { setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isFalse(); assertThat(entry.isDone()).isFalse(); @@ -215,7 +218,9 @@ public void changedLifecycle() throws InterruptedException { setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/true); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/true); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isTrue(); assertThat(entry.isDone()).isFalse(); @@ -243,11 +248,15 @@ public void markDirtyThenChanged() throws InterruptedException { setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult firstResult = entry.markDirty(/*isChanged=*/false); + assertThat(firstResult.wasCallRedundant()).isFalse(); + assertThat(firstResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isFalse(); assertThat(entry.isDone()).isFalse(); - entry.markDirty(/*isChanged=*/true); + MarkedDirtyResult secondResult = entry.markDirty(/*isChanged=*/true); + assertThat(secondResult.wasCallRedundant()).isFalse(); + assertThat(secondResult.wasClean()).isFalse(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isTrue(); assertThat(entry.isDone()).isFalse(); @@ -267,11 +276,15 @@ public void markChangedThenDirty() throws InterruptedException { setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/true); + MarkedDirtyResult firstResult = entry.markDirty(/*isChanged=*/true); + assertThat(firstResult.wasCallRedundant()).isFalse(); + assertThat(firstResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isTrue(); assertThat(entry.isDone()).isFalse(); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult secondResult = entry.markDirty(/*isChanged=*/false); + assertThat(secondResult.wasCallRedundant()).isFalse(); + assertThat(secondResult.wasClean()).isFalse(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isTrue(); assertThat(entry.isDone()).isFalse(); @@ -282,35 +295,60 @@ public void markChangedThenDirty() throws InterruptedException { } @Test - public void crashOnTwiceMarkedChanged() throws InterruptedException { + public void markChangedTwice() throws InterruptedException { NodeEntry entry = new InMemoryNodeEntry(); entry.addReverseDepAndCheckIfDone(null); // Start evaluation. - setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); + + setValue(entry, new SkyValue() {}, /*errorInfo=*/ null, /*graphVersion=*/ 0L); assertThat(entry.isDirty()).isFalse(); + assertThat(entry.isChanged()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/true); - try { - entry.markDirty(/*isChanged=*/true); - fail("Cannot mark entry changed twice"); - } catch (IllegalStateException e) { - // Expected. - } + + MarkedDirtyResult firstResult = entry.markDirty(/*isChanged=*/ true); + assertThat(firstResult.getReverseDepsUnsafeIfWasClean()).isNotNull(); + assertThat(firstResult.wasCallRedundant()).isFalse(); + assertThat(firstResult.wasClean()).isTrue(); + assertThat(entry.isDirty()).isTrue(); + assertThat(entry.isChanged()).isTrue(); + assertThat(entry.isDone()).isFalse(); + + MarkedDirtyResult secondResult = entry.markDirty(/*isChanged=*/ true); + assertThat(secondResult.wasClean()).isFalse(); + assertThat(secondResult.wasCallRedundant()).isTrue(); + assertThat(entry.isDirty()).isTrue(); + assertThat(entry.isChanged()).isTrue(); + assertThat(entry.isDone()).isFalse(); } @Test - public void crashOnTwiceMarkedDirty() throws InterruptedException { + public void markDirtyTwice() throws InterruptedException { NodeEntry entry = new InMemoryNodeEntry(); entry.addReverseDepAndCheckIfDone(null); // Start evaluation. + + // To successfully mark a node dirty (but not changed), that node must have at least one + // dependency. The node sanity-checks its deps while being marked dirty. addTemporaryDirectDep(entry, key("dep")); entry.signalDep(); - setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); - entry.markDirty(/*isChanged=*/false); - try { - entry.markDirty(/*isChanged=*/false); - fail("Cannot mark entry dirty twice"); - } catch (IllegalStateException e) { - // Expected. - } + + setValue(entry, new SkyValue() {}, /*errorInfo=*/ null, /*graphVersion=*/ 0L); + assertThat(entry.isDirty()).isFalse(); + assertThat(entry.isChanged()).isFalse(); + assertThat(entry.isDone()).isTrue(); + + MarkedDirtyResult firstResult = entry.markDirty(/*isChanged=*/ false); + assertThat(firstResult.getReverseDepsUnsafeIfWasClean()).isNotNull(); + assertThat(firstResult.wasCallRedundant()).isFalse(); + assertThat(firstResult.wasClean()).isTrue(); + assertThat(entry.isDirty()).isTrue(); + assertThat(entry.isChanged()).isFalse(); + assertThat(entry.isDone()).isFalse(); + + MarkedDirtyResult secondResult = entry.markDirty(/*isChanged=*/ false); + assertThat(secondResult.wasClean()).isFalse(); + assertThat(secondResult.wasCallRedundant()).isTrue(); + assertThat(entry.isDirty()).isTrue(); + assertThat(entry.isChanged()).isFalse(); + assertThat(entry.isDone()).isFalse(); } @Test @@ -373,7 +411,9 @@ public void pruneBeforeBuild() throws InterruptedException { setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isFalse(); assertThat(entry.isDone()).isFalse(); @@ -419,7 +459,9 @@ public void pruneAfterBuild() throws InterruptedException { addTemporaryDirectDep(entry, dep); entry.signalDep(); setValue(entry, new IntegerValue(5), /*errorInfo=*/null, /*graphVersion=*/0L); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); entry.addReverseDepAndCheckIfDone(null); // Start evaluation. assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.CHECK_DEPENDENCIES); entry.addReverseDepAndCheckIfDone(null); // Start evaluation. @@ -444,7 +486,9 @@ public void noPruneWhenDetailsChange() throws InterruptedException { setValue(entry, new IntegerValue(5), /*errorInfo=*/null, /*graphVersion=*/0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isFalse(); assertThat(entry.isDone()).isFalse(); @@ -487,7 +531,9 @@ public void pruneWhenDepGroupReordered() throws InterruptedException { setValue(entry, new IntegerValue(5), /*errorInfo=*/ null, /*graphVersion=*/ 0L); assertThat(entry.isDirty()).isFalse(); assertThat(entry.isDone()).isTrue(); - entry.markDirty(/*isChanged=*/ false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/ false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); assertThat(entry.isDirty()).isTrue(); assertThat(entry.isChanged()).isFalse(); assertThat(entry.isDone()).isFalse(); @@ -525,7 +571,9 @@ public void errorInfoCannotBePruned() throws InterruptedException { key("cause")); ErrorInfo errorInfo = ErrorInfo.fromException(exception, false); setValue(entry, /*value=*/null, errorInfo, /*graphVersion=*/0L); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); entry.addReverseDepAndCheckIfDone(null); // Restart evaluation. assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.CHECK_DEPENDENCIES); assertThat(entry.getNextDirtyDirectDeps()).containsExactly(dep); @@ -553,7 +601,9 @@ public void getDependencyGroup() throws InterruptedException { entry.signalDep(); entry.signalDep(); setValue(entry, /*value=*/new IntegerValue(5), null, 0L); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); entry.addReverseDepAndCheckIfDone(null); // Restart evaluation. assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.CHECK_DEPENDENCIES); assertThat(entry.getNextDirtyDirectDeps()).containsExactly(dep, dep2); @@ -584,7 +634,9 @@ public void maintainDependencyGroupAfterRemoval() throws InterruptedException { new GenericFunctionException(new SomeErrorException("oops"), Transience.PERSISTENT), key("key")); setValue(entry, null, ErrorInfo.fromException(exception, false), 0L); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); entry.addReverseDepAndCheckIfDone(null); // Restart evaluation. assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.CHECK_DEPENDENCIES); assertThat(entry.getNextDirtyDirectDeps()).containsExactly(dep); @@ -602,7 +654,9 @@ public void pruneWhenDepsChange() throws InterruptedException { addTemporaryDirectDep(entry, dep); entry.signalDep(); setValue(entry, new IntegerValue(5), /*errorInfo=*/null, /*graphVersion=*/0L); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); entry.addReverseDepAndCheckIfDone(null); // Start evaluation. assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.CHECK_DEPENDENCIES); assertThat(entry.getNextDirtyDirectDeps()).containsExactly(dep); @@ -630,7 +684,9 @@ public void checkDepsOneByOne() throws InterruptedException { entry.signalDep(); } setValue(entry, new IntegerValue(5), /*errorInfo=*/null, /*graphVersion=*/0L); - entry.markDirty(/*isChanged=*/false); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/false); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); entry.addReverseDepAndCheckIfDone(null); // Start new evaluation. assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.CHECK_DEPENDENCIES); for (int ii = 0; ii < 10; ii++) { @@ -650,7 +706,9 @@ public void signalOnlyNewParents() throws InterruptedException { NodeEntry entry = new InMemoryNodeEntry(); entry.addReverseDepAndCheckIfDone(key("parent")); setValue(entry, new SkyValue() {}, /*errorInfo=*/null, /*graphVersion=*/0L); - entry.markDirty(/*isChanged=*/true); + MarkedDirtyResult markedDirtyResult = entry.markDirty(/*isChanged=*/true); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); SkyKey newParent = key("new parent"); entry.addReverseDepAndCheckIfDone(newParent); assertThat(entry.getDirtyState()).isEqualTo(NodeEntry.DirtyState.NEEDS_REBUILDING); @@ -678,7 +736,9 @@ public void testClone() throws InterruptedException { entry.removeReverseDep(key("parent1")); entry.removeReverseDep(key("parent2")); IntegerValue updatedValue = new IntegerValue(52); - clone2.markDirty(true); + MarkedDirtyResult markedDirtyResult = clone2.markDirty(true); + assertThat(markedDirtyResult.wasCallRedundant()).isFalse(); + assertThat(markedDirtyResult.wasClean()).isTrue(); clone2.addReverseDepAndCheckIfDone(null); SkyKey newChild = key("newchild"); addTemporaryDirectDep(clone2, newChild);