Refactor QueryableGraph and ThinNodeQueryableGraph to be independent interfaces, in preparation for further changes. -- MOS_MIGRATED_REVID=126924789
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 8e9ddb6..d714924 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 @@ @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 similarity index 82% rename from src/main/java/com/google/devtools/build/skyframe/DirtiableGraph.java rename to src/main/java/com/google/devtools/build/skyframe/DeletableGraph.java index fe7e88b..d091f08 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 @@ * 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 bb6d2b2..b7c1ee9 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 @@ * 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 @@ @Nullable static DeletingNodeVisitor createDeletingVisitorIfNeeded( - DirtiableGraph graph, + InMemoryGraph graph, Iterable<SkyKey> diff, EvaluationProgressReceiver invalidationReceiver, InvalidationState state, @@ -70,7 +75,7 @@ @Nullable static DirtyingNodeVisitor createInvalidatingVisitorIfNeeded( - ThinNodeQueryableGraph graph, + InvalidatableGraph graph, Iterable<SkyKey> diff, EvaluationProgressReceiver invalidationReceiver, InvalidationState state, @@ -84,7 +89,7 @@ @Nullable private static DirtyingNodeVisitor createInvalidatingVisitorIfNeeded( - ThinNodeQueryableGraph graph, + InvalidatableGraph graph, Iterable<SkyKey> diff, EvaluationProgressReceiver invalidationReceiver, InvalidationState state, @@ -110,7 +115,7 @@ * 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 @@ * the provided {@link ForkJoinPool}. */ public static void invalidate( - ThinNodeQueryableGraph graph, + InvalidatableGraph graph, Iterable<SkyKey> diff, EvaluationProgressReceiver invalidationReceiver, InvalidationState state, @@ -154,11 +159,9 @@ } } - /** - * 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 7527146..b3d8bd0 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 @@ * 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 02735c0..5d508a9 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 @@ 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 a5e22221..5d853b7 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 @@ } @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 similarity index 78% rename from src/main/java/com/google/devtools/build/skyframe/ThinNodeQueryableGraph.java rename to src/main/java/com/google/devtools/build/skyframe/InvalidatableGraph.java index eeb3ef5..2437407 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 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 6eec236..1fe90d1 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 @@ * * <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 @@ } /** 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 @@ } /** 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 @@ private final boolean supportInterruptions; protected DirtyingNodeVisitor( - ThinNodeQueryableGraph graph, + InvalidatableGraph graph, EvaluationProgressReceiver invalidationReceiver, InvalidationState state, DirtyKeyTracker dirtyKeyTracker, @@ -365,7 +365,7 @@ * 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 269ea09..4d5d21c 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 @@ /** * 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 7b38324..8b7b091 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 @@ @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 @@ /** * 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 bc391a8..532a151 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 @@ * 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 ee09699..0419828 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 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 @@ * {@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 1a5c075..f81355c 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 @@ 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. */