From 87181b8b215c4bb8760052ae4060eacd2afac8e6 Mon Sep 17 00:00:00 2001
From: Googler <jhorvitz@google.com>
Date: Thu, 3 Aug 2023 08:45:48 -0700
Subject: [PATCH] Use a `ConstantVersion` for non-incremental evaluations.

This is a prerequisite for not storing a version field in edgeless nodes. In order to satisfy a check in `ParallelEvaluator#informProgressReceiverThatValueIsDone`, the node and the evaluator must agree on the version.

Also skip calling `deleteDirty` if incremental state is not being tracked, since it doesn't make sense with a non-`IntVersion` and is irrelevant without incrementality anyway.

PiperOrigin-RevId: 553489413
Change-Id: Iedd5ad22e99ad3ef7c9398b2105ff289e1fd00b8
---
 .../lib/skyframe/SequencedSkyframeExecutor.java  |  4 +++-
 ...actIncrementalInMemoryMemoizingEvaluator.java | 13 ++++++++-----
 .../skyframe/InMemoryMemoizingEvaluator.java     | 16 +++++++++++++---
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 47b55e0b5bdf6b..e62c57cebf37af 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -678,7 +678,9 @@ protected ExecutionFinishedEvent.Builder createExecutionFinishedEventInternal()
   public void deleteOldNodes(long versionWindowForDirtyGc) {
     // TODO(bazel-team): perhaps we should come up with a separate GC class dedicated to maintaining
     // value garbage. If we ever do so, this logic should be moved there.
-    memoizingEvaluator.deleteDirty(versionWindowForDirtyGc);
+    if (trackIncrementalState) {
+      memoizingEvaluator.deleteDirty(versionWindowForDirtyGc);
+    }
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractIncrementalInMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractIncrementalInMemoryMemoizingEvaluator.java
index c36880fa86f42a..7a212553b27a85 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractIncrementalInMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractIncrementalInMemoryMemoizingEvaluator.java
@@ -50,9 +50,11 @@ public abstract class AbstractIncrementalInMemoryMemoizingEvaluator
   protected final GraphInconsistencyReceiver graphInconsistencyReceiver;
   protected final EventFilter eventFilter;
 
-  // Keep edges in graph. Can be false to save memory, in which case incremental builds are
-  // not possible.
-  private final boolean keepEdges;
+  /**
+   * Whether to store edges in the graph. Can be false to save memory, in which case incremental
+   * builds are not possible, and all evaluations will be at {@link ConstantVersion#INSTANCE}.
+   */
+  final boolean keepEdges;
 
   // Values that the caller explicitly specified are assumed to be changed -- they will be
   // re-evaluated even if none of their children are changed.
@@ -60,7 +62,8 @@ public abstract class AbstractIncrementalInMemoryMemoizingEvaluator
 
   protected final EmittedEventState emittedEventState;
 
-  protected IntVersion lastGraphVersion = null;
+  // Null until the first incremental evaluation completes. Always null when not keeping edges.
+  @Nullable protected IntVersion lastGraphVersion = null;
 
   protected AbstractIncrementalInMemoryMemoizingEvaluator(
       ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions,
@@ -128,7 +131,7 @@ protected void pruneInjectedValues(Map<SkyKey, Delta> valuesToInject) {
   }
 
   /** Injects values in {@code valuesToInject} into the graph. */
-  protected void injectValues(IntVersion version) {
+  protected void injectValues(Version version) {
     if (valuesToInject.isEmpty()) {
       return;
     }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index dbaa35338248d4..5a12abba28b974 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -37,7 +37,8 @@
  * the returned graphs. However, it is allowed to access the graph from multiple threads as long as
  * that does not happen in parallel with an {@link #evaluate} call.
  *
- * <p>This memoizing evaluator uses a monotonically increasing {@link IntVersion}.
+ * <p>This memoizing evaluator uses a monotonically increasing {@link IntVersion} for incremental
+ * evaluations and {@link ConstantVersion#INSTANCE} for non-incremental evaluations.
  */
 public final class InMemoryMemoizingEvaluator
     extends AbstractIncrementalInMemoryMemoizingEvaluator {
@@ -126,7 +127,14 @@ public <T extends SkyValue> EvaluationResult<T> evaluate(
       Iterable<? extends SkyKey> roots, EvaluationContext evaluationContext)
       throws InterruptedException {
     // NOTE: Performance critical code. See bug "Null build performance parity".
-    IntVersion graphVersion = lastGraphVersion == null ? IntVersion.of(0) : lastGraphVersion.next();
+    Version graphVersion;
+    if (!keepEdges) {
+      graphVersion = ConstantVersion.INSTANCE;
+    } else if (lastGraphVersion == null) {
+      graphVersion = IntVersion.of(0);
+    } else {
+      graphVersion = lastGraphVersion.next();
+    }
     setAndCheckEvaluateState(true, roots);
     try {
       // Mark for removal any inflight nodes from the previous evaluation.
@@ -181,7 +189,9 @@ public <T extends SkyValue> EvaluationResult<T> evaluate(
           .setWalkableGraph(new DelegatingWalkableGraph(graph))
           .build();
     } finally {
-      lastGraphVersion = graphVersion;
+      if (keepEdges) {
+        lastGraphVersion = (IntVersion) graphVersion;
+      }
       setAndCheckEvaluateState(false, roots);
     }
   }