Tighten some node types to reduce use of wildcards, casting, and hand…

…ling of impossible exceptions. PiperOrigin-RevId: 417704799
bazelbuild · Dec 21, 2021 · 1c95255 · 1c95255
1 parent 47f4919
commit 1c95255
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 104 deletions.
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
@@ -13,10 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
-import com.google.common.base.Predicates;
 import com.google.common.collect.Maps;
 import java.util.Collections;
 import java.util.Map;
+import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
 import javax.annotation.Nullable;
 
@@ -53,25 +53,14 @@ default Map<SkyKey, SkyValue> getDoneValues() {
   }
 
   // Only for use by MemoizingEvaluator#delete
-  Map<SkyKey, ? extends NodeEntry> getAllValues();
+  Map<SkyKey, InMemoryNodeEntry> getAllValues();
 
-  ConcurrentHashMap<SkyKey, ? extends NodeEntry> getAllValuesMutable();
+  ConcurrentHashMap<SkyKey, InMemoryNodeEntry> getAllValuesMutable();
 
-  static Map<SkyKey, SkyValue> transformDoneEntries(Map<SkyKey, ? extends NodeEntry> nodeMap) {
+  static Map<SkyKey, SkyValue> transformDoneEntries(Map<SkyKey, InMemoryNodeEntry> nodeMap) {
     return Collections.unmodifiableMap(
         Maps.filterValues(
-            Maps.transformValues(
-                nodeMap,
-                entry -> {
-                  if (!entry.isDone()) {
-                    return null;
-                  }
-                  try {
-                    return entry.getValue();
-                  } catch (InterruptedException e) {
-                    throw new IllegalStateException("Interrupted getting " + entry, e);
-                  }
-                }),
-            Predicates.notNull()));
+            Maps.transformValues(nodeMap, entry -> entry.isDone() ? entry.getValue() : null),
+            Objects::nonNull));
   }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -32,7 +32,7 @@
  * <p>This class is public only for use in alternative graph implementations.
  */
 public class InMemoryGraphImpl implements InMemoryGraph {
-  protected final ConcurrentHashMap<SkyKey, NodeEntry> nodeMap;
+  protected final ConcurrentHashMap<SkyKey, InMemoryNodeEntry> nodeMap;
   private final boolean keepEdges;
 
   @VisibleForTesting
@@ -74,7 +74,7 @@ public Map<SkyKey, NodeEntry> getBatch(
     return result;
   }
 
-  protected NodeEntry newNodeEntry(SkyKey key) {
+  protected InMemoryNodeEntry newNodeEntry(SkyKey key) {
     return keepEdges ? new InMemoryNodeEntry() : new EdgelessInMemoryNodeEntry();
   }
 
@@ -83,7 +83,7 @@ protected NodeEntry newNodeEntry(SkyKey key) {
    * lambda instantiation overhead.
    */
   @SuppressWarnings("UnnecessaryLambda")
-  private final Function<SkyKey, NodeEntry> newNodeEntryFunction = k -> newNodeEntry(k);
+  private final Function<SkyKey, InMemoryNodeEntry> newNodeEntryFunction = this::newNodeEntry;
 
   @Override
   public Map<SkyKey, NodeEntry> createIfAbsentBatch(
@@ -102,25 +102,16 @@ public DepsReport analyzeDepsDoneness(SkyKey parent, Collection<SkyKey> deps) {
 
   @Override
   public Map<SkyKey, SkyValue> getValues() {
-    return Collections.unmodifiableMap(
-        Maps.transformValues(
-            nodeMap,
-            entry -> {
-              try {
-                return entry.toValue();
-              } catch (InterruptedException e) {
-                throw new IllegalStateException(e);
-              }
-            }));
+    return Collections.unmodifiableMap(Maps.transformValues(nodeMap, InMemoryNodeEntry::toValue));
   }
 
   @Override
-  public Map<SkyKey, NodeEntry> getAllValues() {
+  public Map<SkyKey, InMemoryNodeEntry> getAllValues() {
     return Collections.unmodifiableMap(nodeMap);
   }
 
   @Override
-  public ConcurrentHashMap<SkyKey, ? extends NodeEntry> getAllValuesMutable() {
+  public ConcurrentHashMap<SkyKey, InMemoryNodeEntry> getAllValuesMutable() {
     return nodeMap;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -362,14 +362,10 @@ public void dump(boolean summarize, PrintStream out) {
     if (summarize) {
       long nodes = 0;
       long edges = 0;
-      for (NodeEntry entry : graph.getAllValues().values()) {
+      for (InMemoryNodeEntry entry : graph.getAllValues().values()) {
         nodes++;
         if (entry.isDone()) {
-          try {
-            edges += Iterables.size(entry.getDirectDeps());
-          } catch (InterruptedException e) {
-            throw new IllegalStateException("InMemoryGraph doesn't throw: " + entry, e);
-          }
+          edges += Iterables.size(entry.getDirectDeps());
         }
       }
       out.println("Node count: " + nodes);
@@ -380,21 +376,17 @@ public void dump(boolean summarize, PrintStream out) {
               String.format(
                   "%s:%s", key.functionName(), key.argument().toString().replace('\n', '_'));
 
-      for (Map.Entry<SkyKey, ? extends NodeEntry> mapPair : graph.getAllValues().entrySet()) {
+      for (Map.Entry<SkyKey, InMemoryNodeEntry> mapPair : graph.getAllValues().entrySet()) {
         SkyKey key = mapPair.getKey();
-        NodeEntry entry = mapPair.getValue();
+        InMemoryNodeEntry entry = mapPair.getValue();
         if (entry.isDone()) {
           out.print(keyFormatter.apply(key));
           out.print("|");
-          if (((InMemoryNodeEntry) entry).keepEdges() == NodeEntry.KeepEdgesPolicy.NONE) {
+          if (entry.keepEdges() == NodeEntry.KeepEdgesPolicy.NONE) {
             out.println(" (direct deps not stored)");
           } else {
-            try {
-              out.println(
-                  Joiner.on('|').join(Iterables.transform(entry.getDirectDeps(), keyFormatter)));
-            } catch (InterruptedException e) {
-              throw new IllegalStateException("InMemoryGraph doesn't throw: " + entry, e);
-            }
+            out.println(
+                Joiner.on('|').join(Iterables.transform(entry.getDirectDeps(), keyFormatter)));
           }
         }
       }

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -129,7 +129,6 @@
 import com.google.devtools.build.skyframe.InMemoryGraphImpl;
 import com.google.devtools.build.skyframe.InMemoryNodeEntry;
 import com.google.devtools.build.skyframe.MemoizingEvaluator.GraphTransformerForTesting;
-import com.google.devtools.build.skyframe.NodeEntry;
 import com.google.devtools.build.skyframe.NotifyingHelper;
 import com.google.devtools.build.skyframe.NotifyingHelper.EventType;
 import com.google.devtools.build.skyframe.ProcessableGraph;
@@ -364,11 +363,11 @@ private static GraphTransformerForTesting inMemoryGraphWithValues(
     return new GraphTransformerForTesting() {
       @Override
       public InMemoryGraph transform(InMemoryGraph graph) {
-        return new InMemoryGraphImpl() {
-          {
-            nodeMap.putAll(Maps.transformValues(values, v -> createNodeEntry(v)));
-          }
-        };
+        InMemoryGraph transformed = new InMemoryGraphImpl();
+        transformed
+            .getAllValuesMutable()
+            .putAll(Maps.transformValues(values, this::createNodeEntry));
+        return transformed;
       }
 
       @Override
@@ -381,7 +380,7 @@ public ProcessableGraph transform(ProcessableGraph graph) {
         throw new UnsupportedOperationException();
       }
 
-      private NodeEntry createNodeEntry(SkyValue value) {
+      private InMemoryNodeEntry createNodeEntry(SkyValue value) {
         InMemoryNodeEntry nodeEntry = new InMemoryNodeEntry();
         nodeEntry.addReverseDepAndCheckIfDone(null);
         nodeEntry.markRebuilding();

diff --git a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
@@ -64,12 +64,12 @@ public Map<SkyKey, SkyValue> getValues() {
   }
 
   @Override
-  public Map<SkyKey, ? extends NodeEntry> getAllValues() {
+  public Map<SkyKey, InMemoryNodeEntry> getAllValues() {
     return ((InMemoryGraph) delegate).getAllValues();
   }
 
   @Override
-  public ConcurrentHashMap<SkyKey, ? extends NodeEntry> getAllValuesMutable() {
+  public ConcurrentHashMap<SkyKey, InMemoryNodeEntry> getAllValuesMutable() {
     return ((InMemoryGraph) delegate).getAllValuesMutable();
   }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
@@ -71,12 +71,12 @@ public int valuesSize() {
   }
 
   @Override
-  public Map<SkyKey, ? extends NodeEntry> getAllValues() {
+  public Map<SkyKey, InMemoryNodeEntry> getAllValues() {
     return ((InMemoryGraph) delegate).getAllValues();
   }
 
   @Override
-  public ConcurrentHashMap<SkyKey, ? extends NodeEntry> getAllValuesMutable() {
+  public ConcurrentHashMap<SkyKey, InMemoryNodeEntry> getAllValuesMutable() {
     return ((InMemoryGraph) delegate).getAllValuesMutable();
   }
 }