Convert directDeps to a map of SkyValues

SkyFunctionEnvironment only cares about directDeps' values, not other
NodeEntry data.

This reduces the space of code which could be sensitive to nodes which
transition from done to dirty during evaluation.

To prevent check-then-act races in the refactored code (and only there;
other code will be fixed in future refactorings), instead of checking
deps' isDone() methods before accessing their value, allow
getValueMaybeWithMetadata to be called when not done, and have it return
null when not done.

(Note that done->dirty node transitions during evaluation are planned,
but not yet possible.)

RELNOTES: None.
PiperOrigin-RevId: 202518781

4 files changed, 78 insertions, 34 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
index b607e2fa20..abca3b25c4 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
@@ -424,20 +424,10 @@ public abstract class AbstractParallelEvaluator {
               }
             }
 
-            Map<SkyKey, ? extends NodeEntry> newlyRequestedDeps =
-                evaluatorContext.getBatchValues(
-                    skyKey, Reason.DONE_CHECKING, env.getNewlyRequestedDeps());
-            boolean isTransitivelyTransient = reifiedBuilderException.isTransient();
-            for (NodeEntry depEntry :
-                Iterables.concat(env.getDirectDepsValues(), newlyRequestedDeps.values())) {
-              if (!isDoneForBuild(depEntry)) {
-                continue;
-              }
-              ErrorInfo depError = depEntry.getErrorInfo();
-              if (depError != null) {
-                isTransitivelyTransient |= depError.isTransitivelyTransient();
-              }
-            }
+            boolean isTransitivelyTransient =
+                reifiedBuilderException.isTransient()
+                    || env.isAnyDirectDepErrorTransitivelyTransient()
+                    || env.isAnyNewlyRequestedDepErrorTransitivelyTransient();
             ErrorInfo errorInfo = evaluatorContext.getErrorInfoManager().fromException(
                 skyKey,
                 reifiedBuilderException,
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 bb6a2d1d4b..859f531666 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -194,8 +194,8 @@ public class InMemoryNodeEntry implements NodeEntry {
   }
 
   @Override
+  @Nullable
   public SkyValue getValueMaybeWithMetadata() {
-    Preconditions.checkState(isDone(), "no value until done: %s", this);
     return value;
   }
 
diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
index aee68b3b91..a9f1d78bfd 100644
--- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -119,12 +119,17 @@ public interface NodeEntry extends ThinNodeEntry {
 
   /**
    * Returns raw {@link SkyValue} stored in this entry, which may include metadata associated with
-   * it (like events and errors). This method may only be called after the evaluation of this node
-   * is complete, i.e., after {@link #setValue} has been called.
+   * it (like events and errors).
+   *
+   * <p>This method returns {@code null} if the evaluation of this node is not complete, i.e.,
+   * after node creation or dirtying and before {@link #setValue} has been called. Callers should
+   * assert that the returned value is not {@code null} whenever they expect the node should be
+   * done.
    *
    * <p>Use the static methods of {@link ValueWithMetadata} to extract metadata if necessary.
    */
   @ThreadSafe
+  @Nullable
   SkyValue getValueMaybeWithMetadata() throws InterruptedException;
 
   /** Returns the value, even if dirty or changed. Returns null otherwise. */
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
index 638a7615ff..333b0e41de 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -14,11 +14,11 @@
 package com.google.devtools.build.skyframe;
 
 import static com.google.devtools.build.skyframe.AbstractParallelEvaluator.isDoneForBuild;
-import static com.google.devtools.build.skyframe.ParallelEvaluator.maybeGetValueFromError;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
@@ -37,11 +37,11 @@ import com.google.devtools.build.skyframe.ParallelEvaluatorContext.EnqueueParent
 import com.google.devtools.build.skyframe.QueryableGraph.Reason;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import javax.annotation.Nullable;
@@ -71,8 +71,15 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
   private SkyValue value = null;
   private ErrorInfo errorInfo = null;
   private final Map<SkyKey, ValueWithMetadata> bubbleErrorInfo;
-  /** The values previously declared as dependencies. */
-  private final Map<SkyKey, NodeEntry> directDeps;
+
+  /**
+   * The values previously declared as dependencies.
+   *
+   * <p>Values in this map are either {@link #NULL_MARKER} or were retrieved via {@link
+   * NodeEntry#getValueMaybeWithMetadata}. In the latter case, they should be processed using the
+   * static methods of {@link ValueWithMetadata}.
+   */
+  private final Map<SkyKey, SkyValue> directDeps;
 
   /**
    * The grouped list of values requested during this build as dependencies. On a subsequent build,
@@ -132,9 +139,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     this.oldDeps = oldDeps;
     this.evaluatorContext = evaluatorContext;
     this.directDeps =
-        Collections.<SkyKey, NodeEntry>unmodifiableMap(
-            batchPrefetch(
-                skyKey, directDeps, oldDeps, /*assertDone=*/ bubbleErrorInfo == null, skyKey));
+        batchPrefetch(skyKey, directDeps, oldDeps, /*assertDone=*/ bubbleErrorInfo == null, skyKey);
     this.bubbleErrorInfo = bubbleErrorInfo;
     Preconditions.checkState(
         !this.directDeps.containsKey(ErrorTransienceValue.KEY),
@@ -142,7 +147,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
         skyKey);
   }
 
