From a714e8d753cd9a504b6a67e2e37a5a660523ef36 Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Mon, 28 Jan 2019 13:12:45 +0100 Subject: [PATCH] Teach the FilesystemValueChecker about remote outputs The FilesystemValueChecker is used by Bazel to detect modified outputs before a command. This change teaches it about remote outputs that don't exist in the output base. That is if SkyFrame has metadata about a remote output file which does not exist in the output base it will not invalidate the output. However, if the file exists in the output base it will be taken as the source of truth. Progress towards #6862 --- .../build/lib/actions/FileArtifactValue.java | 16 ++- .../lib/skyframe/FilesystemValueChecker.java | 13 ++ .../build/lib/skyframe/TreeArtifactValue.java | 18 ++- .../skyframe/FilesystemValueCheckerTest.java | 114 ++++++++++++++++++ 4 files changed, 157 insertions(+), 4 deletions(-) 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 c7e4966e6a29b1..63bfa96158f41e 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 @@ -51,6 +51,8 @@ *
  • a "middleman marker" object, which has a null digest, 0 size, and mtime of 0. *
  • The "self data" of a TreeArtifact, where we would expect to see a digest representing the * artifact's contents, and a size of 0. + *
  • a file that's only stored by a remote caching/execution system, in which case we would + * expect to see a digest and size. * */ @Immutable @@ -138,6 +140,13 @@ public final boolean isMarkerValue() { */ public abstract boolean wasModifiedSinceDigest(Path path) throws IOException; + /** + * Returns {@code true} if the file only exists remotely. + */ + public boolean isRemote() { + return false; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -492,7 +501,12 @@ public int getLocationIndex() { @Override public boolean wasModifiedSinceDigest(Path path) { - throw new UnsupportedOperationException(); + return false; + } + + @Override + public boolean isRemote() { + return true; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 9efc2b67a78305..52736c1dee3ad1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -26,6 +26,7 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFileMetadata; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; @@ -80,6 +81,7 @@ public class FilesystemValueChecker { private static final Predicate ACTION_FILTER = SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION); + @Nullable private final TimestampGranularityMonitor tsgm; @Nullable private final Range lastExecutionTimeRange; @@ -398,6 +400,9 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) try { Set currentDirectoryValue = TreeArtifactValue.explodeDirectory(artifact.getPath()); + if (currentDirectoryValue.isEmpty() && value.isRemote()) { + return false; + } Set valuePaths = value.getChildPaths(); return !currentDirectoryValue.equals(valuePaths); } catch (IOException e) { @@ -417,6 +422,14 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act try { ArtifactFileMetadata fileMetadata = ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm); + FileArtifactValue fileValue = actionValue.getArtifactValue(file); + boolean lastSeenRemotely = fileValue != null && fileValue.isRemote(); + if (!fileMetadata.exists() && lastSeenRemotely) { + // The output file does not exist in the output tree, but the last time we created it + // it was stored on a remotely so there is no need to invalidate it. + continue; + } + if (!fileMetadata.equals(lastKnownData)) { updateIntraBuildModifiedCounter( fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 9221ebc61ff732..aa8413165d1fd5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -56,11 +56,13 @@ public PathFragment apply(Artifact artifact) { private final byte[] digest; private final Map childData; private BigInteger valueFingerprint; + private final boolean isRemote; @AutoCodec.VisibleForSerialization - TreeArtifactValue(byte[] digest, Map childData) { + TreeArtifactValue(byte[] digest, Map childData, boolean isRemote) { this.digest = digest; this.childData = ImmutableMap.copyOf(childData); + this.isRemote = isRemote; } /** @@ -70,13 +72,16 @@ public PathFragment apply(Artifact artifact) { static TreeArtifactValue create(Map childFileValues) { Map digestBuilder = Maps.newHashMapWithExpectedSize(childFileValues.size()); + boolean isRemote = true; for (Map.Entry e : childFileValues.entrySet()) { + isRemote = isRemote && e.getValue().isRemote(); digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue()); } return new TreeArtifactValue( DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(), - ImmutableMap.copyOf(childFileValues)); + ImmutableMap.copyOf(childFileValues), + isRemote); } FileArtifactValue getSelfData() { @@ -104,6 +109,13 @@ Map getChildValues() { return childData; } + /** + * Returns {@code true} if at least one {@link TreeFileArtifact} is only stored remotely. + */ + public boolean isRemote() { + return isRemote; + } + @Override public BigInteger getValueFingerprint() { if (valueFingerprint == null) { @@ -150,7 +162,7 @@ public String toString() { * Java's concurrent collections disallow null members. */ static final TreeArtifactValue MISSING_TREE_ARTIFACT = - new TreeArtifactValue(null, ImmutableMap.of()) { + new TreeArtifactValue(null, ImmutableMap.of(), false) { @Override FileArtifactValue getSelfData() { throw new UnsupportedOperationException(); 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 f35556c38d9829..6853f7ab19f07f 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 @@ -21,8 +21,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Runnables; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -32,8 +34,10 @@ import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ServerDirectories; @@ -49,6 +53,7 @@ import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.BatchStat; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; @@ -73,9 +78,11 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; +import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -786,6 +793,113 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List /*actionDependsOnBuildId=*/ false); } + private ActionExecutionValue actionValueWithRemoteTreeArtifact(SpecialArtifact output, Map children) { + ImmutableMap.Builder childFileValues = ImmutableMap.builder(); + for (Map.Entry child : children.entrySet()) { + childFileValues.put(ActionInputHelper.treeFileArtifact(output, child.getKey()), child.getValue()); + } + TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + return ActionExecutionValue.create(Collections.emptyMap(), + Collections.singletonMap(output, treeArtifactValue), ImmutableMap.of(), null, null, false); + } + + private ActionExecutionValue actionValueWithRemoteArtifact(Artifact output, RemoteFileArtifactValue value) { + return ActionExecutionValue.create( + Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(), + Collections.singletonMap(output, value), null, null, false); + } + + private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + byte[] data = contents.getBytes(); + DigestHashFunction hashFn = fs.getDigestFunction(); + HashCode hash = hashFn.getHashFunction().hashBytes(data); + return new RemoteFileArtifactValue(hash.asBytes(), data.length, -1); + } + + @Test + public void testRemoteAndLocalArtifacts() throws Exception { + // Test that injected remote artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey1 = ActionExecutionValue.key(actionLookupKey, 0); + SkyKey actionKey2 = ActionExecutionValue.key(actionLookupKey, 1); + + Artifact out1 = createDerivedArtifact("foo"); + Artifact out2 = createDerivedArtifact("bar"); + Map metadataToInject = new HashMap<>(); + metadataToInject.put(actionKey1, actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + metadataToInject.put(actionKey2, actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content"))); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext).hasError()).isFalse(); + assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(), + null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty(); + + // Create the "out1" artifact on the filesystem and test that it invalidates the generating + // action's SkyKey. + FileSystemUtils.writeContentAsLatin1(out1.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(null, null) + .getDirtyActionValues( + evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey1); + } + + @Test + public void testRemoteAndLocalTreeArtifacts() throws Exception { + // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey = ActionExecutionValue.key(actionLookupKey, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + Map treeArtifactMetadata = new HashMap<>(); + treeArtifactMetadata.put(PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content")); + treeArtifactMetadata.put(PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content")); + + Map metadataToInject = new HashMap<>(); + metadataToInject.put(actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata)); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()).isFalse(); + assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(), + null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty(); + + // Create dir/foo on the local disk and test that it invalidates the associated sky key. + TreeFileArtifact fooArtifact = treeFileArtifact(treeArtifact, "foo"); + FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(null, null) + .getDirtyActionValues( + evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey); + } + @Test public void testPropagatesRuntimeExceptions() throws Exception { Collection values =