Allow passing a minimal version to use for skyframe evaluations.

Allows the evaluator to specify a version compatible with its versioning scheme. This minimal version is used for the MTSV of hermetic nodes with no non-hermetic dependencies (e.g. `FileSymlinkCycleUniquenessFunction`) instead of the previously used `null`. This way it can be distinguished from a transient error which also uses `null`. `InMemoryNodeEntry` no longer considers the MTSV, since no evaluator implementations depend on that behavior. PiperOrigin-RevId: 445933706
bazelbuild · May 2, 2022 · 27c0896 · 27c0896
1 parent 691fc5b
commit 27c0896
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 16 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
@@ -125,6 +125,7 @@ abstract class AbstractParallelEvaluator {
  AbstractParallelEvaluator(
  ProcessableGraph graph,
  Version graphVersion,
+ Version minimalVersion,
  ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
  ExtendedEventHandler reporter,
  EmittedEventState emittedEventState,
@@ -146,6 +147,7 @@ abstract class AbstractParallelEvaluator {
  new ParallelEvaluatorContext(
  graph,
  graphVersion,
+ minimalVersion,
  skyFunctions,
  reporter,
  emittedEventState,
@@ -161,7 +163,7 @@ abstract class AbstractParallelEvaluator {
  stateCache);
  }
 
- private Supplier<QuiescingExecutor> getQuiescingExecutorSupplier(
+ private static Supplier<QuiescingExecutor> getQuiescingExecutorSupplier(
  Supplier<ExecutorService> executorService,
  int cpuHeavySkyKeysThreadPoolSize,
  int executionJobsThreadPoolSize) {

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -186,6 +186,7 @@ public <T extends SkyValue> EvaluationResult<T> evaluate(
  new ParallelEvaluator(
  graph,
  graphVersion,
+ MinimalVersion.INSTANCE,
  skyFunctions,
  evaluationContext.getEventHandler(),
  emittedEventState,

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -13,8 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
-import static com.google.common.base.MoreObjects.firstNonNull;
-
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -276,14 +274,14 @@ public synchronized Set<SkyKey> setValue(
  SkyValue value, Version graphVersion, @Nullable Version maxTransitiveSourceVersion)
  throws InterruptedException {
  Preconditions.checkState(isReady(), "Not ready (this=%s, value=%s)", this, value);
- Version version = firstNonNull(maxTransitiveSourceVersion, graphVersion);
  Preconditions.checkState(
- this.lastChangedVersion.atMost(version) && this.lastEvaluatedVersion.atMost(version),
+ this.lastChangedVersion.atMost(graphVersion)
+ && this.lastEvaluatedVersion.atMost(graphVersion),
  "Bad version (this=%s, version=%s, value=%s)",
  this,
- version,
+ graphVersion,
  value);
- this.lastEvaluatedVersion = version;
+ this.lastEvaluatedVersion = graphVersion;
 
  if (dirtyBuildingState.unchangedFromLastBuild(value)) {
  // If the value is the same as before, just use the old value. Note that we don't use the new
@@ -292,7 +290,7 @@ public synchronized Set<SkyKey> setValue(
  } else {
  // If this is a new value, or it has changed since the last build, set the version to the
  // current graph version.
- this.lastChangedVersion = version;
+ this.lastChangedVersion = graphVersion;
  this.value = value;
  }
  return setStateFinishedAndReturnReverseDepsToSignal();

diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -61,8 +61,9 @@ public class ParallelEvaluator extends AbstractParallelEvaluator {
  public ParallelEvaluator(
  ProcessableGraph graph,
  Version graphVersion,
+ Version minimalVersion,
  ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
- final ExtendedEventHandler reporter,
+ ExtendedEventHandler reporter,
  EmittedEventState emittedEventState,
  EventFilter storedEventFilter,
  ErrorInfoManager errorInfoManager,
@@ -77,6 +78,7 @@ public ParallelEvaluator(
  super(
  graph,
  graphVersion,
+ minimalVersion,
  skyFunctions,
  reporter,
  emittedEventState,
@@ -507,7 +509,7 @@ private Map<SkyKey, ValueWithMetadata> bubbleErrorUp(
  * <p>{@code visitor} may be null, but only in the case where all graph entries corresponding to
  * {@code skyKeys} are known to be in the DONE state ({@code entry.isDone()} returns true).
  */
- <T extends SkyValue> EvaluationResult<T> constructResult(
+ private <T extends SkyValue> EvaluationResult<T> constructResult(
  Iterable<SkyKey> skyKeys,
  @Nullable Map<SkyKey, ValueWithMetadata> bubbleErrorInfo,
  boolean catastrophe)

diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
@@ -39,6 +39,7 @@ class ParallelEvaluatorContext {
 
  private final QueryableGraph graph;
  private final Version graphVersion;
+ private final Version minimalVersion;
  private final ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions;
  private final ExtendedEventHandler reporter;
  private final NestedSetVisitor<TaggedEvents> replayingNestedSetEventVisitor;
@@ -74,6 +75,7 @@ interface ComparableRunnable extends Runnable, Comparable<ComparableRunnable> {}
  public ParallelEvaluatorContext(
  QueryableGraph graph,
  Version graphVersion,
+ Version minimalVersion,
  ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
  ExtendedEventHandler reporter,
  EmittedEventState emittedEventState,
@@ -87,6 +89,7 @@ public ParallelEvaluatorContext(
  Cache<SkyKey, SkyKeyComputeState> stateCache) {
  this.graph = graph;
  this.graphVersion = graphVersion;
+ this.minimalVersion = minimalVersion;
  this.skyFunctions = skyFunctions;
  this.reporter = reporter;
  this.graphInconsistencyReceiver = graphInconsistencyReceiver;
@@ -151,6 +154,10 @@ Version getGraphVersion() {
  return graphVersion;
  }
 
+ Version getMinimalVersion() {
+ return minimalVersion;
+ }
+
  boolean keepGoing() {
  return keepGoing;
  }

diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
@@ -414,7 +414,21 @@ interface SkyKeyComputeState {}
  * Returns the max transitive source version of a {@link NodeEntry}.
  *
  * <p>This value might not consider all deps' source versions if called before all deps have
- * been requested or if {@link #valuesMissing()} returns true.
+ * been requested or if {@link #valuesMissing} returns {@code true}.
+ *
+ * <p>Rules for calculation of the max transitive source version:
+ *
+ * <ul>
+ * <li>Returns {@code null} during cycle detection and error bubbling, or for transient
+ * errors.
+ * <li>If the node is {@link FunctionHermeticity#NONHERMETIC}, returns the version passed to
+ * {@link #injectVersionForNonHermeticFunction} if it was called, or else {@code null}.
+ * <li>For all other nodes, queries {@link NodeEntry#getMaxTransitiveSourceVersion} of direct
+ * dependency nodes and chooses the maximal version seen (according to {@link
+ * Version#atMost}). If there are no direct dependencies, returns {@link
+ * ParallelEvaluatorContext#getMinimalVersion}. If any direct dependency node has a {@code
+ * null} MTSV, returns {@code null}.
+ * </ul>
  */
  @Nullable
  Version getMaxTransitiveSourceVersionSoFar();

diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -207,7 +207,7 @@ private SkyFunctionEnvironment(
  this.maxTransitiveSourceVersion =
  bubbleErrorInfo == null
  && skyKey.functionName().getHermeticity() != FunctionHermeticity.NONHERMETIC
- ? MinimalVersion.INSTANCE
+ ? evaluatorContext.getMinimalVersion()
  : null;
  this.previouslyRequestedDepsValues = batchPrefetch(throwIfPreviouslyRequestedDepsUndone);
  Preconditions.checkState(
@@ -847,10 +847,6 @@ Set<SkyKey> commitAndGetParents(NodeEntry primaryEntry) throws InterruptedExcept
  }
  }
 
- if (temporaryDirectDeps.isEmpty()
- && skyKey.functionName().getHermeticity() != FunctionHermeticity.NONHERMETIC) {
- maxTransitiveSourceVersion = null; // No dependencies on source.
- }
  Preconditions.checkState(
  maxTransitiveSourceVersion == null || newlyRegisteredDeps.isEmpty(),
  "Dependency registration not supported when tracking max transitive source versions");

diff --git a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
@@ -137,6 +137,7 @@ protected <T extends SkyValue> EvaluationResult<T> eval(boolean keepGoing, SkyKe
  new ParallelEvaluator(
  graph,
  graphVersion,
+ MinimalVersion.INSTANCE,
  tester.getSkyFunctionMap(),
  reporter,
  new MemoizingEvaluator.EmittedEventState(),

diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -120,6 +120,7 @@ private ParallelEvaluator makeEvaluator(
  return new ParallelEvaluator(
  graph,
  oldGraphVersion,
+ MinimalVersion.INSTANCE,
  builders,
  storedEventHandler,
  new MemoizingEvaluator.EmittedEventState(),
@@ -3118,6 +3119,7 @@ public void onEvaluationFinished() {
  new ParallelEvaluator(
  graph,
  graphVersion,
+ MinimalVersion.INSTANCE,
  tester.getSkyFunctionMap(),
  storedEventHandler,
  new MemoizingEvaluator.EmittedEventState(),