-  private Map<SkyKey, ? extends NodeEntry> batchPrefetch(
+  private Map<SkyKey, SkyValue> batchPrefetch(
       SkyKey requestor,
       GroupedList<SkyKey> depKeys,
       Set<SkyKey> oldDeps,
@@ -176,13 +181,18 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
               + ": "
               + Sets.difference(depKeys.toSet(), batchMap.keySet()));
     }
-    if (assertDone) {
-      for (Map.Entry<SkyKey, ? extends NodeEntry> entry : batchMap.entrySet()) {
-        Preconditions.checkState(
-            entry.getValue().isDone(), "%s had not done %s", keyForDebugging, entry);
+    ImmutableMap.Builder<SkyKey, SkyValue> depValuesBuilder =
+        ImmutableMap.builderWithExpectedSize(batchMap.size());
+    for (Entry<SkyKey, ? extends NodeEntry> entry : batchMap.entrySet()) {
+      SkyValue valueMaybeWithMetadata = entry.getValue().getValueMaybeWithMetadata();
+      if (assertDone) {
+        Preconditions.checkNotNull(
+            valueMaybeWithMetadata, "%s had not done %s", keyForDebugging, entry);
       }
+      depValuesBuilder.put(
+          entry.getKey(), valueMaybeWithMetadata == null ? NULL_MARKER : valueMaybeWithMetadata);
     }
-    return batchMap;
+    return depValuesBuilder.build();
   }
 
   private void checkActive() {
@@ -339,8 +349,14 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
   }
 
   @Nullable
-  private SkyValue maybeGetValueFromErrorOrDeps(SkyKey key) throws InterruptedException {
-    return maybeGetValueFromError(key, directDeps.get(key), bubbleErrorInfo);
+  private SkyValue maybeGetValueFromErrorOrDeps(SkyKey key) {
+    if (bubbleErrorInfo != null) {
+      ValueWithMetadata bubbleErrorInfoValue = bubbleErrorInfo.get(key);
+      if (bubbleErrorInfoValue != null) {
+        return bubbleErrorInfoValue;
+      }
+    }
+    return directDeps.get(key);
   }
 
   private static SkyValue getValueOrNullMarker(@Nullable NodeEntry nodeEntry)
@@ -476,8 +492,41 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     return newlyRequestedDeps;
   }
 
-  Collection<NodeEntry> getDirectDepsValues() {
-    return directDeps.values();
+  boolean isAnyDirectDepErrorTransitivelyTransient() {
+    Preconditions.checkState(
+        bubbleErrorInfo == null,
+        "Checking dep error transitive transience during error bubbling for: %s",
+        skyKey);
+    for (SkyValue skyValue : directDeps.values()) {
+      ErrorInfo maybeErrorInfo = ValueWithMetadata.getMaybeErrorInfo(skyValue);
+      if (maybeErrorInfo != null && maybeErrorInfo.isTransitivelyTransient()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  boolean isAnyNewlyRequestedDepErrorTransitivelyTransient() throws InterruptedException {
+    // TODO(mschaller): consider collecting SkyValues of newly requested deps as they're requested
+    // which would allow this code to avoid graph queries for nodes already queried.
+    //
+    // This will also be necessary for correct behavior in the presence of node re-dirtying.
+      Preconditions.checkState(
+          bubbleErrorInfo == null,
+          "Checking dep error transitive transience during error bubbling for: %s",
+          skyKey);
+    Map<SkyKey, ? extends NodeEntry> newlyRequestedDeps =
+        evaluatorContext.getBatchValues(skyKey, Reason.DONE_CHECKING, getNewlyRequestedDeps());
+    for (NodeEntry depEntry : newlyRequestedDeps.values()) {
+      if (!isDoneForBuild(depEntry)) {
+        continue;
+      }
+      ErrorInfo depError = depEntry.getErrorInfo();
+      if (depError != null && depError.isTransitivelyTransient()) {
+        return true;
+      }
+    }
+    return false;
   }
 
   Collection<ErrorInfo> getChildErrorInfos() {