In FileSystemValueChecker, only check TreeArtfacts that are in the passed-in modified file sets. -- MOS_MIGRATED_REVID=125173579
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 9df851b..6f78572 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.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 @@ 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 @@ 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 @@ } } - // 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 @@ } 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 @@ 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 @@ } 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 @@ 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 @@ || 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 b457c00..54b65cb 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 @@ 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 @@ 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 @@ ActionExecutionValue.key(actionEmpty), actionValueWithEmptyDirectory(outEmpty), ActionExecutionValue.key(actionUnchanging), - actionValueWithEmptyDirectory(outUnchanging))); + actionValueWithEmptyDirectory(outUnchanging), + ActionExecutionValue.key(lastAction), + actionValueWithEmptyDirectory(last))); assertFalse( driver @@ -510,6 +518,48 @@ .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(