Delete NodeEntryField since it's now superfluous in the presence of the new QueryableGraph.Reason which conveys more information. Add a few more Reason enum values to make this refactor benign.

--
MOS_MIGRATED_REVID=129118462

7 files changed, 55 insertions, 101 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java b/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java
index b6c582559e..c56bf55132 100644
--- a/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.skyframe.QueryableGraph.Reason;
-import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -36,11 +35,10 @@ public class DelegatingWalkableGraph implements WalkableGraph {
     this.graph = graph;
   }
 
-  private NodeEntry getEntry(SkyKey key) {
+  private NodeEntry getEntryForValue(SkyKey key) {
     NodeEntry entry =
         Preconditions.checkNotNull(
-            graph.getBatchWithFieldHints(
-                null, Reason.OTHER, ImmutableList.of(key), NodeEntryField.VALUE_ONLY).get(key),
+            graph.getBatch(null, Reason.WALKABLE_GRAPH_VALUE, ImmutableList.of(key)).get(key),
             key);
     Preconditions.checkState(entry.isDone(), "%s %s", key, entry);
     return entry;
@@ -49,15 +47,14 @@ public class DelegatingWalkableGraph implements WalkableGraph {
   @Override
   public boolean exists(SkyKey key) {
     NodeEntry entry =
-        graph.getBatchWithFieldHints(
-            null, Reason.OTHER, ImmutableList.of(key), NodeEntryField.NO_FIELDS).get(key);
+        graph.getBatch(null, Reason.EXISTENCE_CHECKING, ImmutableList.of(key)).get(key);
     return entry != null && entry.isDone();
   }
 
   @Nullable
   @Override
   public SkyValue getValue(SkyKey key) {
-    return getEntry(key).getValue();
+    return getEntryForValue(key).getValue();
   }
 
   private static final Function<NodeEntry, SkyValue> GET_SKY_VALUE_FUNCTION =
@@ -73,7 +70,7 @@ public class DelegatingWalkableGraph implements WalkableGraph {
   public Map<SkyKey, SkyValue> getSuccessfulValues(Iterable<SkyKey> keys) {
     return Maps.filterValues(
         Maps.transformValues(
-            graph.getBatchWithFieldHints(null, Reason.OTHER, keys, NodeEntryField.VALUE_ONLY),
+            graph.getBatch(null, Reason.WALKABLE_GRAPH_VALUE, keys),
             GET_SKY_VALUE_FUNCTION),
         Predicates.notNull());
   }
@@ -81,8 +78,7 @@ public class DelegatingWalkableGraph implements WalkableGraph {
   @Override
   public Map<SkyKey, Exception> getMissingAndExceptions(Iterable<SkyKey> keys) {
     Map<SkyKey, Exception> result = new HashMap<>();
-    Map<SkyKey, NodeEntry> graphResult =
-        graph.getBatchWithFieldHints(null, Reason.OTHER, keys, NodeEntryField.VALUE_ONLY);
+    Map<SkyKey, NodeEntry> graphResult = graph.getBatch(null, Reason.WALKABLE_GRAPH_VALUE, keys);
     for (SkyKey key : keys) {
       NodeEntry nodeEntry = graphResult.get(key);
       if (nodeEntry == null || !nodeEntry.isDone()) {
@@ -100,14 +96,13 @@ public class DelegatingWalkableGraph implements WalkableGraph {
   @Nullable
   @Override
   public Exception getException(SkyKey key) {
-    ErrorInfo errorInfo = getEntry(key).getErrorInfo();
+    ErrorInfo errorInfo = getEntryForValue(key).getErrorInfo();
     return errorInfo == null ? null : errorInfo.getException();
   }
 
   @Override
   public Map<SkyKey, Iterable<SkyKey>> getDirectDeps(Iterable<SkyKey> keys) {
-    Map<SkyKey, NodeEntry> entries = graph.getBatchWithFieldHints(
-        null, Reason.OTHER, keys, EnumSet.of(NodeEntryField.DIRECT_DEPS));
+    Map<SkyKey, NodeEntry> entries = graph.getBatch(null, Reason.WALKABLE_GRAPH_DEPS, keys);
     Map<SkyKey, Iterable<SkyKey>> result = new HashMap<>(entries.size());
     for (Entry<SkyKey, NodeEntry> entry : entries.entrySet()) {
       Preconditions.checkState(entry.getValue().isDone(), entry);
@@ -118,8 +113,7 @@ public class DelegatingWalkableGraph implements WalkableGraph {
 
   @Override
   public Map<SkyKey, Iterable<SkyKey>> getReverseDeps(Iterable<SkyKey> keys) {
-    Map<SkyKey, NodeEntry> entries = graph.getBatchWithFieldHints(
-        null, Reason.OTHER, keys, EnumSet.of(NodeEntryField.REVERSE_DEPS));
+    Map<SkyKey, NodeEntry> entries = graph.getBatch(null, Reason.WALKABLE_GRAPH_RDEPS, keys);
     Map<SkyKey, Iterable<SkyKey>> result = new HashMap<>(entries.size());
     for (Entry<SkyKey, NodeEntry> entry : entries.entrySet()) {
       Preconditions.checkState(entry.getValue().isDone(), entry);
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 0db7daddf9..6e93ce4ce3 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EvaluableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EvaluableGraph.java
@@ -24,7 +24,7 @@ import javax.annotation.Nullable;
 @ThreadSafe
 interface EvaluableGraph extends QueryableGraph, DeletableGraph {
   /**
-   * Like {@link QueryableGraph#getBatchWithFieldHints}, except it creates a new node for each key
+   * 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/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
index e068c3f4ad..ff5ce57b69 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.MapMaker;
 import com.google.common.collect.Maps;
 import java.util.Collections;
-import java.util.EnumSet;
 import java.util.Map;
 import java.util.concurrent.ConcurrentMap;
 import javax.annotation.Nullable;
@@ -68,8 +67,7 @@ public class InMemoryGraphImpl implements InMemoryGraph {
   }
 
   @Override
-  public Map<SkyKey, NodeEntry> getBatchWithFieldHints(
-      SkyKey requestor, Reason reason, Iterable<SkyKey> keys, EnumSet<NodeEntryField> fields) {
+  public Map<SkyKey, NodeEntry> getBatch(SkyKey requestor, Reason reason, Iterable<SkyKey> keys) {
     return getBatchForInvalidation(keys);
   }
 
diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntryField.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntryField.java
deleted file mode 100644
index 97c1dfd87c..0000000000
--- a/src/main/java/com/google/devtools/build/skyframe/NodeEntryField.java
+++ /dev/null
@@ -1,41 +0,0 @@
-// Copyright 2016 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.skyframe;
-
-import java.util.EnumSet;
-
-/**
- * Fields of a {@link NodeEntry} that clients of a {@link QueryableGraph} may need. Clients may
- * specify these fields in {@link QueryableGraph#getBatchWithFieldHints} to help particular {@link
- * QueryableGraph} implementations decide how lazily to construct the returned node entries.
- */
-public enum NodeEntryField {
-  /** The value ({@link NodeEntry#getValueMaybeWithMetadata}) will be needed. */
-  VALUE,
-  /** The direct deps ({@link NodeEntry#getDirectDeps}) will be needed. */
-  DIRECT_DEPS,
-  /** The reverse deps ({@link NodeEntry#getReverseDeps}) will be needed. */
-  REVERSE_DEPS,
-  /**
-   * The reverse deps as a whole will not be needed, but we may need to check the presence of a
-   * reverse dep or add/delete one.
-   */
-  INDIVIDUAL_REVERSE_DEPS;
-
-  public static final EnumSet<NodeEntryField> NO_FIELDS = EnumSet.noneOf(NodeEntryField.class);
-  public static final EnumSet<NodeEntryField> VALUE_ONLY = EnumSet.of(VALUE);
-  public static final EnumSet<NodeEntryField> NO_VALUE = EnumSet.of(DIRECT_DEPS, REVERSE_DEPS);
-  public static final EnumSet<NodeEntryField> ALL_FIELDS =
-      EnumSet.of(VALUE, DIRECT_DEPS, REVERSE_DEPS);
-}
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index 61c93cb757..ea83d875cc 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -57,7 +57,6 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Deque;
-import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -227,7 +226,7 @@ public final class ParallelEvaluator implements Evaluator {
       SkyKey parent,
       Reason reason,
       Iterable<SkyKey> keys) {
-    return graph.getBatchWithFieldHints(parent, reason, keys, NodeEntryField.VALUE_ONLY);
+    return graph.getBatch(parent, reason, keys);
   }
 
   /** Receives the events from the NestedSet and delegates to the reporter. */
@@ -667,11 +666,10 @@ public final class ParallelEvaluator implements Evaluator {
         // Remove the rdep on this entry for each of its old deps that is no longer a direct dep.
         Set<SkyKey> depsToRemove =
             Sets.difference(oldDeps, primaryEntry.getTemporaryDirectDeps().toSet());
-        Collection<NodeEntry> oldDepEntries = graph.getBatchWithFieldHints(
+        Collection<NodeEntry> oldDepEntries = graph.getBatch(
             skyKey,
             Reason.RDEP_REMOVAL,
-            depsToRemove,
-            EnumSet.of(NodeEntryField.INDIVIDUAL_REVERSE_DEPS)).values();
+            depsToRemove).values();
         for (NodeEntry oldDepEntry : oldDepEntries) {
           oldDepEntry.removeReverseDep(skyKey);
         }
@@ -935,11 +933,7 @@ public final class ParallelEvaluator implements Evaluator {
             // We check the deps for errors so that we don't continue building this node if it has
             // a child error.
             Map<SkyKey, NodeEntry> entriesToCheck =
-                graph.getBatchWithFieldHints(
-                    skyKey,
-                    Reason.EVALUATION,
-                    directDepsToCheck,
-                    EnumSet.of(NodeEntryField.VALUE, NodeEntryField.INDIVIDUAL_REVERSE_DEPS));
+                graph.getBatch(skyKey, Reason.OTHER, directDepsToCheck);
             for (Map.Entry<SkyKey, NodeEntry> entry : entriesToCheck.entrySet()) {
               if (entry.getValue().isDone() && entry.getValue().getErrorInfo() != null) {
                 // If any child has an error, we arbitrarily add a dep on the first one (needed
@@ -1111,11 +1105,7 @@ public final class ParallelEvaluator implements Evaluator {
         registerNewlyDiscoveredDepsForDoneEntry(
             skyKey,
             state,
-            graph.getBatchWithFieldHints(
-                skyKey,
-                Reason.RDEP_ADDITION,
-                env.newlyRequestedDeps,
-                EnumSet.of(NodeEntryField.INDIVIDUAL_REVERSE_DEPS)),
+            graph.getBatch(skyKey, Reason.RDEP_ADDITION, env.newlyRequestedDeps),
             oldDeps,
             env);
         env.commit(state, /*enqueueParents=*/true);
@@ -1241,7 +1231,7 @@ public final class ParallelEvaluator implements Evaluator {
     // importantly, these hints are not respected for not-done nodes. If they are, we may need to
     // alter this hint.
     Map<SkyKey, NodeEntry> batch =
-        graph.getBatchWithFieldHints(skyKey, Reason.SIGNAL_DEP, keys, NodeEntryField.NO_FIELDS);
+        graph.getBatch(skyKey, Reason.SIGNAL_DEP, keys);
     if (visitor != null) {
       for (SkyKey key : keys) {
         NodeEntry entry = Preconditions.checkNotNull(batch.get(key), key);
@@ -1339,7 +1329,7 @@ public final class ParallelEvaluator implements Evaluator {
     // directly without launching the heavy machinery, spawning threads, etc.
     // Inform progressReceiver that these nodes are done to be consistent with the main code path.
     boolean allAreDone = true;
-    Map<SkyKey, NodeEntry> batch = getBatchValues(null, Reason.EVALUATION, skyKeySet);
+    Map<SkyKey, NodeEntry> batch = getBatchValues(null, Reason.PRE_OR_POST_EVALUATION, skyKeySet);
     for (SkyKey key : skyKeySet) {
       if (!isDoneForBuild(batch.get(key))) {
         allAreDone = false;
@@ -1358,7 +1348,7 @@ public final class ParallelEvaluator implements Evaluator {
     if (!keepGoing) {
       Set<SkyKey> cachedErrorKeys = new HashSet<>();
       for (SkyKey skyKey : skyKeySet) {
-        NodeEntry entry = graph.get(null, Reason.EVALUATION, skyKey);
+        NodeEntry entry = graph.get(null, Reason.PRE_OR_POST_EVALUATION, skyKey);
         if (entry == null) {
           continue;
         }
@@ -1402,9 +1392,10 @@ public final class ParallelEvaluator implements Evaluator {
     // We unconditionally add the ErrorTransienceValue here, to ensure that it will be created, and
     // in the graph, by the time that it is needed. Creating it on demand in a parallel context sets
     // up a race condition, because there is no way to atomically create a node and set its value.
-    NodeEntry errorTransienceEntry = Iterables.getOnlyElement(
-        graph.createIfAbsentBatch(
-            null, Reason.EVALUATION, ImmutableList.of(ErrorTransienceValue.KEY)).values());
+    NodeEntry errorTransienceEntry = Iterables.getOnlyElement(graph.createIfAbsentBatch(
+            null,
+            Reason.PRE_OR_POST_EVALUATION,
+            ImmutableList.of(ErrorTransienceValue.KEY)).values());
     if (!errorTransienceEntry.isDone()) {
       injectValues(
           ImmutableMap.of(ErrorTransienceValue.KEY, (SkyValue) ErrorTransienceValue.INSTANCE),
@@ -1413,7 +1404,7 @@ public final class ParallelEvaluator implements Evaluator {
           dirtyKeyTracker);
     }
     for (Map.Entry<SkyKey, NodeEntry> e
-        : graph.createIfAbsentBatch(null, Reason.EVALUATION, skyKeys).entrySet()) {
+        : graph.createIfAbsentBatch(null, Reason.PRE_OR_POST_EVALUATION, skyKeys).entrySet()) {
       SkyKey skyKey = e.getKey();
       NodeEntry entry = e.getValue();
       // This must be equivalent to the code in enqueueChild above, in order to be thread-safe.
@@ -1686,7 +1677,7 @@ public final class ParallelEvaluator implements Evaluator {
     for (SkyKey skyKey : skyKeys) {
       SkyValue unwrappedValue = maybeGetValueFromError(
           skyKey,
-          graph.get(null, Reason.EVALUATION, skyKey),
+          graph.get(null, Reason.PRE_OR_POST_EVALUATION, skyKey),
           bubbleErrorInfo);
       ValueWithMetadata valueWithMetadata =
           unwrappedValue == NULL_MARKER ? null : ValueWithMetadata.wrapWithMetadata(unwrappedValue);
@@ -1937,8 +1928,8 @@ public final class ParallelEvaluator implements Evaluator {
       // out.
       // TODO(janakr): If graph implementations start using these hints for not-done nodes, we may
       // have to change this.
-      Map<SkyKey, NodeEntry> childrenNodes = graph.getBatchWithFieldHints(
-          key, Reason.CYCLE_CHECKING, children, NodeEntryField.NO_FIELDS);
+      Map<SkyKey, NodeEntry> childrenNodes =
+          graph.getBatch(key, Reason.EXISTENCE_CHECKING, children);
       Preconditions.checkState(childrenNodes.size() == Iterables.size(children), childrenNodes);
       children = Maps.filterValues(childrenNodes, new Predicate<NodeEntry>() {
         @Override
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 75ce05c970..f76c0aeeb7 100644
--- a/src/main/java/com/google/devtools/build/skyframe/QueryableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/QueryableGraph.java
@@ -14,8 +14,6 @@
 package com.google.devtools.build.skyframe;
 
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-
-import java.util.EnumSet;
 import java.util.Map;
 
 import javax.annotation.Nullable;
@@ -36,20 +34,13 @@ public interface QueryableGraph {
   /**
    * Fetches all the given 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 {@code
-   * !m.containsKey(k)} iff {@code get(k) == null}. The {@code fields} parameter is a hint to the
-   * QueryableGraph implementation that allows it to possibly construct certain fields of the
-   * returned node entries more lazily. Hints may only be applied to nodes in a certain state, like
-   * done nodes.
+   * !m.containsKey(k)} iff {@code get(k) == null}.
    * 
    * @param requestor if non-{@code null}, the node on behalf of which the given {@code keys} are
    *     being requested.
    * @param reason the reason the nodes are being requested.
    */
-  Map<SkyKey, NodeEntry> getBatchWithFieldHints(
-      @Nullable SkyKey requestor,
-      Reason reason,
-      Iterable<SkyKey> keys,
-      EnumSet<NodeEntryField> fields);
+  Map<SkyKey, NodeEntry> getBatch(@Nullable SkyKey requestor, Reason reason, Iterable<SkyKey> keys);
 
   /**
    * The reason that a node is being looked up in the Skyframe graph.
@@ -58,14 +49,20 @@ public interface QueryableGraph {
    */
   enum Reason {
     /**
+     * The node is being fetched in order to see if it needs to be evaluated or because it was just
+     * evaluated, but *not* because it was just requested during evaluation of a SkyFunction
+     * (see {@link #DEP_REQUESTED}).
+     */
+    PRE_OR_POST_EVALUATION,
+
+    /**
      * The node is being looked up as part of the prefetch step before evaluation of a SkyFunction.
      */
     PREFETCH,
 
     /**
-     * The node is being fetched because it is about to be evaluated or it has already been
-     * evaluated, but *not* because it was just requested during evaluation of a SkyFunction (see
-     * DEP_REQUESTED).
+     * The node is being fetched because it is about to be evaluated, but *not* because it was just
+     * requested during evaluation of a SkyFunction (see {@link #DEP_REQUESTED}).
      */
     EVALUATION,
 
@@ -98,6 +95,22 @@ public interface QueryableGraph {
      */
     ERROR_BUBBLING,
 
+    /** The node is being looked up merely for an existence check. */
+    EXISTENCE_CHECKING,
+
+    /**
+     * The node is being looked up to service {@link WalkableGraph#getValue},
+     * {@link WalkableGraph#getException}, {@link WalkableGraph#getMissingAndExceptions}, or
+     * {@link WalkableGraph#getSuccessfulValues}.
+     */
+    WALKABLE_GRAPH_VALUE,
+
+    /** The node is being looked up to service {@link WalkableGraph#getDirectDeps}. */
+    WALKABLE_GRAPH_DEPS,
+
+    /** The node is being looked up to service {@link WalkableGraph#getReverseDeps}. */
+    WALKABLE_GRAPH_RDEPS,
+
     /** Some other reason than one of the above. */
     OTHER,
   }
diff --git a/src/main/java/com/google/devtools/build/skyframe/QueryableGraphBackedSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/QueryableGraphBackedSkyFunctionEnvironment.java
index 6aaadcd80a..79ed64056a 100644
--- a/src/main/java/com/google/devtools/build/skyframe/QueryableGraphBackedSkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/QueryableGraphBackedSkyFunctionEnvironment.java
@@ -70,8 +70,7 @@ public class QueryableGraphBackedSkyFunctionEnvironment extends AbstractSkyFunct
 
   @Override
   protected Map<SkyKey, ValueOrUntypedException> getValueOrUntypedExceptions(Set<SkyKey> depKeys) {
-    Map<SkyKey, NodeEntry> resultMap = queryableGraph.getBatchWithFieldHints(
-        null, Reason.DEP_REQUESTED, depKeys, NodeEntryField.VALUE_ONLY);
+    Map<SkyKey, NodeEntry> resultMap = queryableGraph.getBatch(null, Reason.DEP_REQUESTED, depKeys);
     Map<SkyKey, NodeEntry> resultWithMissingKeys = new HashMap<>(resultMap);
     for (SkyKey missingDep : Sets.difference(depKeys, resultMap.keySet())) {
       resultWithMissingKeys.put(missingDep, null);