Tighten some node types to reduce use of wildcards, casting, and handling of impossible exceptions.
PiperOrigin-RevId: 417704799
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 968d360..e4e423e 100644
--- 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 @@
}
// 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
index ea16a9a..b6b0fcb 100644
--- 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 @@
return result;
}
- protected NodeEntry newNodeEntry(SkyKey key) {
+ protected InMemoryNodeEntry newNodeEntry(SkyKey key) {
return keepEdges ? new InMemoryNodeEntry() : new EdgelessInMemoryNodeEntry();
}
@@ -83,7 +83,7 @@
* 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 @@
@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
index b471603..5798910 100644
--- 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 @@
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 @@
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
index 44a2f36..434f6ed 100644
--- 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 @@
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 @@
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
index 41e3203..39b9169 100644
--- 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 @@
}
@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
index 53d110e..2a7ad76 100644
--- 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 @@
}
@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/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
index f63cbb2..57c9700 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -22,6 +22,9 @@
import static com.google.devtools.build.skyframe.GraphTester.skyKey;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
@@ -76,7 +79,6 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.mockito.Mockito;
/** Tests for {@link ParallelEvaluator}. */
@RunWith(TestParameterInjector.class)
@@ -205,7 +207,7 @@
private ListenableFuture<SkyValue> future;
@Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+ public SkyValue compute(SkyKey skyKey, Environment env) {
if (future == null) {
future =
executor.submit(
@@ -364,9 +366,9 @@
// thread, aka the main Skyframe evaluation thread),
CountDownLatch keyAStartedComputingLatch = new CountDownLatch(1);
CountDownLatch keyBAddReverseDepAndCheckIfDoneLatch = new CountDownLatch(1);
- NodeEntry nodeEntryB = Mockito.mock(NodeEntry.class);
+ InMemoryNodeEntry nodeEntryB = mock(InMemoryNodeEntry.class);
AtomicBoolean keyBAddReverseDepAndCheckIfDoneInterrupted = new AtomicBoolean(false);
- Mockito.doAnswer(
+ doAnswer(
invocation -> {
keyAStartedComputingLatch.await();
keyBAddReverseDepAndCheckIfDoneLatch.countDown();
@@ -379,11 +381,11 @@
}
})
.when(nodeEntryB)
- .addReverseDepAndCheckIfDone(Mockito.eq(null));
+ .addReverseDepAndCheckIfDone(eq(null));
graph =
new InMemoryGraphImpl() {
@Override
- protected NodeEntry newNodeEntry(SkyKey key) {
+ protected InMemoryNodeEntry newNodeEntry(SkyKey key) {
return key.equals(keyB) ? nodeEntryB : super.newNodeEntry(key);
}
};
@@ -579,17 +581,16 @@
class CustomRuntimeException extends RuntimeException {}
final CustomRuntimeException expected = new CustomRuntimeException();
- final SkyFunction builder =
+ SkyFunction builder =
new SkyFunction() {
@Override
@Nullable
- public SkyValue compute(SkyKey skyKey, Environment env)
- throws SkyFunctionException, InterruptedException {
+ public SkyValue compute(SkyKey skyKey, Environment env) {
throw expected;
}
};
- final ParallelEvaluator evaluator =
+ ParallelEvaluator evaluator =
makeEvaluator(
new InMemoryGraphImpl(), ImmutableMap.of(GraphTester.NODE_TYPE, builder), false);
@@ -895,10 +896,6 @@
} catch (SomeErrorException e) {
throw new SkyFunctionException(
new SomeErrorException("We got: " + e.getMessage()), Transience.PERSISTENT) {
- @Override
- public boolean isCatastrophic() {
- return false;
- }
};
}
return null;
@@ -1084,12 +1081,7 @@
.getOrCreate(errorKey)
.setBuilder(
new ChainedFunction(
- null,
- /*waitToFinish=*/ latch,
- null,
- false,
- /*value=*/ null,
- ImmutableList.<SkyKey>of()));
+ null, /*waitToFinish=*/ latch, null, false, /*value=*/ null, ImmutableList.of()));
tester.getOrCreate(parentKey).addDependency(errorKey).setComputedValue(CONCATENATE);
EvaluationResult<StringValue> result =
eval(/*keepGoing=*/ false, ImmutableList.of(parentKey, errorKey));
@@ -1129,7 +1121,7 @@
/*notifyFinish=*/ null,
/*waitForException=*/ false,
/*value=*/ null,
- ImmutableList.<SkyKey>of()));
+ ImmutableList.of()));
tester
.getOrCreate(secondError)
.setBuilder(
@@ -1139,7 +1131,7 @@
/*notifyFinish=*/ null,
/*waitForException=*/ false,
/*value=*/ null,
- ImmutableList.<SkyKey>of()));
+ ImmutableList.of()));
EvaluationResult<StringValue> result = eval(/*keepGoing=*/ false, firstError, secondError);
assertWithMessage(result.toString()).that(result.hasError()).isTrue();
// With keepGoing=false, the eval call will terminate with exactly one error (the first one
@@ -1599,7 +1591,7 @@
.addDependency("after")
.setComputedValue(CONCATENATE);
EvaluationResult<StringValue> result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey));
- assertThat(ImmutableList.<String>copyOf(result.<String>keyNames())).containsExactly("top");
+ assertThat(ImmutableList.<String>copyOf(result.keyNames())).containsExactly("top");
assertThat(result.get(topKey).getValue()).isEqualTo("parent valueafter");
assertThat(result.errorMap()).isEmpty();
}
@@ -1726,22 +1718,12 @@
.getOrCreate(cycleKey)
.setBuilder(
new ChainedFunction(
- null,
- null,
- cycleFinish,
- false,
- new StringValue(""),
- ImmutableSet.<SkyKey>of(midKey)));
+ null, null, cycleFinish, false, new StringValue(""), ImmutableSet.of(midKey)));
tester
.getOrCreate(errorKey)
.setBuilder(
new ChainedFunction(
- null,
- cycleFinish,
- null,
- /*waitForException=*/ false,
- null,
- ImmutableSet.<SkyKey>of()));
+ null, cycleFinish, null, /*waitForException=*/ false, null, ImmutableSet.of()));
EvaluationResult<StringValue> result = eval(keepGoing, ImmutableSet.of(topKey));
assertThatEvaluationResult(result)
@@ -1796,7 +1778,7 @@
null,
/*waitForException=*/ true,
new StringValue("never returned"),
- ImmutableSet.<SkyKey>of(GraphTester.toSkyKey("dep that never builds"))));
+ ImmutableSet.of(GraphTester.toSkyKey("dep that never builds"))));
tester
.getOrCreate(cycleKey)
@@ -1807,7 +1789,7 @@
topStartAndCycleFinish,
/*waitForException=*/ false,
new StringValue(""),
- ImmutableSet.<SkyKey>of(midKey)));
+ ImmutableSet.of(midKey)));
// error waits until otherTop starts and cycle finishes, to make sure otherTop will request
// its dep before the threadpool shuts down.
tester
@@ -1819,7 +1801,7 @@
null,
/*waitForException=*/ false,
null,
- ImmutableSet.<SkyKey>of()));
+ ImmutableSet.of()));
EvaluationResult<StringValue> result =
eval(/*keepGoing=*/ false, ImmutableSet.of(topKey, otherTop));
assertThat(result.errorMap().keySet()).containsExactly(topKey);
@@ -1862,7 +1844,7 @@
null,
/*waitForException=*/ !keepGoing,
null,
- ImmutableSet.<SkyKey>of()));
+ ImmutableSet.of()));
// error waits until otherTop starts and cycle finishes, to make sure otherTop will request
// its dep before the threadpool shuts down.
tester
@@ -1874,7 +1856,7 @@
null,
/*waitForException=*/ false,
null,
- ImmutableSet.<SkyKey>of()));
+ ImmutableSet.of()));
tester
.getOrCreate(cycleKey)
.setBuilder(
@@ -1884,7 +1866,7 @@
topStartAndCycleFinish,
/*waitForException=*/ false,
new StringValue(""),
- ImmutableSet.<SkyKey>of(midKey)));
+ ImmutableSet.of(midKey)));
EvaluationResult<StringValue> result = eval(keepGoing, ImmutableSet.of(topKey, otherTop));
if (keepGoing) {
assertThatEvaluationResult(result).hasErrorMapThat().hasSize(2);
@@ -1945,8 +1927,8 @@
+ "(requested by nodes 'parent:octodad')");
}
- private static class SomeOtherErrorException extends Exception {
- public SomeOtherErrorException(String msg) {
+ private static final class SomeOtherErrorException extends Exception {
+ SomeOtherErrorException(String msg) {
super(msg);
}
}
@@ -2205,7 +2187,7 @@
// Skyframe doesn't automatically dedupe cycles that are the same except for entry point.
assertThat(cycles).hasSize(2);
int numUniqueCycles = 0;
- CycleDeduper<SkyKey> cycleDeduper = new CycleDeduper<SkyKey>();
+ CycleDeduper<SkyKey> cycleDeduper = new CycleDeduper<>();
for (ImmutableList<SkyKey> cycle : cycles) {
if (!cycleDeduper.alreadySeen(cycle)) {
numUniqueCycles++;
@@ -2216,8 +2198,8 @@
@Test
public void signalValueEnqueuedAndEvaluated() throws Exception {
- final Set<SkyKey> enqueuedValues = Sets.newConcurrentHashSet();
- final Set<SkyKey> evaluatedValues = Sets.newConcurrentHashSet();
+ Set<SkyKey> enqueuedValues = Sets.newConcurrentHashSet();
+ Set<SkyKey> evaluatedValues = Sets.newConcurrentHashSet();
EvaluationProgressReceiver progressReceiver =
new EvaluationProgressReceiver() {
@Override
