Convert evaluated tracking to take a lazy SkyValue

The EvaluationProgressReceiver#evaluated method took a SkyValue,
but that parameter was only used by some of its implementations,
and only under some conditions.

Outside of tests, the main users are SkyframeBuildView's
ConfiguredTargetValueInvalidationReceiver and SkyframeBuilder's
ExecutionProgressReceiver.

The former builds up a set of built ConfiguredTarget keys when the
SkyValue is non-null and the EvaluationState is BUILT, and so its
nullity check can live behind those two conditions.

The latter cares about builting up a set of ConfiguredTargets, and
raising events on the eventBus when a TARGET_COMPLETION or
ASPECT_COMPLETION value is evaluated and is non-null. The extraction
of these values can live behind the conditions that check the type of
the SkyKey.

By making the SkyValue parameter lazy, this change enforces that it's
only accessed under these conditions.

This CL introduces a semantic change that should be small in effect.
The SkyframeBuildView keeps track of a set, dirtiedConfiguredTargetKeys,
and ConfiguredTarget keys evaluated as CLEAN were removed from it if
they had a non-null value. With this change, ConfiguredTarget keys
evaluated as CLEAN get removed regardless of whether their values are
null or non-null. The set is used to determine whether artifact conflict
search has to be rerun, and the extra removals that result from this CL
could cause fewer artifact conflict searches to run, but the only
affected searches would be those that were caused by dirtied configured
target values in error all of which were subsequently marked as clean,
which is probably rare.

--
MOS_MIGRATED_REVID=101144655

8 files changed, 72 insertions, 35 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
index bd7202638d..0ab5e8059c 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.buildtool;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -328,16 +329,21 @@ public class SkyframeBuilder implements Builder {
     }
 
     @Override
