Allow for a set of known modified files to be passed into the FileSystemValueChecker when checking for dirty actions.
--
MOS_MIGRATED_REVID=108046467
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 0e8a5cf..30088ef 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
@@ -17,6 +17,7 @@
import com.google.common.base.Predicate;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Range;
import com.google.common.collect.Sets;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -32,6 +33,8 @@
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.BatchStat;
import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.Differencer;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -152,8 +155,12 @@
* the on-disk file value (were modified externally).
*/
Collection<SkyKey> getDirtyActionValues(Map<SkyKey, SkyValue> valuesMap,
- @Nullable final BatchStat batchStatter) throws InterruptedException {
- // CPU-bound (usually) stat() calls, plus a fudge factor.
+ @Nullable final BatchStat batchStatter, ModifiedFileSet modifiedOutputFiles)
+ throws InterruptedException {
+ if (modifiedOutputFiles == ModifiedFileSet.NOTHING_MODIFIED) {
+ LOG.info("Not checking for dirty actions since nothing was modified");
+ return ImmutableList.of();
+ }
LOG.info("Accumulating dirty actions");
final int numOutputJobs = Runtime.getRuntime().availableProcessors() * 4;
final Set<SkyKey> actionSkyKeys = new HashSet<>();
@@ -180,10 +187,14 @@
modifiedOutputFilesCounter.set(0);
modifiedOutputFilesIntraBuildCounter.set(0);
+ ImmutableSet<PathFragment> knownModifiedOutputFiles =
+ modifiedOutputFiles == ModifiedFileSet.EVERYTHING_MODIFIED
+ ? null
+ : modifiedOutputFiles.modifiedSourceFiles();
for (List<Pair<SkyKey, ActionExecutionValue>> shard : outputShards) {
Runnable job = (batchStatter == null)
- ? outputStatJob(dirtyKeys, shard)
- : batchStatJob(dirtyKeys, shard, batchStatter);
+ ? outputStatJob(dirtyKeys, shard, knownModifiedOutputFiles)
+ : batchStatJob(dirtyKeys, shard, batchStatter, knownModifiedOutputFiles);
executor.submit(wrapper.wrap(job));
}
@@ -197,8 +208,8 @@
}
private Runnable batchStatJob(final Collection<SkyKey> dirtyKeys,
- final List<Pair<SkyKey, ActionExecutionValue>> shard,
- final BatchStat batchStatter) {
+ final List<Pair<SkyKey, ActionExecutionValue>> shard,
+ final BatchStat batchStatter, final ImmutableSet<PathFragment> knownModifiedOutputFiles) {
return new Runnable() {
@Override
public void run() {
@@ -209,7 +220,9 @@
dirtyKeys.add(keyAndValue.getFirst());
} else {
for (Artifact artifact : actionValue.getAllOutputArtifactData().keySet()) {
- artifactToKeyAndValue.put(artifact, keyAndValue);
+ if (shouldCheckArtifact(knownModifiedOutputFiles, artifact)) {
+ artifactToKeyAndValue.put(artifact, keyAndValue);
+ }
}
}
}
@@ -222,7 +235,7 @@
} catch (IOException e) {
// Batch stat did not work. Log an exception and fall back on system calls.
LoggingUtil.logToRemote(Level.WARNING, "Unable to process batch stat", e);
- outputStatJob(dirtyKeys, shard).run();
+ outputStatJob(dirtyKeys, shard, knownModifiedOutputFiles).run();
return;
} catch (InterruptedException e) {
// We handle interrupt in the main thread.
@@ -266,13 +279,15 @@
}
private Runnable outputStatJob(final Collection<SkyKey> dirtyKeys,
- final List<Pair<SkyKey, ActionExecutionValue>> shard) {
+ final List<Pair<SkyKey, ActionExecutionValue>> shard,
+ final ImmutableSet<PathFragment> knownModifiedOutputFiles) {
return new Runnable() {
@Override
public void run() {
for (Pair<SkyKey, ActionExecutionValue> keyAndValue : shard) {
ActionExecutionValue value = keyAndValue.getSecond();
- if (value == null || actionValueIsDirtyWithDirectSystemCalls(value)) {
+ if (value == null
+ || actionValueIsDirtyWithDirectSystemCalls(value, knownModifiedOutputFiles)) {
dirtyKeys.add(keyAndValue.getFirst());
}
}
@@ -292,30 +307,39 @@
return modifiedOutputFilesIntraBuildCounter.get();
}
- private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue actionValue) {
+ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue actionValue,
+ ImmutableSet<PathFragment> knownModifiedOutputFiles) {
boolean isDirty = false;
for (Map.Entry<Artifact, FileValue> entry :
actionValue.getAllOutputArtifactData().entrySet()) {
Artifact artifact = entry.getKey();
FileValue lastKnownData = entry.getValue();
- try {
- FileValue fileValue = ActionMetadataHandler.fileValueFromArtifact(artifact, null, tsgm);
- if (!fileValue.equals(lastKnownData)) {
- updateIntraBuildModifiedCounter(fileValue.exists()
- ? fileValue.realRootedPath().asPath().getLastModifiedTime()
- : -1, lastKnownData.isSymlink(), fileValue.isSymlink());
+ if (shouldCheckArtifact(knownModifiedOutputFiles, artifact)) {
+ try {
+ FileValue fileValue = ActionMetadataHandler.fileValueFromArtifact(artifact, null, tsgm);
+ if (!fileValue.equals(lastKnownData)) {
+ updateIntraBuildModifiedCounter(fileValue.exists()
+ ? fileValue.realRootedPath().asPath().getLastModifiedTime()
+ : -1, lastKnownData.isSymlink(), fileValue.isSymlink());
+ modifiedOutputFilesCounter.getAndIncrement();
+ isDirty = true;
+ }
+ } catch (IOException e) {
+ // This is an unexpected failure getting a digest or symlink target.
modifiedOutputFilesCounter.getAndIncrement();
isDirty = true;
}
- } catch (IOException e) {
- // This is an unexpected failure getting a digest or symlink target.
- modifiedOutputFilesCounter.getAndIncrement();
- isDirty = true;
}
}
return isDirty;
}
+ private static boolean shouldCheckArtifact(ImmutableSet<PathFragment> knownModifiedOutputFiles,
+ Artifact artifact) {
+ return knownModifiedOutputFiles == null
+ || knownModifiedOutputFiles.contains(artifact.getExecPath());
+ }
+
private BatchDirtyResult getDirtyValues(ValueFetcher fetcher,
Iterable<SkyKey> keys, final SkyValueDirtinessChecker checker,
final boolean checkMissingValues) throws InterruptedException {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 8b78370..f3e5d16 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1646,16 +1646,14 @@
InterruptedException {
maybeInjectEmbeddedArtifacts();
- if (modifiedOutputFiles != ModifiedFileSet.NOTHING_MODIFIED) {
- // Detect external modifications in the output tree.
- FilesystemValueChecker fsvc = new FilesystemValueChecker(tsgm, lastExecutionTimeRange);
- BatchStat batchStatter = outputService == null ? null : outputService.getBatchStatter();
- invalidateDirtyActions(fsvc.getDirtyActionValues(memoizingEvaluator.getValues(),
- batchStatter));
- modifiedFiles += fsvc.getNumberOfModifiedOutputFiles();
- outputDirtyFiles += fsvc.getNumberOfModifiedOutputFiles();
- modifiedFilesDuringPreviousBuild += fsvc.getNumberOfModifiedOutputFilesDuringPreviousBuild();
- }
+ // Detect external modifications in the output tree.
+ FilesystemValueChecker fsvc = new FilesystemValueChecker(tsgm, lastExecutionTimeRange);
+ BatchStat batchStatter = outputService == null ? null : outputService.getBatchStatter();
+ invalidateDirtyActions(fsvc.getDirtyActionValues(memoizingEvaluator.getValues(),
+ batchStatter, modifiedOutputFiles));
+ modifiedFiles += fsvc.getNumberOfModifiedOutputFiles();
+ outputDirtyFiles += fsvc.getNumberOfModifiedOutputFiles();
+ modifiedFilesDuringPreviousBuild += fsvc.getNumberOfModifiedOutputFilesDuringPreviousBuild();
informAboutNumberOfModifiedFiles();
}
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 2aed79a..d1d2018 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
@@ -38,6 +38,7 @@
import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -337,14 +338,28 @@
.evaluate(ImmutableList.<SkyKey>of(), false, 1, NullEventHandler.INSTANCE)
.hasError());
assertThat(new FilesystemValueChecker(tsgm, null).getDirtyActionValues(evaluator.getValues(),
- batchStatter)).isEmpty();
+ batchStatter, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty();
FileSystemUtils.writeContentAsLatin1(out1.getPath(), "goodbye");
assertEquals(
ActionExecutionValue.key(action1),
Iterables.getOnlyElement(
new FilesystemValueChecker(tsgm, null).getDirtyActionValues(evaluator.getValues(),
- batchStatter)));
+ batchStatter, ModifiedFileSet.EVERYTHING_MODIFIED)));
+ assertEquals(
+ ActionExecutionValue.key(action1),
+ Iterables.getOnlyElement(
+ new FilesystemValueChecker(tsgm, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter,
+ new ModifiedFileSet.Builder().modify(out1.getExecPath()).build())));
+ assertThat(
+ new FilesystemValueChecker(tsgm, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter,
+ new ModifiedFileSet.Builder().modify(
+ out1.getExecPath().getParentDirectory()).build())).isEmpty();
+ assertThat(
+ new FilesystemValueChecker(tsgm, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter, ModifiedFileSet.NOTHING_MODIFIED)).isEmpty();
}
private Artifact createDerivedArtifact(String relPath) throws IOException {
