From 7f55cb768ae22352ac6599086e7cf6525a9565a5 Mon Sep 17 00:00:00 2001 From: jhorvitz Date: Thu, 16 Dec 2021 18:52:24 -0800 Subject: [PATCH] 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 --- .../build/lib/actions/ActionLookupData.java | 27 ++-- .../devtools/build/lib/actions/Artifact.java | 5 +- .../build/lib/actions/FileArtifactValue.java | 115 ++++++------------ .../build/lib/exec/SingleBuildFileCache.java | 2 +- .../common/RemoteActionFileArtifactValue.java | 10 +- .../lib/skyframe/ActionExecutionFunction.java | 4 +- .../lib/skyframe/ActionExecutionValue.java | 55 ++------- .../lib/skyframe/ActionMetadataHandler.java | 11 +- .../lib/skyframe/ArtifactNestedSetKey.java | 5 +- .../lib/skyframe/ArtifactNestedSetValue.java | 9 +- .../lib/skyframe/AspectCompletionValue.java | 7 +- .../build/lib/skyframe/BzlLoadValue.java | 7 ++ .../build/lib/skyframe/PrecomputedValue.java | 76 +++++++----- .../build/lib/skyframe/SkyFunctions.java | 40 ++---- .../lib/skyframe/SkyframeActionExecutor.java | 3 +- .../lib/skyframe/TargetCompletionValue.java | 7 +- .../lib/skyframe/TestCompletionValue.java | 25 ++-- .../com/google/devtools/build/skyframe/BUILD | 3 +- .../build/skyframe/ErrorTransienceValue.java | 27 ++-- .../build/skyframe/ShareabilityOfValue.java | 36 ------ .../build/skyframe/SkyFunctionName.java | 76 ++++-------- .../devtools/build/skyframe/SkyKey.java | 18 ++- .../devtools/build/skyframe/SkyValue.java | 12 +- .../lib/exec/SpawnInputExpanderTest.java | 18 +-- .../lib/remote/FakeActionInputFileCache.java | 11 +- .../remote/RemoteActionFileSystemTest.java | 24 ++-- ...ValueTransformSharedTreeArtifactsTest.java | 8 +- .../skyframe/ActionMetadataHandlerTest.java | 11 +- .../lib/skyframe/ArtifactFunctionTest.java | 31 ++--- .../lib/skyframe/FileArtifactValueTest.java | 33 ++--- .../skyframe/FilesystemValueCheckerTest.java | 27 ++-- .../FilesystemValueCheckerTestBase.java | 9 +- .../skyframe/TreeArtifactMetadataTest.java | 27 ++-- .../devtools/build/skyframe/GraphTester.java | 60 +++------ 34 files changed, 305 insertions(+), 534 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/skyframe/ShareabilityOfValue.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java index 7e4c87e9a2111a..4e4282c8b2a0ed 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java @@ -18,7 +18,6 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.skyframe.ExecutionPhaseSkyKey; -import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyFunctionName; /** Data that uniquely identifies an action. */ @@ -44,7 +43,10 @@ public static ActionLookupData create(ActionLookupKey actionLookupKey, int actio : new ActionLookupData(actionLookupKey, actionIndex); } - /** Similar to {@link #create}, but the key will have {@link ShareabilityOfValue#NEVER}. */ + /** + * Similar to {@link #create}, but the key will return {@code false} for {@link + * #valueIsShareable}. + */ public static ActionLookupData createUnshareable( ActionLookupKey actionLookupKey, int actionIndex) { return new UnshareableActionLookupData(actionLookupKey, actionIndex); @@ -58,21 +60,25 @@ public ActionLookupKey getActionLookupKey() { * Index of the action in question in the node keyed by {@link #getActionLookupKey}. Should be * passed to {@link ActionLookupValue#getAction}. */ - public int getActionIndex() { + public final int getActionIndex() { return actionIndex; } - public Label getLabel() { + public final Label getLabel() { return actionLookupKey.getLabel(); } @Override - public int hashCode() { - return 37 * actionLookupKey.hashCode() + actionIndex; + public final int hashCode() { + int hash = 1; + hash = 37 * hash + actionLookupKey.hashCode(); + hash = 37 * hash + Integer.hashCode(actionIndex); + hash = 37 * hash + Boolean.hashCode(valueIsShareable()); + return hash; } @Override - public boolean equals(Object obj) { + public final boolean equals(Object obj) { if (this == obj) { return true; } @@ -81,7 +87,8 @@ public boolean equals(Object obj) { } ActionLookupData that = (ActionLookupData) obj; return this.actionIndex == that.actionIndex - && this.actionLookupKey.equals(that.actionLookupKey); + && this.actionLookupKey.equals(that.actionLookupKey) + && valueIsShareable() == that.valueIsShareable(); } @Override @@ -103,8 +110,8 @@ private UnshareableActionLookupData(ActionLookupKey actionLookupKey, int actionI } @Override - public ShareabilityOfValue getShareabilityOfValue() { - return ShareabilityOfValue.NEVER; + public boolean valueIsShareable() { + return false; } } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 121307784b5254..6c2ebc85744837 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -49,7 +49,6 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; -import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.protobuf.CodedInputStream; @@ -1117,8 +1116,8 @@ public PathFragment getParentRelativePath() { } @Override - public ShareabilityOfValue getShareabilityOfValue() { - return isConstantMetadata() ? ShareabilityOfValue.NEVER : super.getShareabilityOfValue(); + public boolean valueIsShareable() { + return !isConstantMetadata(); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 58009868547c6a..d74ed514535e7d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -201,48 +201,34 @@ public static FileArtifactValue createForSourceArtifact(Artifact artifact, FileV isFile, isFile ? fileValue.getSize() : 0, isFile ? fileValue.realFileStateValue().getContentsProxy() : null, - isFile ? fileValue.getDigest() : null, - /* isShareable=*/ true); + isFile ? fileValue.getDigest() : null); } public static FileArtifactValue createFromInjectedDigest( - FileArtifactValue metadata, @Nullable byte[] digest, boolean isShareable) { - return createForNormalFile( - digest, metadata.getContentsProxy(), metadata.getSize(), isShareable); + FileArtifactValue metadata, @Nullable byte[] digest) { + return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize()); } @VisibleForTesting public static FileArtifactValue createForTesting(Artifact artifact) throws IOException { - Path path = artifact.getPath(); - boolean isShareable = !artifact.isConstantMetadata(); - // Caution: there's a race condition between stating the file and computing the - // digest. We need to stat first, since we're using the stat to detect changes. - // We follow symlinks here to be consistent with getDigest. - return createFromStat(path, path.stat(Symlinks.FOLLOW), isShareable); + return createForTesting(artifact.getPath()); } @VisibleForTesting public static FileArtifactValue createForTesting(Path path) throws IOException { - /*isShareable=*/ - // Caution: there's a race condition between stating the file and computing the - // digest. We need to stat first, since we're using the stat to detect changes. - // We follow symlinks here to be consistent with getDigest. - return createFromStat(path, path.stat(Symlinks.FOLLOW), true); + // Caution: there's a race condition between stating the file and computing the digest. We need + // to stat first, since we're using the stat to detect changes. We follow symlinks here to be + // consistent with getDigest. + return createFromStat(path, path.stat(Symlinks.FOLLOW)); } - public static FileArtifactValue createFromStat(Path path, FileStatus stat, boolean isShareable) - throws IOException { + public static FileArtifactValue createFromStat(Path path, FileStatus stat) throws IOException { return create( - path, stat.isFile(), stat.getSize(), FileContentsProxy.create(stat), null, isShareable); + path, stat.isFile(), stat.getSize(), FileContentsProxy.create(stat), /*digest=*/ null); } private static FileArtifactValue create( - Path path, - boolean isFile, - long size, - FileContentsProxy proxy, - @Nullable byte[] digest, - boolean isShareable) + Path path, boolean isFile, long size, FileContentsProxy proxy, @Nullable byte[] digest) throws IOException { if (!isFile) { // In this case, we need to store the mtime because the action cache uses mtime for @@ -254,7 +240,7 @@ private static FileArtifactValue create( digest = DigestUtils.getDigestWithManualFallback(path, size); } Preconditions.checkState(digest != null, path); - return createForNormalFile(digest, proxy, size, isShareable); + return createForNormalFile(digest, proxy, size); } public static FileArtifactValue createForVirtualActionInput(byte[] digest, long size) { @@ -281,10 +267,8 @@ public static FileArtifactValue createForUnresolvedSymlink(Path symlink) throws @VisibleForTesting public static FileArtifactValue createForNormalFile( - byte[] digest, @Nullable FileContentsProxy proxy, long size, boolean isShareable) { - return isShareable - ? new RegularFileArtifactValue(digest, proxy, size) - : new UnshareableRegularFileArtifactValue(digest, proxy, size); + byte[] digest, @Nullable FileContentsProxy proxy, long size) { + return new RegularFileArtifactValue(digest, proxy, size); } /** @@ -293,7 +277,7 @@ public static FileArtifactValue createForNormalFile( */ public static FileArtifactValue createForNormalFileUsingPath(Path path, long size) throws IOException { - return create(path, true, size, null, null, true); + return create(path, /*isFile=*/ true, size, /*proxy=*/ null, /*digest=*/ null); } public static FileArtifactValue createForDirectoryWithHash(byte[] digest) { @@ -310,7 +294,7 @@ public static FileArtifactValue createForDirectoryWithMtime(long mtime) { */ public static FileArtifactValue createProxy(byte[] digest) { Preconditions.checkNotNull(digest); - return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0, /*isShareable=*/ true); + return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0); } private static String bytesToString(byte[] bytes) { @@ -447,8 +431,7 @@ public String toString() { } } - private static class RegularFileArtifactValue extends FileArtifactValue { - + private static final class RegularFileArtifactValue extends FileArtifactValue { private final byte[] digest; @Nullable private final FileContentsProxy proxy; private final long size; @@ -462,20 +445,21 @@ private RegularFileArtifactValue( @Override public boolean equals(Object o) { + if (this == o) { + return true; + } if (!(o instanceof RegularFileArtifactValue)) { return false; } - RegularFileArtifactValue that = (RegularFileArtifactValue) o; return Arrays.equals(digest, that.digest) && Objects.equals(proxy, that.proxy) - && size == that.size - && dataIsShareable() == that.dataIsShareable(); + && size == that.size; } @Override public int hashCode() { - return Objects.hash(Arrays.hashCode(digest), proxy, size, dataIsShareable()); + return Objects.hash(Arrays.hashCode(digest), proxy, size); } @Override @@ -535,18 +519,6 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue o) { } } - private static final class UnshareableRegularFileArtifactValue extends RegularFileArtifactValue { - private UnshareableRegularFileArtifactValue( - byte[] digest, @Nullable FileContentsProxy proxy, long size) { - super(digest, proxy, size); - } - - @Override - public boolean dataIsShareable() { - return false; - } - } - /** Metadata for remotely stored files. */ public static class RemoteFileArtifactValue extends FileArtifactValue { private final byte[] digest; @@ -575,14 +547,12 @@ public boolean equals(Object o) { return Arrays.equals(digest, that.digest) && size == that.size && locationIndex == that.locationIndex - && Objects.equals(actionId, that.actionId) - && dataIsShareable() == that.dataIsShareable(); + && Objects.equals(actionId, that.actionId); } @Override public int hashCode() { - return Objects.hash( - Arrays.hashCode(digest), size, locationIndex, actionId, dataIsShareable()); + return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId); } @Override @@ -682,7 +652,12 @@ public boolean wasModifiedSinceDigest(Path path) { } /** File stored inline in metadata. */ - public static class InlineFileArtifactValue extends FileArtifactValue { + public static final class InlineFileArtifactValue extends FileArtifactValue { + + public static InlineFileArtifactValue create(byte[] bytes, HashFunction hashFunction) { + return new InlineFileArtifactValue(bytes, hashFunction.hashBytes(bytes).asBytes()); + } + private final byte[] data; private final byte[] digest; @@ -691,30 +666,21 @@ private InlineFileArtifactValue(byte[] data, byte[] digest) { this.digest = Preconditions.checkNotNull(digest); } - private InlineFileArtifactValue(byte[] bytes, HashFunction hashFunction) { - this(bytes, hashFunction.hashBytes(bytes).asBytes()); - } - @Override public boolean equals(Object o) { + if (this == o) { + return true; + } if (!(o instanceof InlineFileArtifactValue)) { return false; } - InlineFileArtifactValue that = (InlineFileArtifactValue) o; - return Arrays.equals(digest, that.digest) && dataIsShareable() == that.dataIsShareable(); + return Arrays.equals(digest, that.digest); } @Override public int hashCode() { - return Objects.hash(Arrays.hashCode(digest), dataIsShareable()); - } - - public static InlineFileArtifactValue create( - byte[] bytes, boolean shareable, HashFunction hashFunction) { - return shareable - ? new InlineFileArtifactValue(bytes, hashFunction) - : new UnshareableInlineFileArtifactValue(bytes, hashFunction); + return Arrays.hashCode(digest); } public ByteArrayInputStream getInputStream() { @@ -752,17 +718,6 @@ public boolean wasModifiedSinceDigest(Path path) { } } - private static final class UnshareableInlineFileArtifactValue extends InlineFileArtifactValue { - UnshareableInlineFileArtifactValue(byte[] bytes, HashFunction hashFunction) { - super(bytes, hashFunction); - } - - @Override - public boolean dataIsShareable() { - return false; - } - } - /** * Used to resolve source symlinks when diskless. * diff --git a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java index 7588e810d0ff91..1a0ff6284ca715 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java @@ -60,7 +60,7 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { Path path = ActionInputHelper.toInputPath(input, execRoot); FileArtifactValue metadata; try { - metadata = FileArtifactValue.createFromStat(path, path.stat(Symlinks.FOLLOW), true); + metadata = FileArtifactValue.createFromStat(path, path.stat(Symlinks.FOLLOW)); } catch (IOException e) { return new ActionInputMetadata(input, e); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java index e3e96f66bf58b9..e3e30f74139daf 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java @@ -46,18 +46,12 @@ public boolean equals(Object o) { && getSize() == that.getSize() && getLocationIndex() == that.getLocationIndex() && Objects.equals(getActionId(), that.getActionId()) - && isExecutable == that.isExecutable - && dataIsShareable() == that.dataIsShareable(); + && isExecutable == that.isExecutable; } @Override public int hashCode() { return Objects.hash( - Arrays.hashCode(getDigest()), - getSize(), - getLocationIndex(), - getActionId(), - isExecutable, - dataIsShareable()); + Arrays.hashCode(getDigest()), getSize(), getLocationIndex(), getActionId(), isExecutable); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 1c1b7eebc2782b..60d1d48f4a6df8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -382,7 +382,7 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) // Remove action from state map in case it's there (won't be unless it discovers inputs). stateMap.remove(action); - if (sketch != null && result.dataIsShareable()) { + if (sketch != null && actionLookupData.valueIsShareable()) { topDownActionCache.put(sketch, result); } return result; @@ -819,7 +819,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( + "SkyframeAwareAction which should be re-executed unconditionally. Action: %s", action); return ActionExecutionValue.createFromOutputStore( - metadataHandler.getOutputStore(), /*outputSymlinks=*/ null, action, actionLookupData); + metadataHandler.getOutputStore(), /*outputSymlinks=*/ null, action); } metadataHandler.prepareForActionExecution(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java index a17fef55d34e48..65ee2fa1921485 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java @@ -21,8 +21,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.Action; -import com.google.devtools.build.lib.actions.ActionLookupData; -import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; @@ -36,7 +34,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.rules.cpp.IncludeScannable; import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; -import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyValue; import java.util.Map; import java.util.NoSuchElementException; @@ -46,7 +43,7 @@ /** A value representing an executed action. */ @Immutable @ThreadSafe -public class ActionExecutionValue implements SkyValue { +public final class ActionExecutionValue implements SkyValue { /** A map from each output artifact of this action to their {@link FileArtifactValue}s. */ private final ImmutableMap artifactData; @@ -107,31 +104,23 @@ private ActionExecutionValue( static ActionExecutionValue createFromOutputStore( OutputStore outputStore, @Nullable ImmutableList outputSymlinks, - Action action, - ActionLookupData lookupData) { - return create( + Action action) { + return new ActionExecutionValue( outputStore.getAllArtifactData(), outputStore.getAllTreeArtifactData(), outputSymlinks, action instanceof IncludeScannable ? ((IncludeScannable) action).getDiscoveredModules() - : null, - !Actions.dependsOnBuildId(action) - && lookupData.getShareabilityOfValue() != ShareabilityOfValue.NEVER); + : null); } @VisibleForTesting - static ActionExecutionValue create( + public static ActionExecutionValue createForTesting( ImmutableMap artifactData, ImmutableMap treeArtifactData, - @Nullable ImmutableList outputSymlinks, - @Nullable NestedSet discoveredModules, - boolean shareable) { - return shareable - ? new ActionExecutionValue( - artifactData, treeArtifactData, outputSymlinks, discoveredModules) - : new CrossServerUnshareableActionExecutionValue( - artifactData, treeArtifactData, outputSymlinks, discoveredModules); + @Nullable ImmutableList outputSymlinks) { + return new ActionExecutionValue( + artifactData, treeArtifactData, outputSymlinks, /*discoveredModules=*/ null); } /** @@ -230,33 +219,12 @@ public boolean equals(Object obj) { ActionExecutionValue o = (ActionExecutionValue) obj; return artifactData.equals(o.artifactData) && treeArtifactData.equals(o.treeArtifactData) - && dataIsShareable() == o.dataIsShareable() && Objects.equal(outputSymlinks, o.outputSymlinks); } @Override public int hashCode() { - return Objects.hashCode(artifactData, treeArtifactData); - } - - /** - * Subclass that reports this value cannot be shared across servers. Note that this is unrelated - * to the concept of shared actions. - */ - private static final class CrossServerUnshareableActionExecutionValue - extends ActionExecutionValue { - CrossServerUnshareableActionExecutionValue( - ImmutableMap artifactData, - ImmutableMap treeArtifactData, - @Nullable ImmutableList outputSymlinks, - @Nullable NestedSet discoveredModules) { - super(artifactData, treeArtifactData, outputSymlinks, discoveredModules); - } - - @Override - public boolean dataIsShareable() { - return false; - } + return Objects.hashCode(artifactData, treeArtifactData, outputSymlinks); } private static ImmutableMap transformMap( @@ -327,13 +295,12 @@ public ActionExecutionValue transformForSharedAction(Action action) { action); Map newArtifactMap = Maps.uniqueIndex(action.getOutputs(), OwnerlessArtifactWrapper::new); - return create( + return new ActionExecutionValue( transformMap(artifactData, newArtifactMap, action, (newArtifact, value) -> value), transformMap( treeArtifactData, newArtifactMap, action, ActionExecutionValue::transformSharedTree), outputSymlinks, // Discovered modules come from the action's inputs, and so don't need to be transformed. - discoveredModules, - dataIsShareable()); + discoveredModules); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 52464a9d342500..3171f12c977e5f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -556,8 +556,7 @@ private FileArtifactValue constructFileArtifactValue( injectedDigest = DigestUtils.manuallyComputeDigest(artifactPathResolver.toPath(artifact), value.getSize()); } - return FileArtifactValue.createFromInjectedDigest( - value, injectedDigest, /*isShareable=*/ !artifact.isConstantMetadata()); + return FileArtifactValue.createFromInjectedDigest(value, injectedDigest); } /** @@ -608,7 +607,6 @@ private static FileArtifactValue fileArtifactValueFromArtifact( rootedPathNoFollow, statNoFollow, digestWillBeInjected, - artifact.isConstantMetadata(), tsgm); } @@ -637,7 +635,6 @@ private static FileArtifactValue fileArtifactValueFromArtifact( realRootedPath, realStatWithDigest, digestWillBeInjected, - artifact.isConstantMetadata(), tsgm); } @@ -645,7 +642,6 @@ private static FileArtifactValue fileArtifactValueFromStat( RootedPath rootedPath, FileStatusWithDigest stat, boolean digestWillBeInjected, - boolean isConstantMetadata, @Nullable TimestampGranularityMonitor tsgm) throws IOException { if (stat == null) { @@ -658,10 +654,7 @@ private static FileArtifactValue fileArtifactValueFromStat( return stat.isDirectory() ? FileArtifactValue.createForDirectoryWithMtime(stat.getLastModifiedTime()) : FileArtifactValue.createForNormalFile( - fileStateValue.getDigest(), - fileStateValue.getContentsProxy(), - stat.getSize(), - /*isShareable=*/ !isConstantMetadata); + fileStateValue.getDigest(), fileStateValue.getContentsProxy(), stat.getSize()); } private static void setPathReadOnlyAndExecutableIfFile(Path path) throws IOException { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetKey.java index f535e92bff04b1..c9f6fe5e16442e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetKey.java @@ -17,7 +17,6 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.skyframe.ExecutionPhaseSkyKey; -import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyFunctionName; /** SkyKey for {@code NestedSet}. */ @@ -47,10 +46,10 @@ public NestedSet getSet() { } @Override - public ShareabilityOfValue getShareabilityOfValue() { + public boolean valueIsShareable() { // ArtifactNestedSetValue is just a promise that data is available in memory. Not meant for // cross-server sharing. - return ShareabilityOfValue.NEVER; + return false; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetValue.java index 76193430209042..0e78619822118f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactNestedSetValue.java @@ -23,11 +23,4 @@ */ @Immutable @ThreadSafe -public final class ArtifactNestedSetValue implements SkyValue { - - @Override - public boolean dataIsShareable() { - // This is just a promise that data is available in memory. Not meant for cross-server sharing. - return false; - } -} +public final class ArtifactNestedSetValue implements SkyValue {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java index 6eb9b44aac1404..2e567543a6cbcd 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletionValue.java @@ -49,8 +49,13 @@ public static AspectCompletionKey create( public abstract AspectKey actionLookupKey(); @Override - public SkyFunctionName functionName() { + public final SkyFunctionName functionName() { return SkyFunctions.ASPECT_COMPLETION; } + + @Override + public final boolean valueIsShareable() { + return false; + } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java index 83b8c8e6dd94d9..609af605f29943 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java @@ -103,6 +103,13 @@ boolean isBuiltins() { public SkyFunctionName functionName() { return SkyFunctions.BZL_LOAD; } + + @Override + public final boolean valueIsShareable() { + // We don't guarantee that all constructs implement equality, meaning we can't correctly + // compare deserialized instances. This is currently the case for attribute descriptors. + return false; + } } /** A key for loading a .bzl during package loading (BUILD evaluation). */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java index e1a64e3fff89d4..6518feb6fc0c29 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.base.Suppliers; @@ -23,10 +24,12 @@ import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; import com.google.devtools.build.skyframe.AbstractSkyKey; import com.google.devtools.build.skyframe.Injectable; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.Map; import java.util.UUID; @@ -38,7 +41,7 @@ * "precomputed" from skyframe's perspective and so the graph needs to be prepopulated with them * (e.g. via injection). */ -public class PrecomputedValue implements SkyValue { +public final class PrecomputedValue implements SkyValue { /** * An externally-injected precomputed value. Exists so that modules can inject precomputed values * into Skyframe's graph. @@ -47,9 +50,9 @@ public class PrecomputedValue implements SkyValue { */ public static final class Injected { private final Precomputed precomputed; - private final Supplier supplier; + private final Supplier supplier; - private Injected(Precomputed precomputed, Supplier supplier) { + private Injected(Precomputed precomputed, Supplier supplier) { this.precomputed = precomputed; this.supplier = supplier; } @@ -81,7 +84,7 @@ public static Injected injected(Precomputed precomputed, T value) { public static final Precomputed STARLARK_SEMANTICS = new Precomputed<>("starlark_semantics"); - static final Precomputed BUILD_ID = new UnsharablePrecomputed<>("build_id"); + static final Precomputed BUILD_ID = new Precomputed<>("build_id", /*shareable=*/ false); public static final Precomputed> ACTION_ENV = new Precomputed<>("action_env"); @@ -131,15 +134,19 @@ public String toString() { * *

Instances do not have internal state. */ - public static class Precomputed { - protected final Key key; + public static final class Precomputed { + private final SkyKey key; public Precomputed(String key) { - this.key = Key.create(key); + this(key, /*shareable=*/ true); + } + + private Precomputed(String key, boolean shareable) { + this.key = shareable ? Key.create(key) : UnshareableKey.create(key); } @VisibleForTesting - public Key getKeyForTesting() { + public SkyKey getKeyForTesting() { return key; } @@ -162,49 +169,60 @@ public T get(SkyFunction.Environment env) throws InterruptedException { public void set(Injectable injectable, T value) { injectable.inject(key, new PrecomputedValue(value)); } - } - - private static class UnsharablePrecomputed extends Precomputed { - private UnsharablePrecomputed(String key) { - super(key); - } - /** Injects a new variable value. */ @Override - public void set(Injectable injectable, T value) { - injectable.inject(key, new UnshareablePrecomputedValue(value)); + public String toString() { + return MoreObjects.toStringHelper(this) + .add("key", key) + .add("shareable", key.valueIsShareable()) + .toString(); } } - /** An unshareable version of {@link PrecomputedValue}. */ - private static final class UnshareablePrecomputedValue extends PrecomputedValue { - private UnshareablePrecomputedValue(Object value) { - super(value); + /** {@link com.google.devtools.build.skyframe.SkyKey} for {@code PrecomputedValue}. */ + @AutoCodec + public static final class Key extends AbstractSkyKey { + private static final Interner interner = BlazeInterners.newWeakInterner(); + + private Key(String arg) { + super(arg); + } + + @AutoCodec.Instantiator + public static Key create(String arg) { + return interner.intern(new Key(arg)); } @Override - public boolean dataIsShareable() { - return false; + public SkyFunctionName functionName() { + return SkyFunctions.PRECOMPUTED; } } - /** {@link com.google.devtools.build.skyframe.SkyKey} for {@code PrecomputedValue}. */ + /** Unshareable version of {@link Key}. */ @AutoCodec - public static class Key extends AbstractSkyKey { - private static final Interner interner = BlazeInterners.newWeakInterner(); + @VisibleForSerialization + static final class UnshareableKey extends AbstractSkyKey { + private static final Interner interner = BlazeInterners.newWeakInterner(); - private Key(String arg) { + private UnshareableKey(String arg) { super(arg); } @AutoCodec.Instantiator - public static Key create(String arg) { - return interner.intern(new Key(arg)); + @VisibleForSerialization + static UnshareableKey create(String arg) { + return interner.intern(new UnshareableKey(arg)); } @Override public SkyFunctionName functionName() { return SkyFunctions.PRECOMPUTED; } + + @Override + public boolean valueIsShareable() { + return false; + } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 9d8cd4285d13c8..11c46c327681a8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -13,10 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Predicate; -import com.google.devtools.build.skyframe.FunctionHermeticity; -import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -42,10 +39,7 @@ public final class SkyFunctions { public static final SkyFunctionName BZL_COMPILE = SkyFunctionName.createHermetic("BZL_COMPILE"); public static final SkyFunctionName STARLARK_BUILTINS = SkyFunctionName.createHermetic("STARLARK_BUILTINS"); - // Never shareable - we don't guarantee that all constructs implement equality, meaning we can't - // correctly compare deserialized instances. This is currently the case for attribute descriptors. - public static final SkyFunctionName BZL_LOAD = - SkyFunctionName.create("BZL_LOAD", ShareabilityOfValue.NEVER, FunctionHermeticity.HERMETIC); + public static final SkyFunctionName BZL_LOAD = SkyFunctionName.createHermetic("BZL_LOAD"); public static final SkyFunctionName GLOB = SkyFunctionName.createHermetic("GLOB"); public static final SkyFunctionName PACKAGE = SkyFunctionName.createHermetic("PACKAGE"); static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.createHermetic("PACKAGE_ERROR"); @@ -89,35 +83,26 @@ public final class SkyFunctions { static final SkyFunctionName TOP_LEVEL_ACTION_LOOKUP_CONFLICT_FINDING = SkyFunctionName.createHermetic("TOP_LEVEL_ACTION_LOOKUP_CONFLICT_DETECTION"); public static final SkyFunctionName ASPECT = SkyFunctionName.createHermetic("ASPECT"); - static final SkyFunctionName LOAD_STARLARK_ASPECT = - SkyFunctionName.createHermetic("LOAD_STARLARK_ASPECT"); static final SkyFunctionName TOP_LEVEL_ASPECTS = SkyFunctionName.createHermetic("TOP_LEVEL_ASPECTS"); static final SkyFunctionName BUILD_TOP_LEVEL_ASPECTS_DETAILS = SkyFunctionName.createHermetic("BUILD_TOP_LEVEL_ASPECTS_DETAILS"); public static final SkyFunctionName TARGET_COMPLETION = - SkyFunctionName.create( - "TARGET_COMPLETION", ShareabilityOfValue.NEVER, FunctionHermeticity.HERMETIC); + SkyFunctionName.createHermetic("TARGET_COMPLETION"); public static final SkyFunctionName ASPECT_COMPLETION = - SkyFunctionName.create( - "ASPECT_COMPLETION", ShareabilityOfValue.NEVER, FunctionHermeticity.HERMETIC); - static final SkyFunctionName TEST_COMPLETION = - SkyFunctionName.create( - "TEST_COMPLETION", ShareabilityOfValue.NEVER, FunctionHermeticity.HERMETIC); + SkyFunctionName.createHermetic("ASPECT_COMPLETION"); + static final SkyFunctionName TEST_COMPLETION = SkyFunctionName.createHermetic("TEST_COMPLETION"); public static final SkyFunctionName BUILD_CONFIGURATION = SkyFunctionName.createHermetic("BUILD_CONFIGURATION"); - // Test actions are not shareable. Action execution can be nondeterministic, so is semi-hermetic. + // Action execution can be nondeterministic, so semi-hermetic. public static final SkyFunctionName ACTION_EXECUTION = SkyFunctionName.createSemiHermetic("ACTION_EXECUTION"); public static final SkyFunctionName ARTIFACT_NESTED_SET = SkyFunctionName.createHermetic("ARTIFACT_NESTED_SET"); public static final SkyFunctionName PATH_CASING_LOOKUP = SkyFunctionName.createHermetic("PATH_CASING_LOOKUP"); - - @VisibleForTesting - public static final SkyFunctionName RECURSIVE_FILESYSTEM_TRAVERSAL = + static final SkyFunctionName RECURSIVE_FILESYSTEM_TRAVERSAL = SkyFunctionName.createHermetic("RECURSIVE_DIRECTORY_TRAVERSAL"); - public static final SkyFunctionName FILESET_ENTRY = SkyFunctionName.createHermetic("FILESET_ENTRY"); static final SkyFunctionName BUILD_INFO_COLLECTION = @@ -148,14 +133,10 @@ public final class SkyFunctions { SkyFunctionName.createHermetic("TOOLCHAIN_RESOLUTION"); public static final SkyFunctionName REPOSITORY_MAPPING = SkyFunctionName.createHermetic("REPOSITORY_MAPPING"); - public static final SkyFunctionName REPO_MAPPING_FOR_BZLMOD_BZL_LOAD = - SkyFunctionName.createHermetic("REPO_MAPPING_FOR_BZLMOD_BZL_LOAD"); public static final SkyFunctionName RESOLVED_FILE = SkyFunctionName.createHermetic("RESOLVED_FILE"); public static final SkyFunctionName RESOLVED_HASH_VALUES = SkyFunctionName.createHermetic("RESOLVED_HASH_VALUES"); - public static final SkyFunctionName LOCAL_CONFIG_PLATFORM = - SkyFunctionName.createHermetic("LOCAL_CONFIG_PLATFORM"); public static final SkyFunctionName MODULE_FILE = SkyFunctionName.createNonHermetic("MODULE_FILE"); public static final SkyFunctionName BUILD_DRIVER = @@ -169,12 +150,7 @@ public final class SkyFunctions { public static final SkyFunctionName SINGLE_EXTENSION_EVAL = SkyFunctionName.createNonHermetic("SINGLE_EXTENSION_EVAL"); - public static Predicate isSkyFunction(final SkyFunctionName functionName) { - return new Predicate() { - @Override - public boolean apply(SkyKey key) { - return key.functionName().equals(functionName); - } - }; + public static Predicate isSkyFunction(SkyFunctionName functionName) { + return key -> key.functionName().equals(functionName); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 3cfd0d8122de98..3c90c1737ecdea 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -1264,8 +1264,7 @@ private ActionExecutionValue actuallyCompleteAction( return ActionExecutionValue.createFromOutputStore( this.metadataHandler.getOutputStore(), actionExecutionContext.getOutputSymlinks(), - action, - actionLookupData); + action); } /** A closure to continue an asynchronously running action. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java index ff34827e6ca4f4..9ed1b2df1d1839 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletionValue.java @@ -68,10 +68,15 @@ static TargetCompletionKey create( public abstract ConfiguredTargetKey actionLookupKey(); @Override - public SkyFunctionName functionName() { + public final SkyFunctionName functionName() { return SkyFunctions.TARGET_COMPLETION; } + @Override + public final boolean valueIsShareable() { + return false; + } + abstract boolean willTest(); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionValue.java index f55e50725ae25e..cd2e12f1e975ee 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionValue.java @@ -29,26 +29,22 @@ * A test completion value represents the completion of a test target. This includes the execution * of all test shards and repeated runs, if applicable. */ -public class TestCompletionValue implements SkyValue { +public final class TestCompletionValue implements SkyValue { static final TestCompletionValue TEST_COMPLETION_MARKER = new TestCompletionValue(); private TestCompletionValue() { } - @Override - public boolean dataIsShareable() { - return false; - } - public static SkyKey key( ConfiguredTargetKey lac, - final TopLevelArtifactContext topLevelArtifactContext, - final boolean exclusiveTesting) { + TopLevelArtifactContext topLevelArtifactContext, + boolean exclusiveTesting) { return TestCompletionKey.create(lac, topLevelArtifactContext, exclusiveTesting); } - public static Iterable keys(Collection targets, - final TopLevelArtifactContext topLevelArtifactContext, - final boolean exclusiveTesting) { + public static Iterable keys( + Collection targets, + TopLevelArtifactContext topLevelArtifactContext, + boolean exclusiveTesting) { return Iterables.transform( targets, ct -> @@ -84,8 +80,13 @@ static TestCompletionKey create( public abstract boolean exclusiveTesting(); @Override - public SkyFunctionName functionName() { + public final SkyFunctionName functionName() { return SkyFunctions.TEST_COMPLETION; } + + @Override + public final boolean valueIsShareable() { + return false; + } } } diff --git a/src/main/java/com/google/devtools/build/skyframe/BUILD b/src/main/java/com/google/devtools/build/skyframe/BUILD index 7ab3fdded75589..91c12d51f86ab5 100644 --- a/src/main/java/com/google/devtools/build/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/skyframe/BUILD @@ -11,7 +11,6 @@ package( SKYFRAME_OBJECT_SRCS = [ "AbstractSkyKey.java", "FunctionHermeticity.java", - "ShareabilityOfValue.java", "SkyFunctionName.java", "SkyKey.java", "SkyValue.java", @@ -23,7 +22,7 @@ java_library( srcs = SKYFRAME_OBJECT_SRCS, visibility = ["//visibility:public"], deps = [ - "//third_party:caffeine", + "//src/main/java/com/google/devtools/build/lib/concurrent", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java index c3a84fb954b1b6..941d03dd46fea2 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java +++ b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java @@ -22,21 +22,32 @@ public final class ErrorTransienceValue implements SkyValue { private static final SkyFunctionName FUNCTION_NAME = - SkyFunctionName.create( - "ERROR_TRANSIENCE", ShareabilityOfValue.NEVER, FunctionHermeticity.NONHERMETIC); + SkyFunctionName.createNonHermetic("ERROR_TRANSIENCE"); - @SerializationConstant public static final SkyKey KEY = () -> FUNCTION_NAME; + @SerializationConstant + public static final SkyKey KEY = + new SkyKey() { + @Override + public SkyFunctionName functionName() { + return FUNCTION_NAME; + } + + @Override + public boolean valueIsShareable() { + return false; + } + + @Override + public String toString() { + return "ErrorTransienceValue.KEY"; + } + }; @SerializationConstant public static final ErrorTransienceValue INSTANCE = new ErrorTransienceValue(); private ErrorTransienceValue() {} - @Override - public boolean dataIsShareable() { - return false; - } - @Override public int hashCode() { // Not the prettiest, but since we always return false for equals throw exception here to catch diff --git a/src/main/java/com/google/devtools/build/skyframe/ShareabilityOfValue.java b/src/main/java/com/google/devtools/build/skyframe/ShareabilityOfValue.java deleted file mode 100644 index 625c5db577e229..00000000000000 --- a/src/main/java/com/google/devtools/build/skyframe/ShareabilityOfValue.java +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright 2018 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; - -/** - * When the {@link NodeEntry#getValue} corresponding to a given {@link SkyFunctionName} is - * shareable: always, sometimes (depending on the specific key argument and/or value), or never. - * - *

Values may be unshareable because they are just not serializable, or because they contain data - * that cannot safely be re-used as-is by another invocation. - * - *

Unshareable data should not be serialized, since it will never be re-used. Attempts to fetch - * serialized data will check this value and only perform the fetch if the value is not {@link - * #NEVER}. - */ -public enum ShareabilityOfValue { - /** - * Indicates that values produced by the function are shareable unless they override {@link - * SkyValue#dataIsShareable}. - */ - SOMETIMES, - /** Indicates that values produced by the function are not shareable. */ - NEVER -} diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java index 623a8d234c22ee..3d3f35634702c3 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java @@ -13,23 +13,22 @@ // limitations under the License. package com.google.devtools.build.skyframe; -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; +import com.google.common.collect.Interner; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import java.util.Set; /** An identifier for a {@code SkyFunction}. */ public final class SkyFunctionName { - private static final Cache interner = - Caffeine.newBuilder().build(); + private static final Interner interner = BlazeInterners.newStrongInterner(); /** * A well-known key type intended for testing only. The associated SkyKey should have a String * argument. */ - // Needs to be after the cache is initialized. + // Needs to be after the interner is initialized. public static final SkyFunctionName FOR_TESTING = SkyFunctionName.createHermetic("FOR_TESTING"); /** @@ -39,7 +38,7 @@ public final class SkyFunctionName { * opposed to transitively invalidated). */ public static SkyFunctionName createNonHermetic(String name) { - return create(name, ShareabilityOfValue.SOMETIMES, FunctionHermeticity.NONHERMETIC); + return create(name, FunctionHermeticity.NONHERMETIC); } /** @@ -47,7 +46,7 @@ public static SkyFunctionName createNonHermetic(String name) { * FunctionHermeticity#SEMI_HERMETIC semi-hermetic}. */ public static SkyFunctionName createSemiHermetic(String name) { - return create(name, ShareabilityOfValue.SOMETIMES, FunctionHermeticity.SEMI_HERMETIC); + return create(name, FunctionHermeticity.SEMI_HERMETIC); } /** @@ -55,47 +54,40 @@ public static SkyFunctionName createSemiHermetic(String name) { * to be a deterministic function of its dependencies, not doing any external operations). */ public static SkyFunctionName createHermetic(String name) { - return create(name, ShareabilityOfValue.SOMETIMES, FunctionHermeticity.HERMETIC); + return create(name, FunctionHermeticity.HERMETIC); } - public static SkyFunctionName create( - String name, ShareabilityOfValue shareabilityOfValue, FunctionHermeticity hermeticity) { - SkyFunctionName result = new SkyFunctionName(name, shareabilityOfValue, hermeticity); - SkyFunctionName cached = interner.get(new NameOnlyWrapper(result), unused -> result); + private static SkyFunctionName create(String name, FunctionHermeticity hermeticity) { + SkyFunctionName cached = interner.intern(new SkyFunctionName(name, hermeticity)); Preconditions.checkState( - result.equals(cached), - "Tried to create SkyFunctionName objects with same name but different properties: %s %s", - result, - cached); + cached.hermeticity.equals(hermeticity), + "Tried to create SkyFunctionName objects with same name (%s) but different hermeticity" + + " (old=%s, new=%s)", + name, + cached.hermeticity, + hermeticity); return cached; } private final String name; - private final ShareabilityOfValue shareabilityOfValue; private final FunctionHermeticity hermeticity; - private SkyFunctionName( - String name, ShareabilityOfValue shareabilityOfValue, FunctionHermeticity hermeticity) { - this.name = name; - this.shareabilityOfValue = shareabilityOfValue; - this.hermeticity = hermeticity; + private SkyFunctionName(String name, FunctionHermeticity hermeticity) { + this.name = Preconditions.checkNotNull(name); + this.hermeticity = Preconditions.checkNotNull(hermeticity); } public String getName() { return name; } - public ShareabilityOfValue getShareabilityOfValue() { - return shareabilityOfValue; - } - public FunctionHermeticity getHermeticity() { return hermeticity; } @Override public String toString() { - return name + (shareabilityOfValue.equals(ShareabilityOfValue.NEVER) ? " (unshareable)" : ""); + return name; } @Override @@ -107,48 +99,26 @@ public boolean equals(Object obj) { return false; } SkyFunctionName other = (SkyFunctionName) obj; - return name.equals(other.name) && shareabilityOfValue.equals(other.shareabilityOfValue); + return name.equals(other.name); } @Override public int hashCode() { - // Don't bother incorporating serializabilityOfValue into hashCode: should always be the same. + // Don't bother incorporating hermeticity into hashCode: should always be the same. return name.hashCode(); } /** * A predicate that returns true for {@link SkyKey}s that have the given {@link SkyFunctionName}. */ - public static Predicate functionIs(final SkyFunctionName functionName) { + public static Predicate functionIs(SkyFunctionName functionName) { return skyKey -> functionName.equals(skyKey.functionName()); } /** * A predicate that returns true for {@link SkyKey}s that have the given {@link SkyFunctionName}. */ - public static Predicate functionIsIn(final Set functionNames) { + public static Predicate functionIsIn(Set functionNames) { return skyKey -> functionNames.contains(skyKey.functionName()); } - - private static class NameOnlyWrapper { - private final SkyFunctionName skyFunctionName; - - private NameOnlyWrapper(SkyFunctionName skyFunctionName) { - this.skyFunctionName = skyFunctionName; - } - - @Override - public boolean equals(Object obj) { - if (!(obj instanceof NameOnlyWrapper)) { - return false; - } - SkyFunctionName thatFunctionName = ((NameOnlyWrapper) obj).skyFunctionName; - return this.skyFunctionName.getName().equals(thatFunctionName.name); - } - - @Override - public int hashCode() { - return skyFunctionName.getName().hashCode(); - } - } } diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyKey.java b/src/main/java/com/google/devtools/build/skyframe/SkyKey.java index 79284ac9f30c8d..f6cc4c09da365e 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyKey.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyKey.java @@ -33,7 +33,21 @@ default Object argument() { return this; } - default ShareabilityOfValue getShareabilityOfValue() { - return functionName().getShareabilityOfValue(); + /** + * Returns {@code true} if this key produces a {@link SkyValue} that can be reused across builds. + * + *

Values may be unshareable because they are just not serializable, or because they contain + * data that cannot safely be reused as-is by another invocation, such as stamping information or + * "flaky" values like test statuses. + * + *

Unshareable data should not be serialized, since it will never be reused. Attempts to fetch + * a key's serialized data will call this method and only perform the fetch if it returns {@code + * true}. + * + *

The result of this method only applies to non-error values. In case of an error, {@link + * ErrorInfo#isTransitivelyTransient()} can be used to determine shareability. + */ + default boolean valueIsShareable() { + return true; } } diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyValue.java b/src/main/java/com/google/devtools/build/skyframe/SkyValue.java index 1c0e1c6fc972a6..2d21e869133f97 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyValue.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyValue.java @@ -14,14 +14,4 @@ package com.google.devtools.build.skyframe; /** A return value of a {@code SkyFunction}. */ -public interface SkyValue { - - /** - * Returns true for values that can be reused across builds. Some values are inherently "flaky", - * like test statuses or stamping information, and in certain circumstances, those values cannot - * be shared across builds/servers. - */ - default boolean dataIsShareable() { - return true; - } -} +public interface SkyValue {} diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java index 07e50b758ae433..77f8d209b8d2f5 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java @@ -96,8 +96,7 @@ public void testRunfilesSingleFile() throws Exception { FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); mockCache.put( artifact, - FileArtifactValue.createForNormalFile( - FAKE_DIGEST, /*proxy=*/ null, 0L, /*isShareable=*/ true)); + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 0L)); expander.addRunfilesToInputs( inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); @@ -115,8 +114,7 @@ public void testRunfilesWithFileset() throws Exception { FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); mockCache.put( artifact, - FileArtifactValue.createForNormalFile( - FAKE_DIGEST, /*proxy=*/ null, 0L, /*isShareable=*/ true)); + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 0L)); ArtifactExpander filesetExpander = new ArtifactExpander() { @@ -206,12 +204,10 @@ public void testRunfilesTwoFiles() throws Exception { FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); mockCache.put( artifact1, - FileArtifactValue.createForNormalFile( - FAKE_DIGEST, /*proxy=*/ null, 1L, /*isShareable=*/ true)); + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 1L)); mockCache.put( artifact2, - FileArtifactValue.createForNormalFile( - FAKE_DIGEST, /*proxy=*/ null, 12L, /*isShareable=*/ true)); + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 12L)); expander.addRunfilesToInputs( inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); @@ -237,8 +233,7 @@ public void testRunfilesSymlink() throws Exception { FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); mockCache.put( artifact, - FileArtifactValue.createForNormalFile( - FAKE_DIGEST, /*proxy=*/ null, 1L, /*isShareable=*/ true)); + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 1L)); expander.addRunfilesToInputs( inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); @@ -262,8 +257,7 @@ public void testRunfilesRootSymlink() throws Exception { FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); mockCache.put( artifact, - FileArtifactValue.createForNormalFile( - FAKE_DIGEST, /*proxy=*/ null, 1L, /*isShareable=*/ true)); + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 1L)); expander.addRunfilesToInputs( inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); diff --git a/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java b/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java index 76dfc498bc48cf..d29645ca650b44 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java +++ b/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java @@ -47,10 +47,7 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { Path path = execRoot.getRelative(input.getExecPath()); FileStatus stat = path.stat(Symlinks.FOLLOW); return FileArtifactValue.createForNormalFile( - HashCode.fromString(hexDigest).asBytes(), - FileContentsProxy.create(stat), - stat.getSize(), - /*isShareable=*/ true); + HashCode.fromString(hexDigest).asBytes(), FileContentsProxy.create(stat), stat.getSize()); } @Override @@ -58,13 +55,13 @@ public ActionInput getInput(String execPath) { throw new UnsupportedOperationException(); } - void setDigest(ActionInput input, String digest) { + private void setDigest(ActionInput input, String digest) { cas.put(input, digest); } public Digest createScratchInput(ActionInput input, String content) throws IOException { Path inputFile = execRoot.getRelative(input.getExecPath()); - FileSystemUtils.createDirectoryAndParents(inputFile.getParentDirectory()); + inputFile.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeContentAsLatin1(inputFile, content); Digest digest = digestUtil.compute(inputFile); setDigest(input, digest.getHash()); @@ -73,7 +70,7 @@ public Digest createScratchInput(ActionInput input, String content) throws IOExc public Digest createScratchInputDirectory(ActionInput input, Tree content) throws IOException { Path inputFile = execRoot.getRelative(input.getExecPath()); - FileSystemUtils.createDirectoryAndParents(inputFile); + inputFile.createDirectoryAndParents(); Digest digest = digestUtil.compute(content); setDigest(input, digest.getHash()); return digest; diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 85f324dd8c42d9..a34bb4806f7387 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -28,7 +29,6 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; -import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -41,26 +41,21 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.Mock; -import org.mockito.MockitoAnnotations; /** Tests for {@link RemoteActionFileSystem} */ @RunWith(JUnit4.class) -public class RemoteActionFileSystemTest { +public final class RemoteActionFileSystemTest { private static final DigestHashFunction HASH_FUNCTION = DigestHashFunction.SHA256; - @Mock private RemoteActionInputFetcher inputFetcher; - private FileSystem fs; - private Path execRoot; - private ArtifactRoot outputRoot; + private final RemoteActionInputFetcher inputFetcher = mock(RemoteActionInputFetcher.class); + private final FileSystem fs = new InMemoryFileSystem(HASH_FUNCTION); + private final Path execRoot = fs.getPath("/exec"); + private final ArtifactRoot outputRoot = + ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "out"); @Before - public void setUp() throws IOException { - MockitoAnnotations.initMocks(this); - fs = new InMemoryFileSystem(new JavaClock(), HASH_FUNCTION); - execRoot = fs.getPath("/exec"); - outputRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "out"); + public void createOutputRoot() throws IOException { outputRoot.getRoot().asPath().createDirectoryAndParents(); } @@ -188,8 +183,7 @@ private Artifact createLocalArtifact(String pathFragment, String contents, Actio // Caution: there's a race condition between stating the file and computing the // digest. We need to stat first, since we're using the stat to detect changes. // We follow symlinks here to be consistent with getDigest. - inputs.putWithNoDepOwner( - a, FileArtifactValue.createFromStat(path, path.stat(Symlinks.FOLLOW), true)); + inputs.putWithNoDepOwner(a, FileArtifactValue.createFromStat(path, path.stat(Symlinks.FOLLOW))); return a; } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java index 938b8fbe101a7e..0a3130966b1d19 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java @@ -256,12 +256,8 @@ private static ActionExecutionValue createActionExecutionValue( private static ActionExecutionValue createActionExecutionValue( ImmutableMap fileArtifacts, ImmutableMap treeArtifacts) { - return ActionExecutionValue.create( - fileArtifacts, - treeArtifacts, - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*shareable=*/ true); + return ActionExecutionValue.createForTesting( + fileArtifacts, treeArtifacts, /*outputSymlinks=*/ null); } private static void createFile(Path file) throws IOException { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index a2e46f3f681a36..354447977136d0 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -106,8 +106,7 @@ private ActionMetadataHandler createHandler( public void withNonArtifactInput() throws Exception { ActionInput input = ActionInputHelper.fromPath("foo/bar"); FileArtifactValue metadata = - FileArtifactValue.createForNormalFile( - new byte[] {1, 2, 3}, /*proxy=*/ null, 10L, /*isShareable=*/ true); + FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /*proxy=*/ null, /*size=*/ 10L); ActionInputMap map = new ActionInputMap(1); map.putWithNoDepOwner(input, metadata); assertThat(map.getMetadata(input)).isEqualTo(metadata); @@ -122,8 +121,7 @@ public void withArtifactInput() throws Exception { PathFragment path = PathFragment.create("src/a"); Artifact artifact = ActionsTestUtil.createArtifactWithRootRelativePath(sourceRoot, path); FileArtifactValue metadata = - FileArtifactValue.createForNormalFile( - new byte[] {1, 2, 3}, /*proxy=*/ null, 10L, /*isShareable=*/ true); + FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /*proxy=*/ null, /*size=*/ 10L); ActionInputMap map = new ActionInputMap(1); map.putWithNoDepOwner(artifact, metadata); ActionMetadataHandler handler = @@ -411,10 +409,7 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { // child is missing, getExistingFileArtifactValue will throw. ActionExecutionValue actionExecutionValue = ActionExecutionValue.createFromOutputStore( - handler.getOutputStore(), - /*outputSymlinks=*/ null, - new NullAction(), - ActionsTestUtil.NULL_ACTION_LOOKUP_DATA); + handler.getOutputStore(), /*outputSymlinks=*/ null, new NullAction()); tree.getChildren().forEach(actionExecutionValue::getExistingFileArtifactValue); } 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 37edcb07c5dc7a..a727496e98899d 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 @@ public final void setUp() { 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 @@ public void actionExecutionValueSerialization() throws Exception { 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 static void assertValueMatches(FileStatus file, byte[] digest, FileArtif } } - 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 result = evaluate(ImmutableList.of(key).toArray(new SkyKey[0])); if (result.hasError()) { @@ -520,8 +518,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept } 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); } } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java index eac821f1f23d9c..0435a19691606f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java @@ -65,34 +65,16 @@ public void testEqualsAndHashCode() { new EqualsTester() .addEqualityGroup( FileArtifactValue.createForNormalFile( - toBytes("00112233445566778899AABBCCDDEEFF"), - /*proxy=*/ null, - 1L, - /*isShareable=*/ true), + toBytes("00112233445566778899AABBCCDDEEFF"), /*proxy=*/ null, 1L), FileArtifactValue.createForNormalFile( - toBytes("00112233445566778899AABBCCDDEEFF"), - /*proxy=*/ null, - 1L, - /*isShareable=*/ true)) + toBytes("00112233445566778899AABBCCDDEEFF"), /*proxy=*/ null, 1L)) .addEqualityGroup( FileArtifactValue.createForNormalFile( - toBytes("00112233445566778899AABBCCDDEEFF"), - /*proxy=*/ null, - 2L, - /*isShareable=*/ true)) + toBytes("00112233445566778899AABBCCDDEEFF"), /*proxy=*/ null, 2L)) .addEqualityGroup(FileArtifactValue.createForDirectoryWithMtime(1)) .addEqualityGroup( FileArtifactValue.createForNormalFile( - toBytes("FFFFFF00000000000000000000000000"), - /*proxy=*/ null, - 1L, - /*isShareable=*/ true)) - .addEqualityGroup( - FileArtifactValue.createForNormalFile( - toBytes("FFFFFF00000000000000000000000000"), - /*proxy=*/ null, - 1L, - /*isShareable=*/ false)) + toBytes("FFFFFF00000000000000000000000000"), /*proxy=*/ null, 1L)) .addEqualityGroup( FileArtifactValue.createForDirectoryWithMtime(2), FileArtifactValue.createForDirectoryWithMtime(2)) @@ -152,7 +134,7 @@ public void testNoMtimeIfNonemptyFile() throws Exception { assertThrows( "mtime for non-empty file should not be stored.", UnsupportedOperationException.class, - () -> value.getModifiedTime()); + value::getModifiedTime); } @Test @@ -174,12 +156,12 @@ public void testEmptyFile() throws Exception { assertThrows( "mtime for non-empty file should not be stored.", UnsupportedOperationException.class, - () -> value.getModifiedTime()); + value::getModifiedTime); } @Test public void testIOException() throws Exception { - final IOException exception = new IOException("beep"); + IOException exception = new IOException("beep"); FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override @@ -188,6 +170,7 @@ public byte[] getDigest(PathFragment path) throws IOException { } @Override + @SuppressWarnings("UnsynchronizedOverridesSynchronized") protected byte[] getFastDigest(PathFragment path) throws IOException { throw exception; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 856a10cc577c34..05d81afc0745d6 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -181,7 +181,7 @@ public void setUp() throws Exception { new ExternalPackageFunction(BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); differencer = new SequencedRecordingDifferencer(); - evaluator = new InMemoryMemoizingEvaluator(skyFunctions.build(), differencer); + evaluator = new InMemoryMemoizingEvaluator(skyFunctions.buildOrThrow(), differencer); driver = new SequentialBuildDriver(evaluator); PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID()); PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); @@ -930,39 +930,28 @@ private static ActionExecutionValue actionValue(Action action) { 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); } catch (IOException e) { throw new IllegalStateException(e); } } - return ActionExecutionValue.create( + return ActionExecutionValue.createForTesting( ImmutableMap.copyOf(artifactData), /*treeArtifactData=*/ ImmutableMap.of(), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*shareable=*/ true); + /*outputSymlinks=*/ null); } private static ActionExecutionValue actionValueWithTreeArtifact( SpecialArtifact output, TreeArtifactValue tree) { - return ActionExecutionValue.create( - ImmutableMap.of(), - ImmutableMap.of(output, tree), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*shareable=*/ true); + return ActionExecutionValue.createForTesting( + ImmutableMap.of(), ImmutableMap.of(output, tree), /*outputSymlinks=*/ null); } private static ActionExecutionValue actionValueWithRemoteArtifact( Artifact output, RemoteFileArtifactValue value) { - return ActionExecutionValue.create( - ImmutableMap.of(output, value), - ImmutableMap.of(), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*actionDependsOnBuildId=*/ false); + return ActionExecutionValue.createForTesting( + ImmutableMap.of(output, value), ImmutableMap.of(), /*outputSymlinks=*/ null); } private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java index ba071247971246..8f60dac29e42ab 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java @@ -108,12 +108,10 @@ static ActionExecutionValue actionValueWithTreeArtifacts( Map treeArtifactData = new HashMap<>(); treeArtifacts.injectTo(treeArtifactData::put); - return ActionExecutionValue.create( + return ActionExecutionValue.createForTesting( /*artifactData=*/ ImmutableMap.of(), ImmutableMap.copyOf(treeArtifactData), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*shareable=*/ true); + /*outputSymlinks=*/ null); } private static FileArtifactValue createMetadataFromFileSystem(Artifact artifact) @@ -122,8 +120,7 @@ private static FileArtifactValue createMetadataFromFileSystem(Artifact artifact) FileArtifactValue noDigest = ActionMetadataHandler.fileArtifactValueFromArtifact( artifact, FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)), null); - return FileArtifactValue.createFromInjectedDigest( - noDigest, path.getDigest(), !artifact.isConstantMetadata()); + return FileArtifactValue.createFromInjectedDigest(noDigest, path.getDigest()); } void writeFile(Path path, String... lines) throws IOException { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index c95ba3fc2210d1..6681cfd6aa09aa 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; -import com.google.common.base.Predicate; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -42,7 +41,6 @@ import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; -import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; @@ -107,7 +105,7 @@ private TreeArtifactValue doTestTreeArtifacts( @Test public void testEmptyTreeArtifacts() throws Exception { - TreeArtifactValue value = doTestTreeArtifacts(ImmutableList.of()); + TreeArtifactValue value = doTestTreeArtifacts(ImmutableList.of()); // Additional test, only for this test method: we expect the FileArtifactValue is equal to // the digest [0] assertThat(value.getMetadata().getDigest()).isEqualTo(value.getDigest()); @@ -140,14 +138,8 @@ public void testEqualTreeArtifacts() throws Exception { ImmutableList.of(PathFragment.create("one"), PathFragment.create("two")); TreeArtifactValue valueOne = evaluateTreeArtifact(treeArtifact, children); MemoizingEvaluator evaluator = driver.getGraphForTesting(); - evaluator.delete( - new Predicate() { - @Override - public boolean apply(SkyKey key) { - // Delete action execution node to force our artifacts to be re-evaluated. - return actions.contains(key.argument()); - } - }); + // Delete action execution node to force our artifacts to be re-evaluated. + evaluator.delete(key -> actions.contains(key.argument())); TreeArtifactValue valueTwo = evaluateTreeArtifact(treeArtifact, children); assertThat(valueOne.getDigest()).isNotSameInstanceAs(valueTwo.getDigest()); assertThat(valueOne).isEqualTo(valueTwo); @@ -209,7 +201,7 @@ public FileStatus statIfFound(PathFragment path, boolean followSymlinks) } private static void file(Path path, String contents) throws Exception { - FileSystemUtils.createDirectoryAndParents(path.getParentDirectory()); + path.getParentDirectory().createDirectoryAndParents(); writeFile(path, contents); } @@ -223,7 +215,7 @@ private SpecialArtifact createTreeArtifact(String path) throws IOException { ALL_OWNER, SpecialArtifactType.TREE); actions.add(new DummyAction(NestedSetBuilder.emptySet(Order.STABLE_ORDER), output)); - FileSystemUtils.createDirectoryAndParents(fullPath); + fullPath.createDirectoryAndParents(); return output; } @@ -282,20 +274,17 @@ public SkyValue compute(SkyKey skyKey, Environment env) FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)), null); FileArtifactValue withDigest = - FileArtifactValue.createFromInjectedDigest( - noDigest, path.getDigest(), !output.isConstantMetadata()); + FileArtifactValue.createFromInjectedDigest(noDigest, path.getDigest()); tree.putChild(suboutput, withDigest); } catch (IOException e) { throw new SkyFunctionException(e, Transience.TRANSIENT) {}; } } - return ActionExecutionValue.create( + return ActionExecutionValue.createForTesting( /*artifactData=*/ ImmutableMap.of(), ImmutableMap.of(output, tree.build()), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*shareable=*/ true); + /*outputSymlinks=*/ null); } } } diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java index a94b50127f52f5..a6d4d97ef4ee5f 100644 --- a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java +++ b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java @@ -26,7 +26,6 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.util.Pair; -import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import java.util.HashMap; import java.util.LinkedHashMap; @@ -197,7 +196,7 @@ public TestFunction addDependency(String name) { } public TestFunction addDependency(SkyKey key) { - deps.add(Pair.of(key, null)); + deps.add(Pair.of(key, null)); return this; } @@ -324,9 +323,7 @@ public void putSkyFunction(SkyFunctionName functionName, SkyFunction function) { functionMap.put(functionName, function); } - /** - * Simple value class that stores strings. - */ + /** Simple value class that stores strings. */ public static class StringValue implements SkyValue { protected final String value; @@ -340,6 +337,9 @@ public String getValue() { @Override public boolean equals(Object o) { + if (this == o) { + return true; + } if (!(o instanceof StringValue)) { return false; } @@ -353,7 +353,7 @@ public int hashCode() { @Override public String toString() { - return "StringValue: " + getValue(); + return "StringValue: " + value; } public static StringValue of(String string) { @@ -384,18 +384,6 @@ public int hashCode() { } } - /** An unshareable version of {@link StringValue}. */ - public static final class UnshareableStringValue extends StringValue { - public UnshareableStringValue(String value) { - super(value); - } - - @Override - public boolean dataIsShareable() { - return false; - } - } - /** * A callback interface to provide the value computation. */ @@ -405,32 +393,20 @@ SkyValue compute(Map deps, SkyFunction.Environment env) throws InterruptedException; } - public static final ValueComputer COPY = new ValueComputer() { - @Override - public SkyValue compute(Map deps, SkyFunction.Environment env) { - return Iterables.getOnlyElement(deps.values()); - } - }; + public static final ValueComputer COPY = (deps, env) -> Iterables.getOnlyElement(deps.values()); - public static final ValueComputer CONCATENATE = new ValueComputer() { - @Override - public SkyValue compute(Map deps, SkyFunction.Environment env) { - StringBuilder result = new StringBuilder(); - for (SkyValue value : deps.values()) { - result.append(((StringValue) value).value); - } - return new StringValue(result.toString()); - } - }; + public static final ValueComputer CONCATENATE = + (deps, env) -> { + StringBuilder result = new StringBuilder(); + for (SkyValue value : deps.values()) { + result.append(((StringValue) value).value); + } + return new StringValue(result.toString()); + }; - public static ValueComputer formatter(final SkyKey key, final String format) { - return new ValueComputer() { - @Override - public SkyValue compute(Map deps, Environment env) - throws InterruptedException { - return StringValue.of(String.format(format, StringValue.from(deps.get(key)).getValue())); - } - }; + public static ValueComputer formatter(SkyKey key, String format) { + return (deps, env) -> + StringValue.of(String.format(format, StringValue.from(deps.get(key)).getValue())); } @AutoCodec.VisibleForSerialization