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
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 bd72026..0ab5e80 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 @@
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 @@
}
@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 eed73ae..8f1754d 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 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 cbcc676..e188bf3 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 @@
}
@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 62dcf1e..cddfc0c 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 @@
*
* <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 09296b5..f56dc56 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 @@
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 @@
}
};
+ 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 @@
// 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 @@
// 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 @@
// 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);
}
