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
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 b607e2f..abca3b2 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 @@
}
}
- 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 bb6a2d1..859f531 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 @@
}
@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 aee68b3..a9f1d78 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 @@
/**
* 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 638a761..333b0e4 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.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 @@
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 @@
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 @@
skyKey);
}
- private Map<SkyKey, ? extends NodeEntry> batchPrefetch(
+ private Map<SkyKey, SkyValue> batchPrefetch(
SkyKey requestor,
GroupedList<SkyKey> depKeys,
Set<SkyKey> oldDeps,
@@ -176,13 +181,18 @@
+ ": "
+ 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 @@
}
@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 @@
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() {
