Don't unconditionally update ExternalFilesHelper tracking data at the end of handleDiffsWithMissingDiffInformation: we may skip a full-graph traversal in various circumstances. If that happens, we must preserve existing knowledge for next time. This doesn't have a severe impact on performance because the original goal, of not doing repeated scans when a file had fallen out of the graph, is still met.
PiperOrigin-RevId: 420154787
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 844093a..6b1416b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -471,19 +471,19 @@
int fsvcThreads)
throws InterruptedException {
- // We freshly compute knowledge of the presence of external files in the skyframe graph. We use
- // a fresh ExternalFilesHelper instance and only set the real instance's knowledge *after* we
- // are done with the graph scan, lest an interrupt during the graph scan causes us to
- // incorrectly think there are no longer any external files.
- ExternalFilesHelper tmpExternalFilesHelper =
- externalFilesHelper.cloneWithFreshExternalFilesKnowledge();
-
ExternalFilesKnowledge externalFilesKnowledge = externalFilesHelper.getExternalFilesKnowledge();
if (!pathEntriesWithoutDiffInformation.isEmpty()
|| (externalFilesKnowledge.anyOutputFilesSeen && checkOutputFiles)
|| repositoryHelpersHolder != null
|| externalFilesKnowledge.anyFilesInExternalReposSeen
|| externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+ // We freshly compute knowledge of the presence of external files in the skyframe graph. We
+ // use
+ // a fresh ExternalFilesHelper instance and only set the real instance's knowledge *after* we
+ // are done with the graph scan, lest an interrupt during the graph scan causes us to
+ // incorrectly think there are no longer any external files.
+ ExternalFilesHelper tmpExternalFilesHelper =
+ externalFilesHelper.cloneWithFreshExternalFilesKnowledge();
// Before running the FilesystemValueChecker, ensure that all values marked for invalidation
// have actually been invalidated (recall that invalidation happens at the beginning of the
@@ -514,7 +514,8 @@
if (checkOutputFiles) {
fileTypesToCheck.add(FileType.OUTPUT);
}
- if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+ if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen
+ || !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
fileTypesToCheck.add(FileType.EXTERNAL);
}
logger.atInfo().log(
@@ -540,9 +541,12 @@
batchDirtyResult,
/*numSourceFilesCheckedIfDiffWasMissing=*/ batchDirtyResult.getNumKeysChecked(),
managedDirectoriesChanged);
- }
- if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()
- && !externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+ // We use the knowledge gained during the graph scan that just completed. Otherwise, naively,
+ // once an external file gets into the Skyframe graph, we'll overly-conservatively always
+ // think the graph needs to be scanned.
+ externalFilesHelper.setExternalFilesKnowledge(
+ tmpExternalFilesHelper.getExternalFilesKnowledge());
+ } else if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
logger.atInfo().log(
"About to scan %d external files",
externalFilesKnowledge.nonOutputExternalFilesSeen.size());
@@ -566,8 +570,7 @@
batchDirtyResult =
fsvc.getDirtyKeys(
externalDirtyNodes,
- new ExternalDirtinessChecker(
- tmpExternalFilesHelper, EnumSet.of(FileType.EXTERNAL)));
+ new ExternalDirtinessChecker(externalFilesHelper, EnumSet.of(FileType.EXTERNAL)));
}
handleChangedFiles(
ImmutableList.of(),
@@ -579,11 +582,6 @@
pathEntriesWithoutDiffInformation) {
pair.getSecond().markProcessed();
}
- // We use the knowledge gained during the graph scan that just completed. Otherwise, naively,
- // once an external file gets into the Skyframe graph, we'll overly-conservatively always think
- // the graph needs to be scanned.
- externalFilesHelper.setExternalFilesKnowledge(
- tmpExternalFilesHelper.getExternalFilesKnowledge());
}
private void handleChangedFiles(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java
index 2129bc2..cebba6f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.vfs.DelegateFileSystem;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.NotifyingHelper;
@@ -127,6 +128,34 @@
}
@Test
+ public void ignoreOutputFilesThenCheckAgainDoesCheck() throws Exception {
+ Path buildFile =
+ write(
+ "foo/BUILD",
+ "genrule(name = 'foo', outs = ['out'], cmd = 'cp $< $@', srcs = ['link'])");
+ Path outputFile = directories.getOutputBase().getChild("linkTarget");
+ FileSystemUtils.writeContentAsLatin1(outputFile, "one");
+ buildFile.getParentDirectory().getChild("link").createSymbolicLink(outputFile.asFragment());
+
+ buildTarget("//foo:foo");
+
+ assertContents("one", "//foo:foo");
+
+ addOptions("--noexperimental_check_output_files");
+ FileSystemUtils.writeContentAsLatin1(outputFile, "two");
+
+ buildTarget("//foo:foo");
+
+ assertContents("one", "//foo:foo");
+
+ addOptions("--experimental_check_output_files");
+
+ buildTarget("//foo:foo");
+
+ assertContents("two", "//foo:foo");
+ }
+
+ @Test
public void externalSymlink_doesNotTriggerFullGraphTraversal() throws Exception {
addOptions("--symlink_prefix=/");
AtomicInteger calledGetValues = new AtomicInteger(0);
