From 8ae74279a5fc14a064dcab6c5f5d6333aa8721e8 Mon Sep 17 00:00:00 2001
From: Googler <jhorvitz@google.com>
Date: Fri, 22 Sep 2023 07:54:53 -0700
Subject: [PATCH] Use a special `MarkedDirtyResult` for rewinding that
 prohibits retrieving the reverse deps.

Even after rewinding support is added for incremental nodes, the reverse deps are not needed when marking a node dirty for rewinding. Not including them at all will make the eventual implementation of `markDirty(DirtyType.REWIND)` on an incremental node easier to understand.

PiperOrigin-RevId: 567621648
Change-Id: I8a0fd8811606fc800cdb362e433045ee4579baab
---
 .../devtools/build/skyframe/NodeEntry.java    | 52 +++++++++++++------
 .../NonIncrementalInMemoryNodeEntry.java      |  3 +-
 2 files changed, 38 insertions(+), 17 deletions(-)

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 f8c197e95c18ea..41d0a6c7d82047 100644
--- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -13,7 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
-import com.google.common.collect.ImmutableList;
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.skyframe.SkyFunction.Reset;
@@ -153,33 +154,54 @@ enum DirtyType {
   MarkedDirtyResult markDirty(DirtyType dirtyType) throws InterruptedException;
 
   /**
-   * Returned by {@link #markDirty} if that call changed the node from done to dirty. Contains a
-   * {@link Collection} of the node's reverse deps for efficiency, because an important use case for
-   * {@link #markDirty} is during invalidation, and the invalidator must immediately afterwards
-   * schedule the invalidation of a node's reverse deps if the invalidator successfully dirties that
-   * node.
+   * Returned by {@link #markDirty} if that call changed the node from done to dirty.
+   *
+   * <p>For nodes marked dirty during invalidation ({@link DirtyType#DIRTY} and {@link
+   * DirtyType#CHANGE}), contains a {@link Collection} of the node's reverse deps for efficiency, so
+   * that the invalidator can schedule the invalidation of a node's reverse deps immediately
+   * afterwards.
+   *
+   * <p>For nodes marked dirty intra-evaluation ({@link DirtyType#REWIND}), reverse deps are not
+   * needed by the caller, so {@link #getReverseDepsUnsafe} must not be called.
    *
    * <p>Warning: {@link #getReverseDepsUnsafe()} may return a live view of the reverse deps
    * collection of the marked-dirty node. The consumer of this data must be careful only to iterate
    * over and consume its values while that collection is guaranteed not to change. This is true
    * during invalidation, because reverse deps don't change during invalidation.
    */
-  final class MarkedDirtyResult {
+  abstract class MarkedDirtyResult {
 
-    private static final MarkedDirtyResult NO_RDEPS = new MarkedDirtyResult(ImmutableList.of());
+    private static final MarkedDirtyResult RESULT_FOR_REWINDING =
+        new MarkedDirtyResult() {
+          @Override
+          public Collection<SkyKey> getReverseDepsUnsafe() {
+            throw new IllegalStateException("Should not need reverse deps for rewinding");
+          }
+        };
 
     public static MarkedDirtyResult withReverseDeps(Collection<SkyKey> reverseDepsUnsafe) {
-      return reverseDepsUnsafe.isEmpty() ? NO_RDEPS : new MarkedDirtyResult(reverseDepsUnsafe);
+      return new ResultWithReverseDeps(reverseDepsUnsafe);
     }
 
-    private final Collection<SkyKey> reverseDepsUnsafe;
-
-    private MarkedDirtyResult(Collection<SkyKey> reverseDepsUnsafe) {
-      this.reverseDepsUnsafe = reverseDepsUnsafe;
+    static MarkedDirtyResult forRewinding() {
+      return RESULT_FOR_REWINDING;
     }
 
-    public Collection<SkyKey> getReverseDepsUnsafe() {
-      return reverseDepsUnsafe;
+    private MarkedDirtyResult() {}
+
+    public abstract Collection<SkyKey> getReverseDepsUnsafe();
+
+    private static final class ResultWithReverseDeps extends MarkedDirtyResult {
+      private final Collection<SkyKey> reverseDepsUnsafe;
+
+      private ResultWithReverseDeps(Collection<SkyKey> reverseDepsUnsafe) {
+        this.reverseDepsUnsafe = checkNotNull(reverseDepsUnsafe);
+      }
+
+      @Override
+      public Collection<SkyKey> getReverseDepsUnsafe() {
+        return reverseDepsUnsafe;
+      }
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java
index e9839244547161..8e84aaa47fe895 100644
--- a/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java
@@ -19,7 +19,6 @@
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.MoreObjects;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.skyframe.NonIncrementalInMemoryNodeEntry.NonIncrementalBuildingState;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
@@ -152,7 +151,7 @@ public final synchronized MarkedDirtyResult markDirty(DirtyType dirtyType) {
     }
     dirtyBuildingState = new NonIncrementalBuildingState();
     value = null;
-    return MarkedDirtyResult.withReverseDeps(ImmutableList.of());
+    return MarkedDirtyResult.forRewinding();
   }
 
   @Override