Skip to content

Commit

Permalink
Teach the FilesystemValueChecker about remotely stored outputs
Browse files Browse the repository at this point in the history 
The FilesystemValueChecker is used by Bazel to detect modified outputs before
running a command. This change teaches it about remote output files that don't
exist in the output base, but are only stored physically on a remote storage
system with SkyFrame only tracking file metadata.

Note that this change only affects incremental builds, and there
introduces the following new behavior for artifacts and tree artifacts:
  1) If the previous build created a remote output and the output
 does not exist in the output base then no invalidation happens.
  2) If the previous build created a remote output and the output
 does exist in the output base then the generating action is
 invalidated.

A tree artifact may either be fully materialized in the output base or
not materialized at all.

Progress towards #6862

Closes #7269.

PiperOrigin-RevId: 237422752
  • Loading branch information
@buchgr @copybara-github
buchgr authored and copybara-github committed Mar 8, 2019
1 parent 7f72544 commit 0a0c962
Show file tree
Hide file tree
Showing 4 changed files with 208 additions and 12 deletions.
14 changes: 13 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
* <li>a "middleman marker" object, which has a null digest, 0 size, and mtime of 0.
* <li>The "self data" of a TreeArtifact, where we would expect to see a digest representing the
* artifact's contents, and a size of 0.
* <li>a file that's only stored by a remote caching/execution system, in which case we would
* expect to see a digest and size.
* </ul>
*/
@Immutable
Expand Down Expand Up @@ -138,6 +140,11 @@ 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) {
Expand Down Expand Up @@ -524,7 +531,12 @@ public int getLocationIndex() {

@Override
public boolean wasModifiedSinceDigest(Path path) {
throw new UnsupportedOperationException();
return false;
}

@Override
public boolean isRemote() {
return true;
}

@Override
Expand Down
12 changes: 8 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -80,7 +81,7 @@ public class FilesystemValueChecker {
private static final Predicate<SkyKey> ACTION_FILTER =
SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION);

private final TimestampGranularityMonitor tsgm;
@Nullable private final TimestampGranularityMonitor tsgm;
@Nullable
private final Range<Long> lastExecutionTimeRange;
private AtomicInteger modifiedOutputFilesCounter = new AtomicInteger(0);
Expand Down Expand Up @@ -398,8 +399,8 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value)
try {
Set<PathFragment> currentDirectoryValue =
TreeArtifactValue.explodeDirectory(artifact.getPath());
Set<PathFragment> valuePaths = value.getChildPaths();
return !currentDirectoryValue.equals(valuePaths);
return !(currentDirectoryValue.isEmpty() && value.isRemote())
&& !currentDirectoryValue.equals(value.getChildPaths());
} catch (IOException e) {
return true;
}
Expand All @@ -417,7 +418,10 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act
try {
ArtifactFileMetadata fileMetadata =
ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm);
if (!fileMetadata.equals(lastKnownData)) {
FileArtifactValue fileValue = actionValue.getArtifactValue(file);
boolean lastSeenRemotely = (fileValue != null) && fileValue.isRemote();
boolean trustRemoteValue = !fileMetadata.exists() && lastSeenRemotely;
if (!trustRemoteValue && !fileMetadata.equals(lastKnownData)) {
updateIntraBuildModifiedCounter(
fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1,
lastKnownData.isSymlink(),
Expand Down
45 changes: 38 additions & 7 deletions src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.google.common.base.Function;
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
Expand Down Expand Up @@ -53,30 +54,55 @@ public PathFragment apply(Artifact artifact) {
}
};

private static final TreeArtifactValue EMPTY =
new TreeArtifactValue(
DigestUtils.fromMetadata(ImmutableMap.of()).getDigestBytesUnsafe(),
ImmutableMap.of(),
/* remote= */ false);

private final byte[] digest;
private final Map<TreeFileArtifact, FileArtifactValue> childData;
private BigInteger valueFingerprint;
private final boolean remote;

@AutoCodec.VisibleForSerialization
TreeArtifactValue(byte[] digest, Map<TreeFileArtifact, FileArtifactValue> childData) {
TreeArtifactValue(
byte[] digest, Map<TreeFileArtifact, FileArtifactValue> childData, boolean remote) {
this.digest = digest;
this.childData = ImmutableMap.copyOf(childData);
this.remote = remote;
}

/**
* Returns a TreeArtifactValue out of the given Artifact-relative path fragments
* and their corresponding FileArtifactValues.
* Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their
* corresponding FileArtifactValues.
*
* <p>All {@code childFileValues} must return the same value for {@link
* FileArtifactValue#isRemote()}.
*/
static TreeArtifactValue create(Map<TreeFileArtifact, FileArtifactValue> childFileValues) {
if (childFileValues.isEmpty()) {
return EMPTY;
}
Map<String, FileArtifactValue> digestBuilder =
Maps.newHashMapWithExpectedSize(childFileValues.size());
Boolean remote = null;
for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) {
digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue());
FileArtifactValue value = e.getValue();
if (remote == null) {
remote = value.isRemote();
}
Preconditions.checkArgument(
value.isRemote() == remote,
"files in a tree artifact must either be all remote or all local. '%v', remote=%b",
value,
value.isRemote());
digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), value);
}

return new TreeArtifactValue(
DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(),
ImmutableMap.copyOf(childFileValues));
ImmutableMap.copyOf(childFileValues),
Preconditions.checkNotNull(remote));
}