-    public void evaluated(SkyKey skyKey, SkyValue node, EvaluationState state) {
+    public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+        EvaluationState state) {
       SkyFunctionName type = skyKey.functionName();
-      if (type.equals(SkyFunctions.TARGET_COMPLETION) && node != null) {
-        TargetCompletionValue val = (TargetCompletionValue) node;
-        ConfiguredTarget target = val.getConfiguredTarget();
-        builtTargets.add(target);
-        eventBus.post(TargetCompleteEvent.createSuccessful(target));
-      } else if (type.equals(SkyFunctions.ASPECT_COMPLETION) && node != null) {
-        AspectCompletionValue val = (AspectCompletionValue) node;
-        eventBus.post(AspectCompleteEvent.createSuccessful(val.getAspectValue()));
+      if (type.equals(SkyFunctions.TARGET_COMPLETION)) {
+        TargetCompletionValue value = (TargetCompletionValue) skyValueSupplier.get();
+        if (value != null) {
+          ConfiguredTarget target = value.getConfiguredTarget();
+          builtTargets.add(target);
+          eventBus.post(TargetCompleteEvent.createSuccessful(target));
+        }
+      } else if (type.equals(SkyFunctions.ASPECT_COMPLETION)) {
+        AspectCompletionValue value = (AspectCompletionValue) skyValueSupplier.get();
+        if (value != null) {
+          eventBus.post(AspectCompleteEvent.createSuccessful(value.getAspectValue()));
+        }
       } else if (type.equals(SkyFunctions.ACTION_EXECUTION)) {
         // Remember all completed actions, even those in error, regardless of having been cached or
         // really executed.
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 eed73aef66..8f1754d933 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
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -493,13 +494,16 @@ public final class SkyframeBuildView {
     public void enqueueing(SkyKey skyKey) {}
 
     @Override
-    public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
-      if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET) && value != null) {
+    public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+        EvaluationState state) {
+      if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
         switch (state) {
           case BUILT:
-            evaluatedConfiguredTargets.add(skyKey);
-            // During multithreaded operation, this is only set to true, so no concurrency issues.
-            someConfiguredTargetEvaluated = true;
+            if (skyValueSupplier.get() != null) {
+              evaluatedConfiguredTargets.add(skyKey);
+              // During multithreaded operation, this is only set to true, so no concurrency issues.
+              someConfiguredTargetEvaluated = true;
+            }
             break;
           case CLEAN:
             // If the configured target value did not need to be rebuilt, then it wasn't truly
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 cbcc676aa4..e188bf3b7c 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
@@ -1599,15 +1599,15 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     }
 
     @Override
-    public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+    public void evaluated(SkyKey skyKey, Supplier<SkyValue> valueSupplier, EvaluationState state) {
       if (ignoreInvalidations) {
         return;
       }
       if (skyframeBuildView != null) {
-        skyframeBuildView.getInvalidationReceiver().evaluated(skyKey, value, state);
+        skyframeBuildView.getInvalidationReceiver().evaluated(skyKey, valueSupplier, state);
       }
       if (executionProgressReceiver != null) {
-        executionProgressReceiver.evaluated(skyKey, value, state);
+        executionProgressReceiver.evaluated(skyKey, valueSupplier, state);
       }
     }
   }
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 62dcf1e686..cddfc0cd14 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java
@@ -13,10 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import com.google.common.base.Supplier;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 
-import javax.annotation.Nullable;
-
 /**
  * Receiver to inform callers which values have been invalidated. Values may be invalidated and then
  * re-validated if they have been found not to be changed.
@@ -68,8 +67,9 @@ public interface EvaluationProgressReceiver {
    *
    * <p>{@code state} indicates the new state of the node.
    *
-   * <p>If the value builder threw an error when building this node, then {@code value} is null.
+   * <p>If the value builder threw an error when building this node, then
+   * {@code valueSupplier.get()} evaluates to null.
    */
   @ThreadSafety.ThreadSafe
-  void evaluated(SkyKey skyKey, @Nullable SkyValue value, EvaluationState state);
+  void evaluated(SkyKey skyKey, Supplier<SkyValue> valueSupplier, EvaluationState state);
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index 09296b5ca7..f56dc56331 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -16,6 +16,8 @@ package com.google.devtools.build.skyframe;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -106,6 +108,20 @@ public final class ParallelEvaluator implements Evaluator {
     }
   };
 
+  private static class SkyValueSupplier implements Supplier<SkyValue> {
+
+    private final NodeEntry state;
+
+    public SkyValueSupplier(NodeEntry state) {
+      this.state = state;
+    }
+
+    @Override
+    public SkyValue get() {
+      return state.getValue();
+    }
+  }
+
   private final ImmutableMap<? extends SkyFunctionName, ? extends SkyFunction> skyFunctions;
 
   private final EventHandler reporter;
@@ -459,7 +475,7 @@ public final class ParallelEvaluator implements Evaluator {
         // by the Preconditions check above, and was not actually changed this run -- when it was
         // written above, its version stayed below this update's version, so its value remains the
         // same as before.
-        progressReceiver.evaluated(skyKey, value,
+        progressReceiver.evaluated(skyKey, Suppliers.ofInstance(value),
             valueVersion.equals(graphVersion) ? EvaluationState.BUILT : EvaluationState.CLEAN);
       }
       signalValuesAndEnqueueIfReady(enqueueParents ? visitor : null, reverseDeps, valueVersion);
@@ -670,10 +686,10 @@ public final class ParallelEvaluator implements Evaluator {
             // without any re-evaluation.
             visitor.notifyDone(skyKey);
             Set<SkyKey> reverseDeps = state.markClean();
-            SkyValue value = state.getValue();
             if (progressReceiver != null) {
               // Tell the receiver that the value was not actually changed this run.
-              progressReceiver.evaluated(skyKey, value, EvaluationState.CLEAN);
+              progressReceiver.evaluated(skyKey, new SkyValueSupplier(state),
+                  EvaluationState.CLEAN);
             }
             if (!keepGoing && state.getErrorInfo() != null) {
               if (!visitor.preventNewEvaluations()) {
@@ -920,7 +936,7 @@ public final class ParallelEvaluator implements Evaluator {
       // retrieve them, but top-level nodes are presumably of more interest.
       // If valueVersion is not equal to graphVersion, it must be less than it (by the
       // Preconditions check above), and so the node is clean.
-      progressReceiver.evaluated(key, value, valueVersion.equals(graphVersion)
+      progressReceiver.evaluated(key, Suppliers.ofInstance(value), valueVersion.equals(graphVersion)
           ? EvaluationState.BUILT
           : EvaluationState.CLEAN);
     }
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 9c2f7991b9..8a46276d77 100644
--- a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
@@ -24,6 +24,7 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
@@ -174,7 +175,8 @@ public class EagerInvalidatorTest {
       }
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         throw new UnsupportedOperationException();
       }
     };
@@ -210,7 +212,8 @@ public class EagerInvalidatorTest {
       }
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         throw new UnsupportedOperationException();
       }
     };
@@ -248,7 +251,8 @@ public class EagerInvalidatorTest {
       }
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         throw new UnsupportedOperationException();
       }
     };
@@ -358,7 +362,8 @@ public class EagerInvalidatorTest {
       }
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         throw new UnsupportedOperationException();
       }
     };
@@ -386,7 +391,8 @@ public class EagerInvalidatorTest {
       }
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         throw new UnsupportedOperationException();
       }
     };
@@ -484,7 +490,8 @@ public class EagerInvalidatorTest {
         }
 
         @Override
-        public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+        public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+            EvaluationState state) {
           throw new UnsupportedOperationException();
         }
       };
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 d414feaff0..5d90a50e61 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -31,6 +31,7 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -255,7 +256,8 @@ public class ParallelEvaluatorTest {
       public void enqueueing(SkyKey key) {}
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         receivedValues.add(skyKey);
       }
     };
@@ -1896,7 +1898,8 @@ public class ParallelEvaluatorTest {
       }
 
       @Override
-      public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+      public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
+          EvaluationState state) {
         evaluatedValues.add(skyKey);
       }
     };
diff --git a/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java b/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java
index a9a7af3431..090f04a850 100644
--- a/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java
+++ b/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.skyframe;
 
 import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
 import com.google.common.collect.Sets;
 
 import java.util.Set;
@@ -49,9 +50,9 @@ public class TrackingInvalidationReceiver implements EvaluationProgressReceiver
   }
 
   @Override
-  public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) {
+  public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier, EvaluationState state) {
     evaluated.add(skyKey);
-    if (value != null) {
+    if (skyValueSupplier.get() != null) {
       deleted.remove(skyKey);
       if (state.equals(EvaluationState.CLEAN)) {
         dirty.remove(skyKey);