Skip to content

Commit

Permalink
Teach the FilesystemValueChecker about remote outputs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@buchgr
buchgr committed Mar 1, 2019
1 parent 972e73e commit 90c757c
Show file tree
Hide file tree
Showing 4 changed files with 157 additions and 4 deletions.
16 changes: 15 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,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) {
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 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,6 +81,7 @@ public class FilesystemValueChecker {
private static final Predicate<SkyKey> ACTION_FILTER =
SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION);

@Nullable
private final TimestampGranularityMonitor tsgm;
@Nullable
private final Range<Long> lastExecutionTimeRange;
Expand Down Expand Up @@ -398,6 +400,9 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value)
try {
Set<PathFragment> currentDirectoryValue =
TreeArtifactValue.explodeDirectory(artifact.getPath());
if (currentDirectoryValue.isEmpty() && value.isRemote()) {
return false;
}
Set<PathFragment> valuePaths = value.getChildPaths();
return !currentDirectoryValue.equals(valuePaths);
} catch (IOException e) {
Expand All @@ -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,
Expand Down
18 changes: 15 additions & 3 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 @@ -56,11 +56,13 @@ public PathFragment apply(Artifact artifact) {
private final byte[] digest;
private final Map<TreeFileArtifact, FileArtifactValue> childData;
private BigInteger valueFingerprint;
private final boolean isRemote;

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

/**
Expand All @@ -70,13 +72,16 @@ public PathFragment apply(Artifact artifact) {
static TreeArtifactValue create(Map<TreeFileArtifact, FileArtifactValue> childFileValues) {
Map<String, FileArtifactValue> digestBuilder =
Maps.newHashMapWithExpectedSize(childFileValues.size());
boolean isRemote = true;
for (Map.Entry<TreeFileArtifact, FileArtifactValue> 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() {
Expand Down Expand Up @@ -104,6 +109,13 @@ Map<TreeFileArtifact, FileArtifactValue> 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) {
Expand Down Expand Up @@ -150,7 +162,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
114 changes: 114 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,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;
Expand All @@ -50,6 +54,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 All @@ -74,9 +79,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;
Expand Down Expand Up @@ -787,6 +794,113 @@ 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(), 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<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(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<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(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<SkyKey> values =
Expand Down

0 comments on commit 90c757c

Please sign in to comment.