Let `SkyKey` alone declare its value's shareability.
There are currently three methods for declaring shareability:
* `SkyFunctionName#getShareabilityOfValue`
* `SkyKey#getShareabilityOfValue`
* `SkyValue#dataIsShareable`
The former two are just "hints" - a return of `SOMETIMES` still requires checking the value. However, there is no enforcement of consistency - for example, we can have a particular `SkyFunctionName#getShareabilityOfValue` return `NEVER`, but that function may compute a value whose `SkyValue#dataIsShareable` returns `true`. This currently happens in practice.
My original plan was to check consistency in `SerializationCheckingGraph`, but it turns out that it's not too difficult to just make `SkyKey` the sole proprietor of shareability. This is strictly better than giving the responsibility to `SkyValue` because a remote storage fetch for a value need not be attempted if the key is not shareable (this is what the "hint" in `SkyKey` intended to achieve).
Replace `ShareabilityOfValue` with a simple boolean since the return of `SkyKey#valueIsShareable` is now definite.
PiperOrigin-RevId: 416937942
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 37edcb0..a727496 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -87,23 +87,23 @@
delegateActionExecutionFunction = new SimpleActionExecutionFunction(omittedOutputs);
}
- private void assertFileArtifactValueMatches(boolean expectDigest) throws Throwable {
+ private void assertFileArtifactValueMatches() throws Exception {
Artifact output = createDerivedArtifact("output");
Path path = output.getPath();
file(path, "contents");
- assertValueMatches(path.stat(), expectDigest ? path.getDigest() : null, evaluateFAN(output));
+ assertValueMatches(path.stat(), path.getDigest(), evaluateFAN(output));
}
@Test
- public void testBasicArtifact() throws Throwable {
+ public void testBasicArtifact() throws Exception {
fastDigest = false;
- assertFileArtifactValueMatches(/*expectDigest=*/ true);
+ assertFileArtifactValueMatches();
}
@Test
- public void testBasicArtifactWithXattr() throws Throwable {
+ public void testBasicArtifactWithXattr() throws Exception {
fastDigest = true;
- assertFileArtifactValueMatches(/*expectDigest=*/ true);
+ assertFileArtifactValueMatches();
}
@Test
@@ -335,12 +335,10 @@
FilesetOutputSymlink.createForTesting(
PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT);
ActionExecutionValue actionExecutionValue =
- ActionExecutionValue.create(
+ ActionExecutionValue.createForTesting(
ImmutableMap.of(artifact1, metadata1, artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableMap.of(treeArtifact, tree),
- ImmutableList.of(filesetOutputSymlink),
- /*discoveredModules=*/ null,
- /*shareable=*/ false);
+ ImmutableList.of(filesetOutputSymlink));
new SerializationTester(actionExecutionValue)
.addDependency(FileSystem.class, root.getFileSystem())
.addDependency(
@@ -430,11 +428,11 @@
}
}
- private FileArtifactValue evaluateFAN(Artifact artifact) throws Throwable {
+ private FileArtifactValue evaluateFAN(Artifact artifact) throws Exception {
return ((FileArtifactValue) evaluateArtifactValue(artifact));
}
- private SkyValue evaluateArtifactValue(Artifact artifact) throws Throwable {
+ private SkyValue evaluateArtifactValue(Artifact artifact) throws Exception {
SkyKey key = Artifact.key(artifact);
EvaluationResult<SkyValue> result = evaluate(ImmutableList.of(key).toArray(new SkyKey[0]));
if (result.hasError()) {
@@ -520,8 +518,7 @@
FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)),
null);
FileArtifactValue withDigest =
- FileArtifactValue.createFromInjectedDigest(
- noDigest, path.getDigest(), !output.isConstantMetadata());
+ FileArtifactValue.createFromInjectedDigest(noDigest, path.getDigest());
artifactData.put(output, withDigest);
} else {
artifactData.put(output, FileArtifactValue.DEFAULT_MIDDLEMAN);
@@ -529,12 +526,10 @@
} catch (IOException e) {
throw new IllegalStateException(e);
}
- return ActionExecutionValue.create(
+ return ActionExecutionValue.createForTesting(
ImmutableMap.copyOf(artifactData),
ImmutableMap.copyOf(treeArtifactData),
- /*outputSymlinks=*/ null,
- /*discoveredModules=*/ null,
- /*shareable=*/ true);
+ /*outputSymlinks=*/ null);
}
}
}