FileArtifactValue getSelfData() {
Expand Down Expand Up @@ -104,6 +130,11 @@ Map<TreeFileArtifact, FileArtifactValue> getChildValues() {
return childData;
}

/** Returns true if the {@link TreeFileArtifact}s are only stored remotely. */
public boolean isRemote() {
return remote;
}

@Override
public BigInteger getValueFingerprint() {
if (valueFingerprint == null) {
Expand Down Expand Up @@ -150,7 +181,7 @@ public String toString() {
* Java's concurrent collections disallow null members.
*/
static final TreeArtifactValue MISSING_TREE_ARTIFACT =
new TreeArtifactValue(null, ImmutableMap.<TreeFileArtifact, FileArtifactValue>of()) {
new TreeArtifactValue(null, ImmutableMap.<TreeFileArtifact, FileArtifactValue>of(), false) {
@Override
FileArtifactValue getSelfData() {
throw new UnsupportedOperationException();
Expand Down
149 changes: 149 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,6 +34,7 @@
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.util.TestAction;
Expand All @@ -50,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;
Expand Down Expand Up @@ -77,6 +81,7 @@
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;
Expand Down Expand Up @@ -787,6 +792,150 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List<TreeFileArtifact>
/*actionDependsOnBuildId=*/ false);
}

private ActionExecutionValue actionValueWithRemoteTreeArtifact(
SpecialArtifact output, Map<PathFragment, RemoteFileArtifactValue> children) {
ImmutableMap.Builder<TreeFileArtifact, FileArtifactValue> childFileValues =
ImmutableMap.builder();
for (Map.Entry<PathFragment, RemoteFileArtifactValue> 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(),
/* outputSymlinks= */ null,
/* discoveredModules= */ null,
/* actionDependsOnBuildId= */ false);
}

private ActionExecutionValue actionValueWithRemoteArtifact(
Artifact output, RemoteFileArtifactValue value) {
return ActionExecutionValue.create(
Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER),
ImmutableMap.of(),
Collections.singletonMap(output, value),
/* outputSymlinks= */ null,
/* discoveredModules= */ null,
/* actionDependsOnBuildId= */ 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<SkyKey, SkyValue> 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
.getDirtyActionValues(
evaluator.getValues(),
/* batchStatter= */ 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
.getDirtyActionValues(
evaluator.getValues(),
/* batchStatter= */ 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<PathFragment, RemoteFileArtifactValue> treeArtifactMetadata = new HashMap<>();
treeArtifactMetadata.put(
PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content"));
treeArtifactMetadata.put(
PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content"));

Map<SkyKey, SkyValue> 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
.getDirtyActionValues(
evaluator.getValues(),
/* batchStatter= */ 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
.getDirtyActionValues(
evaluator.getValues(),
/* batchStatter= */ null,
ModifiedFileSet.EVERYTHING_MODIFIED))
.containsExactly(actionKey);
}

@Test
public void testPropagatesRuntimeExceptions() throws Exception {
Collection<SkyKey> values =
Expand Down

0 comments on commit 0a0c962

Please sign in to comment.