Do not invalidate unaffected external directory listings. Diff collection for external files has a bug in which we compare the [`FileStateValue` with `DirectoryListingValue`](https://github.com/bazelbuild/bazel/blob/0e73219fbe4f8053ca196753fc7fac152f02a7d6/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java#L603-L610) which leads to unnecessary invalidation of `DirectoryListingValue.Key` and dependent keys. Use proper value for `DirectoryListingValue` as a base of comparison with existing state when collecting the external diff. PiperOrigin-RevId: 431006201
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 7596352..480eb3c 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
@@ -605,7 +605,7 @@ externalDirtyNodes.put(key, value); } key = DirectoryListingStateValue.key(path); - memoizingEvaluator.getExistingValue(key); + value = memoizingEvaluator.getExistingValue(key); if (value != null) { externalDirtyNodes.put(key, value); }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index 0d2f926..f8ae7ab 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD
@@ -104,6 +104,7 @@ "//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", + "//src/main/java/com/google/devtools/build/lib/skyframe:diff_awareness", "//src/main/java/com/google/devtools/build/lib/skyframe:managed_directories_knowledge", "//src/main/java/com/google/devtools/build/lib/skyframe:package_roots_no_symlink_creation", "//src/main/java/com/google/devtools/build/lib/skyframe:package_value",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 28bf9ed..3200fa1 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -155,6 +155,7 @@ import com.google.devtools.build.lib.skyframe.BzlLoadFunction; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; +import com.google.devtools.build.lib.skyframe.DiffAwareness; import com.google.devtools.build.lib.skyframe.ManagedDirectoriesKnowledge; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageRootsNoSymlinkCreation; @@ -249,6 +250,14 @@ } public void initializeSkyframeExecutor(boolean doPackageLoadingChecks) throws Exception { + initializeSkyframeExecutor( + /*doPackageLoadingChecks=*/ doPackageLoadingChecks, + /*diffAwarenessFactories=*/ ImmutableList.of()); + } + + public void initializeSkyframeExecutor( + boolean doPackageLoadingChecks, ImmutableList<DiffAwareness.Factory> diffAwarenessFactories) + throws Exception { analysisMock = getAnalysisMock(); directories = new BlazeDirectories( @@ -314,7 +323,8 @@ .setActionKeyContext(actionKeyContext) .setWorkspaceStatusActionFactory(workspaceStatusActionFactory) .setExtraSkyFunctions(analysisMock.getSkyFunctions(directories)) - .setPerCommandSyscallCache(SyscallCache.NO_CACHE); + .setPerCommandSyscallCache(SyscallCache.NO_CACHE) + .setDiffAwarenessFactories(diffAwarenessFactories); ManagedDirectoriesKnowledge managedDirectoriesKnowledge = getManagedDirectoriesKnowledge(); if (managedDirectoriesKnowledge != null) { builder.setRepositoryHelpersHolder(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index ef5111d..2f67173 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -29,6 +29,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; import com.google.common.eventbus.EventBus; import com.google.common.hash.HashCode; import com.google.common.testing.GcFinalization; @@ -92,8 +93,10 @@ import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider; import com.google.devtools.build.lib.pkgcache.PackageManager; +import com.google.devtools.build.lib.pkgcache.PackageOptions; import com.google.devtools.build.lib.query2.common.QueryTransitivePackagePreloader; import com.google.devtools.build.lib.runtime.KeepGoingOption; import com.google.devtools.build.lib.server.FailureDetails.Crash; @@ -108,6 +111,7 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.CrashFailureDetails; @@ -124,14 +128,22 @@ import com.google.devtools.build.skyframe.Differencer.Diff; import com.google.devtools.build.skyframe.EvaluationContext; import com.google.devtools.build.skyframe.EvaluationResult; +import com.google.devtools.build.skyframe.InMemoryGraph; +import com.google.devtools.build.skyframe.InMemoryGraphImpl; +import com.google.devtools.build.skyframe.InMemoryNodeEntry; +import com.google.devtools.build.skyframe.MemoizingEvaluator.GraphTransformerForTesting; import com.google.devtools.build.skyframe.NotifyingHelper; import com.google.devtools.build.skyframe.NotifyingHelper.EventType; +import com.google.devtools.build.skyframe.ProcessableGraph; +import com.google.devtools.build.skyframe.QueryableGraph; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.TaggedEvents; import com.google.devtools.build.skyframe.TrackingAwaiter; import com.google.devtools.build.skyframe.ValueWithMetadata; +import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsParser; +import com.google.devtools.common.options.OptionsProvider; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.IOException; @@ -143,6 +155,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -285,6 +298,198 @@ } @Test + public void sync_onlyExternalFileChanged_reportsAffectedFile() throws Exception { + Root externalRoot = Root.fromPath(scratch.dir("/external")); + RootedPath file = RootedPath.toRootedPath(externalRoot, scratch.file("/external/file")); + initializeSkyframeExecutor( + /*doPackageLoadingChecks=*/ true, ImmutableList.of(nothingChangedDiffAwarenessFactory())); + skyframeExecutor.memoizingEvaluator.injectGraphTransformerForTesting( + inMemoryGraphWithValues( + ImmutableMap.of(file, FileStateValue.create(file, /*tsgm=*/ null)))); + skyframeExecutor.externalFilesHelper.getAndNoteFileType(file); + // Initial sync to establish the baseline DiffAwareness.View. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + scratch.overwriteFile("/external/file", "new content"); + + skyframeExecutor.sync( + NullEventHandler.INSTANCE, + Options.getDefaults(PackageOptions.class), + skyframeExecutor.getPackageLocator().get(), + Options.getDefaults(BuildLanguageOptions.class), + UUID.randomUUID(), + /*clientEnv=*/ ImmutableMap.of(), + /*repoEnvOption=*/ ImmutableMap.of(), + new TimestampGranularityMonitor(new ManualClock()), + options); + + Diff diff = getRecordedDiff(); + assertThat(diff.changedKeysWithNewValues()).containsKey(file); + } + + @Test + public void sync_nothingChangedWithExternalFile_reportsNoExternalKeysInDiff() throws Exception { + Root externalRoot = Root.fromPath(scratch.dir("/external")); + RootedPath file = RootedPath.toRootedPath(externalRoot, scratch.file("/external/file")); + initializeSkyframeExecutor( + /*doPackageLoadingChecks=*/ true, ImmutableList.of(nothingChangedDiffAwarenessFactory())); + skyframeExecutor.memoizingEvaluator.injectGraphTransformerForTesting( + inMemoryGraphWithValues( + ImmutableMap.of(file, FileStateValue.create(file, /*tsgm=*/ null)))); + skyframeExecutor.externalFilesHelper.getAndNoteFileType(file); + // Initial sync to establish the baseline DiffAwareness.View. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + + skyframeExecutor.sync( + NullEventHandler.INSTANCE, + Options.getDefaults(PackageOptions.class), + skyframeExecutor.getPackageLocator().get(), + Options.getDefaults(BuildLanguageOptions.class), + UUID.randomUUID(), + /*clientEnv=*/ ImmutableMap.of(), + /*repoEnvOption=*/ ImmutableMap.of(), + new TimestampGranularityMonitor(new ManualClock()), + options); + + Diff diff = getRecordedDiff(); + assertThat(diff.changedKeysWithoutNewValues()).doesNotContain(file); + assertThat(diff.changedKeysWithNewValues()).doesNotContainKey(file); + } + + @Test + public void sync_onlyExternalListingChanged_reportsAffectedListing() throws Exception { + Root externalRoot = Root.fromPath(scratch.dir("/external")); + RootedPath dir = RootedPath.toRootedPath(externalRoot, scratch.dir("/external/foo")); + DirectoryListingStateValue value = DirectoryListingStateValue.create(dir); + DirectoryListingStateValue.Key dirListingKey = DirectoryListingStateValue.key(dir); + initializeSkyframeExecutor( + /*doPackageLoadingChecks=*/ true, ImmutableList.of(nothingChangedDiffAwarenessFactory())); + skyframeExecutor.memoizingEvaluator.injectGraphTransformerForTesting( + inMemoryGraphWithValues( + ImmutableMap.of( + dir, FileStateValue.create(dir, /*tsgm=*/ null), dirListingKey, value))); + skyframeExecutor.externalFilesHelper.getAndNoteFileType(dir); + // Initial sync to establish the baseline DiffAwareness.View. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + scratch.file("/external/foo/new_file"); + + skyframeExecutor.sync( + NullEventHandler.INSTANCE, + Options.getDefaults(PackageOptions.class), + skyframeExecutor.getPackageLocator().get(), + Options.getDefaults(BuildLanguageOptions.class), + UUID.randomUUID(), + /*clientEnv=*/ ImmutableMap.of(), + /*repoEnvOption=*/ ImmutableMap.of(), + new TimestampGranularityMonitor(new ManualClock()), + options); + + Diff diff = getRecordedDiff(); + assertThat(diff.changedKeysWithoutNewValues()).containsNoneOf(dir, dirListingKey); + assertThat(diff.changedKeysWithNewValues()).doesNotContainKey(dir); + assertThat(diff.changedKeysWithNewValues()).containsKey(dirListingKey); + } + + @Test + public void sync_nothingChangedWithExternalListing_reportsNoExternalKeysInDiff() + throws Exception { + Root externalRoot = Root.fromPath(scratch.dir("/external")); + RootedPath dir = RootedPath.toRootedPath(externalRoot, scratch.dir("/external/foo")); + DirectoryListingStateValue value = DirectoryListingStateValue.create(dir); + DirectoryListingStateValue.Key dirListingKey = DirectoryListingStateValue.key(dir); + initializeSkyframeExecutor( + /*doPackageLoadingChecks=*/ true, ImmutableList.of(nothingChangedDiffAwarenessFactory())); + skyframeExecutor.memoizingEvaluator.injectGraphTransformerForTesting( + inMemoryGraphWithValues( + ImmutableMap.of( + dir, FileStateValue.create(dir, /*tsgm=*/ null), dirListingKey, value))); + skyframeExecutor.externalFilesHelper.getAndNoteFileType(dir); + // Initial sync to establish the baseline DiffAwareness.View. + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + + skyframeExecutor.sync( + NullEventHandler.INSTANCE, + Options.getDefaults(PackageOptions.class), + skyframeExecutor.getPackageLocator().get(), + Options.getDefaults(BuildLanguageOptions.class), + UUID.randomUUID(), + /*clientEnv=*/ ImmutableMap.of(), + /*repoEnvOption=*/ ImmutableMap.of(), + new TimestampGranularityMonitor(new ManualClock()), + options); + + Diff diff = getRecordedDiff(); + assertThat(diff.changedKeysWithoutNewValues()).containsNoneOf(dir, dirListingKey); + assertThat(diff.changedKeysWithNewValues()).doesNotContainKey(dir); + assertThat(diff.changedKeysWithNewValues()).doesNotContainKey(dirListingKey); + } + + private static DiffAwareness.Factory nothingChangedDiffAwarenessFactory() { + return pathEntry -> + new DiffAwareness() { + @Override + public View getCurrentView(OptionsProvider options) { + return new View() {}; + } + + @Override + public ModifiedFileSet getDiff(View oldView, View newView) { + return ModifiedFileSet.NOTHING_MODIFIED; + } + + @Override + public String name() { + return null; + } + + @Override + public void close() {} + }; + } + + private Diff getRecordedDiff() { + return skyframeExecutor + .getDifferencerForTesting() + .getDiff(/*fromGraph=*/ null, ignored -> false, ignored -> false); + } + + private static GraphTransformerForTesting inMemoryGraphWithValues( + ImmutableMap<SkyKey, SkyValue> values) { + + return new GraphTransformerForTesting() { + @Override + public InMemoryGraph transform(InMemoryGraph graph) { + return new InMemoryGraphImpl(values.size()) { + { + nodeMap.putAll(Maps.transformValues(values, v -> createNodeEntry(v))); + } + }; + } + + @Override + public QueryableGraph transform(QueryableGraph graph) { + throw new UnsupportedOperationException(); + } + + @Override + public ProcessableGraph transform(ProcessableGraph graph) { + throw new UnsupportedOperationException(); + } + + private InMemoryNodeEntry createNodeEntry(SkyValue value) { + InMemoryNodeEntry nodeEntry = new InMemoryNodeEntry(); + nodeEntry.addReverseDepAndCheckIfDone(null); + nodeEntry.markRebuilding(); + try { + nodeEntry.setValue(value, ignored -> false, /*maxTransitiveSourceVersion=*/ null); + } catch (InterruptedException e) { + throw new RuntimeException(); + } + return nodeEntry; + } + }; + } + + @Test public void testSetDeletedPackages() throws Exception { ExtendedEventHandler eventHandler = NullEventHandler.INSTANCE; scratch.file("foo/bar/BUILD", "cc_library(name = 'bar', hdrs = ['bar.h'])");