aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java75
-rw-r--r--src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java52
2 files changed, 110 insertions, 17 deletions
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 9df851b0d4..6f78572939 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
@@ -14,9 +14,12 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Range;
import com.google.common.collect.Sets;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -50,6 +53,7 @@ import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.NavigableSet;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutorService;
@@ -189,14 +193,31 @@ public class FilesystemValueChecker {
modifiedOutputFilesCounter.set(0);
modifiedOutputFilesIntraBuildCounter.set(0);
- ImmutableSet<PathFragment> knownModifiedOutputFiles =
+ final ImmutableSet<PathFragment> knownModifiedOutputFiles =
modifiedOutputFiles == ModifiedFileSet.EVERYTHING_MODIFIED
? null
: modifiedOutputFiles.modifiedSourceFiles();
+
+ // Initialized lazily through a supplier because it is only used to check modified
+ // TreeArtifacts, which are not frequently used in builds.
+ Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles =
+ Suppliers.memoize(new Supplier<NavigableSet<PathFragment>>() {
+ @Override
+ public NavigableSet<PathFragment> get() {
+ if (knownModifiedOutputFiles == null) {
+ return null;
+ } else {
+ return ImmutableSortedSet.copyOf(knownModifiedOutputFiles);
+ }
+ }
+ });
+
for (List<Pair<SkyKey, ActionExecutionValue>> shard : outputShards) {
Runnable job = (batchStatter == null)
- ? outputStatJob(dirtyKeys, shard, knownModifiedOutputFiles)
- : batchStatJob(dirtyKeys, shard, batchStatter, knownModifiedOutputFiles);
+ ? outputStatJob(dirtyKeys, shard, knownModifiedOutputFiles,
+ sortedKnownModifiedOutputFiles)
+ : batchStatJob(dirtyKeys, shard, batchStatter, knownModifiedOutputFiles,
+ sortedKnownModifiedOutputFiles);
executor.submit(wrapper.wrap(job));
}
@@ -211,7 +232,8 @@ public class FilesystemValueChecker {
private Runnable batchStatJob(final Collection<SkyKey> dirtyKeys,
final List<Pair<SkyKey, ActionExecutionValue>> shard,
- final BatchStat batchStatter, final ImmutableSet<PathFragment> knownModifiedOutputFiles) {
+ final BatchStat batchStatter, final ImmutableSet<PathFragment> knownModifiedOutputFiles,
+ final Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles) {
return new Runnable() {
@Override
public void run() {
@@ -229,14 +251,10 @@ public class FilesystemValueChecker {
}
}
- // TreeArtifacts are always checked because we can't match modified files to modified
- // TreeArtifacts. We could construct a sorted map to do this, but it's unclear
- // whether this is a performance savings, since we expect the ratio of ordinary
- // files to TreeArtifacts directories and subdirectories to be rather high.
- // TODO(bazel-team): Investigate whether we can use modified output file awareness
- // to speed this up.
for (Artifact artifact : actionValue.getAllTreeArtifactValues().keySet()) {
- treeArtifactsToKeyAndValue.put(artifact, keyAndValue);
+ if (shouldCheckTreeArtifact(sortedKnownModifiedOutputFiles.get(), artifact)) {
+ treeArtifactsToKeyAndValue.put(artifact, keyAndValue);
+ }
}
}
}
@@ -249,7 +267,8 @@ public class FilesystemValueChecker {
} 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, knownModifiedOutputFiles).run();
+ outputStatJob(dirtyKeys, shard, knownModifiedOutputFiles, sortedKnownModifiedOutputFiles)
+ .run();
return;
} catch (InterruptedException e) {
// We handle interrupt in the main thread.
@@ -318,14 +337,16 @@ public class FilesystemValueChecker {
private Runnable outputStatJob(final Collection<SkyKey> dirtyKeys,
final List<Pair<SkyKey, ActionExecutionValue>> shard,
- final ImmutableSet<PathFragment> knownModifiedOutputFiles) {
+ final ImmutableSet<PathFragment> knownModifiedOutputFiles,
+ final Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles) {
return new Runnable() {
@Override
public void run() {
for (Pair<SkyKey, ActionExecutionValue> keyAndValue : shard) {
ActionExecutionValue value = keyAndValue.getSecond();
if (value == null
- || actionValueIsDirtyWithDirectSystemCalls(value, knownModifiedOutputFiles)) {
+ || actionValueIsDirtyWithDirectSystemCalls(
+ value, knownModifiedOutputFiles, sortedKnownModifiedOutputFiles)) {
dirtyKeys.add(keyAndValue.getFirst());
}
}
@@ -363,7 +384,8 @@ public class FilesystemValueChecker {
}
private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue actionValue,
- ImmutableSet<PathFragment> knownModifiedOutputFiles) {
+ ImmutableSet<PathFragment> knownModifiedOutputFiles,
+ Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles) {
boolean isDirty = false;
for (Map.Entry<Artifact, FileValue> entry : actionValue.getAllFileValues().entrySet()) {
Artifact file = entry.getKey();
@@ -390,7 +412,9 @@ public class FilesystemValueChecker {
for (Map.Entry<Artifact, TreeArtifactValue> entry :
actionValue.getAllTreeArtifactValues().entrySet()) {
Artifact artifact = entry.getKey();
- if (treeArtifactIsDirty(artifact, entry.getValue())) {
+
+ if (shouldCheckTreeArtifact(sortedKnownModifiedOutputFiles.get(), artifact)
+ && treeArtifactIsDirty(artifact, entry.getValue())) {
Path path = artifact.getPath();
// Count the changed directory as one "file".
try {
@@ -415,6 +439,25 @@ public class FilesystemValueChecker {
|| knownModifiedOutputFiles.contains(artifact.getExecPath());
}
+ private static boolean shouldCheckTreeArtifact(
+ @Nullable NavigableSet<PathFragment> knownModifiedOutputFiles, Artifact treeArtifact) {
+ // If null, everything needs to be checked.
+ if (knownModifiedOutputFiles == null) {
+ return true;
+ }
+
+ // Here we do the following to see whether a TreeArtifact is modified:
+ // 1. Sort the set of modified file paths in lexicographical order using TreeSet.
+ // 2. Get the first modified output file path that is greater than or equal to the exec path of
+ // the TreeArtifact to check.
+ // 3. Check whether the returned file path contains the exec path of the TreeArtifact as a
+ // prefix path.
+ PathFragment artifactExecPath = treeArtifact.getExecPath();
+ PathFragment headPath = knownModifiedOutputFiles.ceiling(artifactExecPath);
+
+ return headPath != null && headPath.startsWith(artifactExecPath);
+ }
+
private BatchDirtyResult getDirtyValues(ValueFetcher fetcher,
Iterable<SkyKey> keys, final SkyValueDirtinessChecker checker,
final boolean checkMissingValues) throws InterruptedException {
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 b457c00aac..54b65cbe98 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
@@ -411,6 +411,9 @@ public class FilesystemValueCheckerTest {
Artifact outUnchanging = createTreeArtifact("untouched");
FileSystemUtils.createDirectoryAndParents(outUnchanging.getPath());
+ Artifact last = createTreeArtifact("zzzzzzzzzz");
+ FileSystemUtils.createDirectoryAndParents(last.getPath());
+
Action action1 =
new TestAction(
Runnables.doNothing(), ImmutableSet.<Artifact>of(), ImmutableSet.of(out1));
@@ -423,6 +426,9 @@ public class FilesystemValueCheckerTest {
Action actionUnchanging =
new TestAction(
Runnables.doNothing(), ImmutableSet.<Artifact>of(), ImmutableSet.of(outUnchanging));
+ Action lastAction =
+ new TestAction(
+ Runnables.doNothing(), ImmutableSet.<Artifact>of(), ImmutableSet.of(last));
differencer.inject(
ImmutableMap.<SkyKey, SkyValue>of(
ActionExecutionValue.key(action1),
@@ -432,7 +438,9 @@ public class FilesystemValueCheckerTest {
ActionExecutionValue.key(actionEmpty),
actionValueWithEmptyDirectory(outEmpty),
ActionExecutionValue.key(actionUnchanging),
- actionValueWithEmptyDirectory(outUnchanging)));
+ actionValueWithEmptyDirectory(outUnchanging),
+ ActionExecutionValue.key(lastAction),
+ actionValueWithEmptyDirectory(last)));
assertFalse(
driver
@@ -510,6 +518,48 @@ public class FilesystemValueCheckerTest {
.build()))
.containsExactly(ActionExecutionValue.key(action1), ActionExecutionValue.key(action2),
ActionExecutionValue.key(actionEmpty));
+ // We also check that if the modified file set does not contain our modified files on disk,
+ // we are not going to check and return them.
+ assertThat(
+ new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter,
+ new ModifiedFileSet.Builder()
+ .modify(file21.getExecPath())
+ .modify(outEmptyNew.getExecPath())
+ .build()))
+ .containsExactly(ActionExecutionValue.key(action2), ActionExecutionValue.key(actionEmpty));
+ assertThat(
+ new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter,
+ new ModifiedFileSet.Builder()
+ .modify(file21.getExecPath())
+ .modify(out1new.getExecPath())
+ .build()))
+ .containsExactly(ActionExecutionValue.key(action1), ActionExecutionValue.key(action2));
+ // Check modifying the last (lexicographically) tree artifact.
+ last.getPath().delete();
+ assertThat(
+ new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter,
+ new ModifiedFileSet.Builder()
+ .modify(file21.getExecPath())
+ .modify(out1new.getExecPath())
+ .modify(last.getExecPath())
+ .build()))
+ .containsExactly(ActionExecutionValue.key(action1), ActionExecutionValue.key(action2),
+ ActionExecutionValue.key(lastAction));
+ // Check ModifiedFileSet without the last (lexicographically) tree artifact.
+ assertThat(
+ new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(),
+ batchStatter,
+ new ModifiedFileSet.Builder()
+ .modify(file21.getExecPath())
+ .modify(out1new.getExecPath())
+ .build()))
+ .containsExactly(ActionExecutionValue.key(action1), ActionExecutionValue.key(action2));
+ // Restore
+ last.getPath().delete();
+ FileSystemUtils.createDirectoryAndParents(last.getPath());
// We add a test for NOTHING_MODIFIED, because FileSystemValueChecker doesn't
// pay attention to file sets for TreeArtifact directory listings.
assertThat(
generated by cgit v1.2.3 (git 2.39.1) at 2024-07-03 10:43:42 +0000