Refactor QueryableGraph and ThinNodeQueryableGraph to be independent interfaces, in preparation for further changes.

--
MOS_MIGRATED_REVID=126924789

13 files changed, 98 insertions, 116 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java
index 8e9ddb62b7..d714924e29 100644
--- a/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java
@@ -157,22 +157,22 @@ public abstract class DelegatingNodeEntry implements NodeEntry {
 
   @Override
   public Iterable<SkyKey> getDirectDeps() {
-    return getThinDelegate().getDirectDeps();
+    return getDelegate().getDirectDeps();
   }
 
   @Override
   public void removeReverseDep(SkyKey reverseDep) {
-    getThinDelegate().removeReverseDep(reverseDep);
+    getDelegate().removeReverseDep(reverseDep);
   }
 
   @Override
   public void removeInProgressReverseDep(SkyKey reverseDep) {
-    getThinDelegate().removeInProgressReverseDep(reverseDep);
+    getDelegate().removeInProgressReverseDep(reverseDep);
   }
 
   @Override
   public Iterable<SkyKey> getReverseDeps() {
-    return getThinDelegate().getReverseDeps();
+    return getDelegate().getReverseDeps();
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/skyframe/DirtiableGraph.java b/src/main/java/com/google/devtools/build/skyframe/DeletableGraph.java
index fe7e88bdb8..d091f083d6 100644
--- a/src/main/java/com/google/devtools/build/skyframe/DirtiableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/DeletableGraph.java
@@ -19,13 +19,11 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
  * Interface for classes that need to remove values from graph. Currently just used by {@link
  * EagerInvalidator}.
  *
- * <p>This class is not intended for direct use, and is only exposed as public for use in
- * evaluation implementations outside of this package.
+ * <p>This class is not intended for direct use, and is only exposed as public for use in evaluation
+ * implementations outside of this package.
  */
 @ThreadSafe
-public interface DirtiableGraph extends QueryableGraph {
-  /**
-   * Remove the value with given name from the graph.
-   */
+public interface DeletableGraph {
+  /** Remove the value with given name from the graph. */
   void remove(SkyKey key);
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/EagerInvalidator.java b/src/main/java/com/google/devtools/build/skyframe/EagerInvalidator.java
index bb6d2b2685..b7c1ee9545 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EagerInvalidator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EagerInvalidator.java
@@ -43,9 +43,14 @@ public final class EagerInvalidator {
    * long as the full upward transitive closure of the nodes is specified for deletion, the graph
    * remains consistent.
    */
-  public static void delete(DirtiableGraph graph, Iterable<SkyKey> diff,
-      EvaluationProgressReceiver invalidationReceiver, InvalidationState state,
-      boolean traverseGraph, DirtyKeyTracker dirtyKeyTracker) throws InterruptedException {
+  public static void delete(
+      InMemoryGraph graph,
+      Iterable<SkyKey> diff,
+      EvaluationProgressReceiver invalidationReceiver,
+      InvalidationState state,
+      boolean traverseGraph,
+      DirtyKeyTracker dirtyKeyTracker)
+      throws InterruptedException {
     DeletingNodeVisitor visitor =
         createDeletingVisitorIfNeeded(
             graph, diff, invalidationReceiver, state, traverseGraph, dirtyKeyTracker);
@@ -56,7 +61,7 @@ public final class EagerInvalidator {
 
   @Nullable
   static DeletingNodeVisitor createDeletingVisitorIfNeeded(
-      DirtiableGraph graph,
+      InMemoryGraph graph,
       Iterable<SkyKey> diff,
       EvaluationProgressReceiver invalidationReceiver,
       InvalidationState state,
@@ -70,7 +75,7 @@ public final class EagerInvalidator {
 
   @Nullable
   static DirtyingNodeVisitor createInvalidatingVisitorIfNeeded(
-      ThinNodeQueryableGraph graph,
+      InvalidatableGraph graph,
       Iterable<SkyKey> diff,
       EvaluationProgressReceiver invalidationReceiver,
       InvalidationState state,
@@ -84,7 +89,7 @@ public final class EagerInvalidator {
 
   @Nullable
   private static DirtyingNodeVisitor createInvalidatingVisitorIfNeeded(
-      ThinNodeQueryableGraph graph,
+      InvalidatableGraph graph,
       Iterable<SkyKey> diff,
       EvaluationProgressReceiver invalidationReceiver,
       InvalidationState state,
@@ -110,7 +115,7 @@ public final class EagerInvalidator {
    * an executor constructed with the provided factory.
    */
   public static void invalidate(
-      ThinNodeQueryableGraph graph,
+      InvalidatableGraph graph,
       Iterable<SkyKey> diff,
       EvaluationProgressReceiver invalidationReceiver,
       InvalidationState state,
@@ -130,7 +135,7 @@ public final class EagerInvalidator {
    * the provided {@link ForkJoinPool}.
    */
   public static void invalidate(
-      ThinNodeQueryableGraph graph,
+      InvalidatableGraph graph,
       Iterable<SkyKey> diff,
       EvaluationProgressReceiver invalidationReceiver,
       InvalidationState state,
@@ -154,11 +159,9 @@ public final class EagerInvalidator {
     }
   }
 
-  /**
-   * Invalidates given values and their upward transitive closure in the graph.
-   */
+  /** Invalidates given values and their upward transitive closure in the graph. */
   public static void invalidate(
-      DirtiableGraph graph,
+      InvalidatableGraph graph,
       Iterable<SkyKey> diff,
       EvaluationProgressReceiver invalidationReceiver,
       InvalidationState state,
diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluableGraph.java b/src/main/java/com/google/devtools/build/skyframe/EvaluableGraph.java
index 7527146b30..b3d8bd0ac2 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluableGraph.java
@@ -22,7 +22,7 @@ import java.util.Map;
  * single version of the graph.
  */
 @ThreadSafe
-interface EvaluableGraph extends QueryableGraph {
+interface EvaluableGraph extends QueryableGraph, DeletableGraph {
   /**
    * Like {@link QueryableGraph#getBatch}, except it creates a new node for each key not already
    * present in the graph. Thus, the returned map will have an entry for each key in {@code keys}.
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
index 02735c0e8e..5d508a9eb1 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
@@ -16,7 +16,7 @@ package com.google.devtools.build.skyframe;
 import java.util.Map;
 
 /** {@link ProcessableGraph} that exposes the contents of the entire graph. */
-interface InMemoryGraph extends ProcessableGraph {
+interface InMemoryGraph extends ProcessableGraph, InvalidatableGraph {
   /**
    * Returns a read-only live view of the nodes in the graph. All node are included. Dirty values
    * include their Node value. Values in error have a null value.
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 a5e22221e6..5d853b7aff 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -284,9 +284,8 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
   }
 
   @Override
-  public void injectGraphTransformerForTesting(
-      Function<ThinNodeQueryableGraph, ProcessableGraph> transformer) {
-    this.graph = (InMemoryGraph) transformer.apply(this.graph);
+  public void injectGraphTransformerForTesting(GraphTransformerForTesting transformer) {
+    this.graph = transformer.transform(this.graph);
   }
 
   public ProcessableGraph getGraphForTesting() {
diff --git a/src/main/java/com/google/devtools/build/skyframe/ThinNodeQueryableGraph.java b/src/main/java/com/google/devtools/build/skyframe/InvalidatableGraph.java
index eeb3ef5867..243740705a 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ThinNodeQueryableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InvalidatableGraph.java
@@ -17,20 +17,14 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 
 import java.util.Map;
 
-import javax.annotation.Nullable;
-
 /**
- * A graph that exposes thin representations of its entries and structure, for use by classes that
- * must traverse it, but not read its entries' values.
+ * A graph that exposes thin representations of its entries and structure, for use during
+ * invalidation.
+ *
+ * <p>Public only for use in alternative graph implementations.
  */
 @ThreadSafe
-public interface ThinNodeQueryableGraph {
-  /**
-   * Returns the thin node with the given name, or {@code null} if the node does not exist.
-   */
-  @Nullable
-  ThinNodeEntry get(SkyKey key);
-
+public interface InvalidatableGraph {
   /**
    * Fetches all the given thin nodes. Returns a map {@code m} such that, for all {@code k} in
    * {@code keys}, {@code m.get(k).equals(e)} iff {@code get(k) == e} and {@code e != null}, and
diff --git a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
index 6eec236b4e..1fe90d193e 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
@@ -62,7 +62,7 @@ import javax.annotation.Nullable;
  *
  * <p>This is intended only for use in alternative {@code MemoizingEvaluator} implementations.
  */
-public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGraph> {
+public abstract class InvalidatingNodeVisitor<TGraph extends InvalidatableGraph> {
 
   // Default thread count is equal to the number of cores to exploit
   // that level of hardware parallelism, since invalidation should be CPU-bound.
@@ -236,13 +236,13 @@ public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGr
   }
 
   /** A node-deleting implementation. */
-  static class DeletingNodeVisitor extends InvalidatingNodeVisitor<DirtiableGraph> {
+  static class DeletingNodeVisitor extends InvalidatingNodeVisitor<InMemoryGraph> {
 
     private final Set<SkyKey> visited = Sets.newConcurrentHashSet();
     private final boolean traverseGraph;
 
     DeletingNodeVisitor(
-        DirtiableGraph graph,
+        InMemoryGraph graph,
         EvaluationProgressReceiver invalidationReceiver,
         InvalidationState state,
         boolean traverseGraph,
@@ -338,7 +338,7 @@ public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGr
   }
 
   /** A node-dirtying implementation. */
-  static class DirtyingNodeVisitor extends InvalidatingNodeVisitor<ThinNodeQueryableGraph> {
+  static class DirtyingNodeVisitor extends InvalidatingNodeVisitor<InvalidatableGraph> {
 
     private final Set<SkyKey> changed =
         Collections.newSetFromMap(
@@ -351,7 +351,7 @@ public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGr
     private final boolean supportInterruptions;
 
     protected DirtyingNodeVisitor(
-        ThinNodeQueryableGraph graph,
+        InvalidatableGraph graph,
         EvaluationProgressReceiver invalidationReceiver,
         InvalidationState state,
         DirtyKeyTracker dirtyKeyTracker,
@@ -365,7 +365,7 @@ public abstract class InvalidatingNodeVisitor<TGraph extends ThinNodeQueryableGr
      * passing {@code false} for {@param supportInterruptions}.
      */
     protected DirtyingNodeVisitor(
-        ThinNodeQueryableGraph graph,
+        InvalidatableGraph graph,
         EvaluationProgressReceiver invalidationReceiver,
         InvalidationState state,
         DirtyKeyTracker dirtyKeyTracker,
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index 269ea0927b..4d5d21c5de 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.skyframe;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Function;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetVisitor;
@@ -126,19 +125,18 @@ public interface MemoizingEvaluator {
 
   /**
    * Tests that want finer control over the graph being used may provide a {@code transformer} here.
-   * This {@code transformer} will be applied to the graph for each invalidation/evaluation. While
-   * the graph returned by {@code transformer#apply} must technically be a {@link ProcessableGraph},
-   * if a {@link ThinNodeQueryableGraph} was given as the argument to {@code transformer#apply},
-   * then only the methods in {@link ThinNodeQueryableGraph} will be called on the returned graph,
-   * in other words it will be treated as a {@link ThinNodeQueryableGraph}. Thus, the returned graph
-   * is free not to actually implement the remaining methods in {@link ProcessableGraph} in that
-   * case.
-   *
-   * <p>Similarly, if the argument to {@code transformer#apply} is an {@link InMemoryGraph}, then
-   * the resulting graph must be an {@link InMemoryGraph}.
-   * */
-  void injectGraphTransformerForTesting(
-      Function<ThinNodeQueryableGraph, ProcessableGraph> transformer);
+   * This {@code transformer} will be applied to the graph for each invalidation/evaluation.
+   */
+  void injectGraphTransformerForTesting(GraphTransformerForTesting transformer);
+
+  /** Transforms a graph, possibly injecting other functionality. */
+  interface GraphTransformerForTesting {
+    InMemoryGraph transform(InMemoryGraph graph);
+
+    InvalidatableGraph transform(InvalidatableGraph graph);
+
+    ProcessableGraph transform(ProcessableGraph graph);
+  }
 
   /**
    * Write the graph to the output stream. Not necessarily thread-safe. Use only for debugging
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 7b38324110..8b7b09193d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -83,6 +83,37 @@ public interface NodeEntry extends ThinNodeEntry {
   @ThreadSafe
   SkyValue getValue();
 
+  /**
+   * Returns an immutable iterable of the direct deps of this node. This method may only be called
+   * after the evaluation of this node is complete.
+   *
+   * <p>This method is not very efficient, but is only be called in limited circumstances -- when
+   * the node is about to be deleted, or when the node is expected to have no direct deps (in which
+   * case the overhead is not so bad). It should not be called repeatedly for the same node, since
+   * each call takes time proportional to the number of direct deps of the node.
+   */
+  @ThreadSafe
+  Iterable<SkyKey> getDirectDeps();
+
+  /** Removes a reverse dependency. */
+  @ThreadSafe
+  void removeReverseDep(SkyKey reverseDep);
+
+  /**
+   * Removes a reverse dependency.
+   *
+   * <p>May only be called if this entry is not done (i.e. {@link #isDone} is false) and {@param
+   * reverseDep} is present in {@link #getReverseDeps}
+   */
+  @ThreadSafe
+  void removeInProgressReverseDep(SkyKey reverseDep);
+
+  /**
+   * Returns a copy of the set of reverse dependencies. Note that this introduces a potential
+   * check-then-act race; {@link #removeReverseDep} may fail for a key that is returned here.
+   */
+  @ThreadSafe
+  Iterable<SkyKey> getReverseDeps();
 
   /**
    * Returns raw {@link SkyValue} stored in this entry, which may include metadata associated with
@@ -236,22 +267,22 @@ public interface NodeEntry extends ThinNodeEntry {
 
   /**
    * Should only be called if the entry is dirty. During the examination to see if the entry must be
-   * re-evaluated, this method returns the next group of children to be checked. Callers should
-   * have already called {@link #getDirtyState} and received a return value of
-   * {@link DirtyState#CHECK_DEPENDENCIES} before calling this method -- any other
-   * return value from {@link #getDirtyState} means that this method must not be called, since
-   * whether or not the node needs to be rebuilt is already known.
+   * re-evaluated, this method returns the next group of children to be checked. Callers should have
+   * already called {@link #getDirtyState} and received a return value of {@link
+   * DirtyState#CHECK_DEPENDENCIES} before calling this method -- any other return value from {@link
+   * #getDirtyState} means that this method must not be called, since whether or not the node needs
+   * to be rebuilt is already known.
    *
-   * <p>Deps are returned in groups. The deps in each group were requested in parallel by the
-   * {@code SkyFunction} last build, meaning independently of the values of any other deps in this
-   * group (although possibly depending on deps in earlier groups). Thus the caller may check all
-   * the deps in this group in parallel, since the deps in all previous groups are verified
-   * unchanged. See {@link SkyFunction.Environment#getValues} for more on dependency groups.
+   * <p>Deps are returned in groups. The deps in each group were requested in parallel by the {@code
+   * SkyFunction} last build, meaning independently of the values of any other deps in this group
+   * (although possibly depending on deps in earlier groups). Thus the caller may check all the deps
+   * in this group in parallel, since the deps in all previous groups are verified unchanged. See
+   * {@link SkyFunction.Environment#getValues} for more on dependency groups.
    *
    * <p>The caller should register these as deps of this entry using {@link #addTemporaryDirectDeps}
    * before checking them.
    *
-   * @see BuildingState#getNextDirtyDirectDeps()
+   * @see DirtyBuildingState#getNextDirtyDirectDeps()
    */
   @ThreadSafe
   Collection<SkyKey> getNextDirtyDirectDeps();
diff --git a/src/main/java/com/google/devtools/build/skyframe/ProcessableGraph.java b/src/main/java/com/google/devtools/build/skyframe/ProcessableGraph.java
index bc391a852f..532a151ef8 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ProcessableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ProcessableGraph.java
@@ -19,9 +19,8 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
  * A graph that is both Dirtiable (values can be deleted) and Evaluable (values can be added). All
  * methods in this interface (as inherited from super-interfaces) should be thread-safe.
  *
- * <p>This class is not intended for direct use, and is only exposed as public for use in
- * evaluation implementations outside of this package.
+ * <p>This class is not intended for direct use, and is only exposed as public for use in evaluation
+ * implementations outside of this package.
  */
 @ThreadSafe
-public interface ProcessableGraph extends DirtiableGraph, EvaluableGraph {
-}
+public interface ProcessableGraph extends DeletableGraph, EvaluableGraph {}
diff --git a/src/main/java/com/google/devtools/build/skyframe/QueryableGraph.java b/src/main/java/com/google/devtools/build/skyframe/QueryableGraph.java
index ee096996ac..04198286d8 100644
--- a/src/main/java/com/google/devtools/build/skyframe/QueryableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/QueryableGraph.java
@@ -19,16 +19,11 @@ import java.util.Map;
 
 import javax.annotation.Nullable;
 
-/**
- * A graph that exposes its entries and structure, for use by classes that must traverse it.
- */
+/** A graph that exposes its entries and structure, for use by classes that must traverse it. */
 @ThreadSafe
-public interface QueryableGraph extends ThinNodeQueryableGraph {
-  /**
-   * Returns the node with the given name, or {@code null} if the node does not exist.
-   */
+public interface QueryableGraph {
+  /** Returns the node with the given name, or {@code null} if the node does not exist. */
   @Nullable
-  @Override
   NodeEntry get(SkyKey key);
 
   /**
@@ -36,6 +31,5 @@ public interface QueryableGraph extends ThinNodeQueryableGraph {
    * {@code keys}, {@code m.get(k).equals(e)} iff {@code get(k) == e} and {@code e != null}, and
    * {@code !m.containsKey(k)} iff {@code get(k) == null}.
    */
-  @Override
   Map<SkyKey, NodeEntry> getBatch(Iterable<SkyKey> keys);
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java
index 1a5c075073..f81355c271 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ThinNodeEntry.java
@@ -32,40 +32,6 @@ public interface ThinNodeEntry {
   boolean isDone();
 
   /**
-   * Returns an immutable iterable of the direct deps of this node. This method may only be called
-   * after the evaluation of this node is complete.
-   *
-   * <p>This method is not very efficient, but is only be called in limited circumstances --
-   * when the node is about to be deleted, or when the node is expected to have no direct deps (in
-   * which case the overhead is not so bad). It should not be called repeatedly for the same node,
-   * since each call takes time proportional to the number of direct deps of the node.
-   */
-  @ThreadSafe
-  Iterable<SkyKey> getDirectDeps();
-
-  /**
-   * Removes a reverse dependency.
-   */
-  @ThreadSafe
-  void removeReverseDep(SkyKey reverseDep);
-
-  /**
-   * Removes a reverse dependency.
-   *
-   * <p>May only be called if this entry is not done (i.e. {@link #isDone} is false) and
-   * {@param reverseDep} is present in {@link #getReverseDeps}
-   */
-  @ThreadSafe
-  void removeInProgressReverseDep(SkyKey reverseDep);
-
-  /**
-   * Returns a copy of the set of reverse dependencies. Note that this introduces a potential
-   * check-then-act race; {@link #removeReverseDep} may fail for a key that is returned here.
-   */
-  @ThreadSafe
-  Iterable<SkyKey> getReverseDeps();
-
-  /**
    * Returns true if the entry is marked dirty, meaning that at least one of its transitive
    * dependencies is marked changed.
    */