Remove legacy filesystem diff approach
PiperOrigin-RevId: 420049959
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 9e36b16..c861189 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
@@ -112,19 +112,6 @@
* Returns a {@link Differencer.DiffWithDelta} containing keys that are dirty according to the
* passed-in {@code dirtinessChecker}.
*/
- public ImmutableBatchDirtyResult getNewAndOldValues(
- Map<SkyKey, SkyValue> valuesMap,
- Collection<SkyKey> keys,
- SkyValueDirtinessChecker dirtinessChecker)
- throws InterruptedException {
- return getDirtyValues(new MapBackedValueFetcher(valuesMap), keys,
- dirtinessChecker, /*checkMissingValues=*/true);
- }
-
- /**
- * Returns a {@link Differencer.DiffWithDelta} containing keys that are dirty according to the
- * passed-in {@code dirtinessChecker}.
- */
public Differencer.DiffWithDelta getNewAndOldValues(
WalkableGraph walkableGraph,
Collection<SkyKey> keys,
@@ -649,11 +636,6 @@
}
@Override
- public Map<SkyKey, Delta> changedKeysWithNewAndOldValues() {
- return dirtyKeysWithNewAndOldValues;
- }
-
- @Override
public Map<SkyKey, SkyValue> changedKeysWithNewValues() {
return Delta.newValues(dirtyKeysWithNewAndOldValues);
}
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 8ddec32..05e32ed 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
@@ -63,7 +63,6 @@
import com.google.devtools.build.lib.actions.DiscoveredModulesPruner;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.Executor;
-import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
@@ -165,7 +164,6 @@
import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ConflictException;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.TopLevelAspectsKey;
-import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.FileDirtinessChecker;
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.MetadataConsumerForMetrics.FilesMetricConsumer;
import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
@@ -178,7 +176,6 @@
import com.google.devtools.build.lib.util.ResourceUsage;
import com.google.devtools.build.lib.util.TestType;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
-import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
import com.google.devtools.build.lib.vfs.OutputService;
@@ -190,7 +187,6 @@
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.CyclesReporter;
import com.google.devtools.build.skyframe.Differencer;
-import com.google.devtools.build.skyframe.Differencer.DiffWithDelta.Delta;
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationContext.UnnecessaryTemporaryStateDropper;
@@ -1293,13 +1289,6 @@
protected abstract void invalidate(Predicate<SkyKey> pred);
- private static boolean compatibleFileTypes(Dirent.Type oldType, FileStateType newType) {
- return (oldType.equals(Dirent.Type.FILE) && newType.equals(FileStateType.REGULAR_FILE))
- || (oldType.equals(Dirent.Type.UNKNOWN) && newType.equals(FileStateType.SPECIAL_FILE))
- || (oldType.equals(Dirent.Type.DIRECTORY) && newType.equals(FileStateType.DIRECTORY))
- || (oldType.equals(Dirent.Type.SYMLINK) && newType.equals(FileStateType.SYMLINK));
- }
-
protected Differencer.Diff getDiff(
TimestampGranularityMonitor tsgm,
ModifiedFileSet modifiedFileSet,
@@ -1322,69 +1311,8 @@
Map<SkyKey, SkyValue> valuesMap = memoizingEvaluator.getValues();
- if (!modifiedFileSet.includesAncestorDirectories()) {
- return FileSystemValueCheckerInferringAncestors.getDiffWithInferredAncestors(
- tsgm, valuesMap, memoizingEvaluator.getDoneValues(), dirtyFileStateSkyKeys, fsvcThreads);
- }
-
- // We only need to invalidate directory values when a file has been created or deleted or
- // changes type, not when it has merely been modified. Unfortunately we do not have that
- // information here, so we compute it ourselves.
- // TODO(bazel-team): Fancy filesystems could provide it with a hypothetically modified
- // DiffAwareness interface.
- logger.atInfo().log(
- "About to recompute filesystem nodes corresponding to files that are known to have "
- + "changed");
- FilesystemValueChecker fsvc =
- new FilesystemValueChecker(tsgm, /* lastExecutionTimeRange= */ null, fsvcThreads);
- Differencer.DiffWithDelta diff =
- fsvc.getNewAndOldValues(valuesMap, dirtyFileStateSkyKeys, new FileDirtinessChecker());
-
- Set<SkyKey> valuesToInvalidate = new HashSet<>();
- Map<SkyKey, SkyValue> valuesToInject = new HashMap<>();
- for (Map.Entry<SkyKey, Delta> entry : diff.changedKeysWithNewAndOldValues().entrySet()) {
- SkyKey key = entry.getKey();
- Preconditions.checkState(key.functionName().equals(FileStateValue.FILE_STATE), key);
- RootedPath rootedPath = (RootedPath) key.argument();
- Delta delta = entry.getValue();
- @Nullable FileStateValue oldValue = (FileStateValue) delta.oldValue();
- FileStateValue newValue = (FileStateValue) delta.newValue();
- valuesToInject.put(key, newValue);
- SkyKey dirListingStateKey = parentDirectoryListingStateKey(rootedPath);
- // Invalidate the directory listing for the path's parent directory if the change was
- // relevant (e.g. path turned from a symlink into a directory) OR if we don't have enough
- // information to determine it was irrelevant.
- boolean changedType;
- if (oldValue != null) {
- changedType = !oldValue.getType().equals(newValue.getType());
- } else {
- DirectoryListingStateValue oldDirListingStateValue =
- (DirectoryListingStateValue) valuesMap.get(dirListingStateKey);
- if (oldDirListingStateValue != null) {
- String baseName = rootedPath.getRootRelativePath().getBaseName();
- Dirent oldDirent = oldDirListingStateValue.getDirents().maybeGetDirent(baseName);
- changedType =
- (oldDirent == null) || !compatibleFileTypes(oldDirent.getType(), newValue.getType());
- } else {
- changedType = true;
- }
- }
- if (changedType) {
- valuesToInvalidate.add(dirListingStateKey);
- }
- }
- for (SkyKey key : diff.changedKeysWithoutNewValues()) {
- Preconditions.checkState(key.functionName().equals(FileStateValue.FILE_STATE), key);
- RootedPath rootedPath = (RootedPath) key.argument();
- valuesToInvalidate.add(key);
- valuesToInvalidate.add(parentDirectoryListingStateKey(rootedPath));
- }
- return new ImmutableDiff(valuesToInvalidate, valuesToInject);
- }
-
- private static SkyKey parentDirectoryListingStateKey(RootedPath rootedPath) {
- RootedPath parentDirRootedPath = rootedPath.getParentDirectory();
- return DirectoryListingStateValue.key(parentDirRootedPath);
+ return FileSystemValueCheckerInferringAncestors.getDiffWithInferredAncestors(
+ tsgm, valuesMap, memoizingEvaluator.getDoneValues(), dirtyFileStateSkyKeys, fsvcThreads);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/ModifiedFileSet.java b/src/main/java/com/google/devtools/build/lib/vfs/ModifiedFileSet.java
index 5bd0181..7a8db52 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/ModifiedFileSet.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/ModifiedFileSet.java
@@ -26,26 +26,23 @@
public class ModifiedFileSet {
// When everything is modified that naturally includes all directories.
- public static final ModifiedFileSet EVERYTHING_MODIFIED =
- new ModifiedFileSet(null, /*includesAncestorDirectories=*/ true);
+ public static final ModifiedFileSet EVERYTHING_MODIFIED = new ModifiedFileSet(null);
/**
* Special case of {@link #EVERYTHING_MODIFIED}, which indicates that the entire tree has been
* deleted.
*/
public static final ModifiedFileSet EVERYTHING_DELETED =
- new ModifiedFileSet(null, /*includesAncestorDirectories=*/ true) {
+ new ModifiedFileSet(null) {
@Override
public boolean treatEverythingAsDeleted() {
return true;
}
};
- public static final ModifiedFileSet NOTHING_MODIFIED =
- new ModifiedFileSet(ImmutableSet.of(), /*includesAncestorDirectories=*/ true);
+ public static final ModifiedFileSet NOTHING_MODIFIED = new ModifiedFileSet(ImmutableSet.of());
@Nullable private final ImmutableSet<PathFragment> modified;
- private final boolean includesAncestorDirectories;
/**
* Whether all files of interest should be treated as potentially modified.
@@ -76,14 +73,6 @@
return modified;
}
- /**
- * Returns whether the diff includes all of affected directories or we need to infer those from
- * reported items.
- */
- public boolean includesAncestorDirectories() {
- return includesAncestorDirectories;
- }
-
@Override
public boolean equals(Object o) {
if (o == this) {
@@ -95,7 +84,6 @@
ModifiedFileSet other = (ModifiedFileSet) o;
return treatEverythingAsModified() == other.treatEverythingAsModified()
&& treatEverythingAsDeleted() == other.treatEverythingAsDeleted()
- && includesAncestorDirectories == other.includesAncestorDirectories
&& Objects.equals(modified, other.modified);
}
@@ -117,29 +105,17 @@
}
}
- private ModifiedFileSet(
- ImmutableSet<PathFragment> modified, boolean includesAncestorDirectories) {
+ private ModifiedFileSet(ImmutableSet<PathFragment> modified) {
this.modified = modified;
- this.includesAncestorDirectories = includesAncestorDirectories;
}
/** The builder for {@link ModifiedFileSet}. */
public static class Builder {
private final ImmutableSet.Builder<PathFragment> setBuilder = ImmutableSet.builder();
- private boolean includesAncestorDirectories = true;
public ModifiedFileSet build() {
ImmutableSet<PathFragment> modified = setBuilder.build();
- return modified.isEmpty()
- // Special case -- if no files were affected, we know the diff is complete even if
- // ancestor directories may not be accounted for.
- ? NOTHING_MODIFIED
- : new ModifiedFileSet(modified, includesAncestorDirectories);
- }
-
- public Builder setIncludesAncestorDirectories(boolean includesAncestorDirectories) {
- this.includesAncestorDirectories = includesAncestorDirectories;
- return this;
+ return modified.isEmpty() ? NOTHING_MODIFIED : new ModifiedFileSet(modified);
}
public Builder modify(PathFragment pathFragment) {
diff --git a/src/main/java/com/google/devtools/build/skyframe/Differencer.java b/src/main/java/com/google/devtools/build/skyframe/Differencer.java
index 1cd7752..d98c56f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/Differencer.java
+++ b/src/main/java/com/google/devtools/build/skyframe/Differencer.java
@@ -50,9 +50,6 @@
/** A {@link Diff} that also potentially contains the new and old values for each changed key. */
interface DiffWithDelta extends Diff {
- /** Returns the value keys whose values have changed, along with their old and new values. */
- Map<SkyKey, Delta> changedKeysWithNewAndOldValues();
-
/** Represents the delta between two values of the same key. */
@AutoValue
abstract class Delta {
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 1f042a7..361ad88 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,7 +29,6 @@
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;
@@ -125,13 +124,8 @@
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.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;
@@ -291,109 +285,6 @@
}
@Test
- public void getDiff_changedFileStillExists_returnsFile() throws Exception {
- Root root = Root.fromPath(scratch.dir("/"));
- Path file = scratch.file("/foo/foo.txt");
- RootedPath fileRootedPath = RootedPath.toRootedPath(root, file);
- FileStateValue.Key key = FileStateValue.key(fileRootedPath);
- FileStateValue oldValue = FileStateValue.create(fileRootedPath, /*tsgm=*/ null);
- skyframeExecutor.memoizingEvaluator.injectGraphTransformerForTesting(
- inMemoryGraphWithValues(ImmutableMap.of(key, oldValue)));
- scratch.overwriteFile(file.getPathString(), "new contents");
-
- Diff diff =
- skyframeExecutor.getDiff(
- /*tsgm=*/ null,
- ModifiedFileSet.builder().modify(PathFragment.create("foo/foo.txt")).build(),
- root,
- /*fsvcThreads=*/ 1);
-
- assertThat(diff.changedKeysWithNewValues())
- .containsExactly(key, FileStateValue.create(fileRootedPath, /*tsgm=*/ null));
- assertThat(diff.changedKeysWithoutNewValues()).isEmpty();
- }
-
- @Test
- public void getDiff_newFile_returnsFileAndParentDirectoryListing() throws Exception {
- Root root = Root.fromPath(scratch.dir("/"));
- Path file = scratch.file("/foo/foo.txt");
-
- Diff diff =
- skyframeExecutor.getDiff(
- /*tsgm=*/ null,
- ModifiedFileSet.builder().modify(PathFragment.create("foo/foo.txt")).build(),
- root,
- /*fsvcThreads=*/ 1);
-
- RootedPath fileRootedPath = RootedPath.toRootedPath(root, file);
- assertThat(diff.changedKeysWithNewValues())
- .containsExactly(
- FileStateValue.key(fileRootedPath),
- FileStateValue.create(fileRootedPath, /*tsgm=*/ null));
- assertThat(diff.changedKeysWithoutNewValues())
- .containsExactly(DirectoryListingStateValue.key(fileRootedPath.getParentDirectory()));
- }
-
- @Test
- public void getDiff_newFileFailsToStat_returnsFileAndParentDirectoryListing() throws Exception {
- Root root = Root.fromPath(scratch.dir("/"));
- Path file = scratch.file("/foo/foo.txt");
- // This makes InMemoryFileSystem throw IOException when we try to stat /foo/foo.txt.
- file.getParentDirectory().setExecutable(false);
-
- Diff diff =
- skyframeExecutor.getDiff(
- /*tsgm=*/ null,
- ModifiedFileSet.builder().modify(PathFragment.create("foo/foo.txt")).build(),
- root,
- /*fsvcThreads=*/ 1);
-
- assertThat(diff.changedKeysWithNewValues()).isEmpty();
- assertThat(diff.changedKeysWithoutNewValues())
- .containsExactly(
- FileStateValue.key(RootedPath.toRootedPath(root, file)),
- DirectoryListingStateValue.key(
- RootedPath.toRootedPath(root, file.getParentDirectory())));
- }
-
- private static GraphTransformerForTesting inMemoryGraphWithValues(
- ImmutableMap<SkyKey, SkyValue> values) {
-
- return new GraphTransformerForTesting() {
- @Override
- public InMemoryGraph transform(InMemoryGraph graph) {
- InMemoryGraph transformed = InMemoryGraph.create();
- transformed
- .getAllValuesMutable()
- .putAll(Maps.transformValues(values, this::createNodeEntry));
- return transformed;
- }
-
- @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, 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'])");
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/ModifiedFileSetTest.java b/src/test/java/com/google/devtools/build/lib/vfs/ModifiedFileSetTest.java
index 075fd45..b91867d 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/ModifiedFileSetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/ModifiedFileSetTest.java
@@ -40,23 +40,9 @@
ModifiedFileSet nonEmpty3 = ModifiedFileSet.builder().modify(fragA).modify(fragB).build();
ModifiedFileSet nonEmpty4 = ModifiedFileSet.builder().modify(fragB).modify(fragA).build();
- ModifiedFileSet nonEmptySkipsAncestorDirectories1 =
- ModifiedFileSet.builder()
- .modify(fragA)
- .modify(fragB)
- .setIncludesAncestorDirectories(false)
- .build();
- ModifiedFileSet nonEmptySkipsAncestorDirectories2 =
- ModifiedFileSet.builder()
- .modify(fragB)
- .modify(fragA)
- .setIncludesAncestorDirectories(false)
- .build();
-
new EqualsTester()
.addEqualityGroup(empty1, empty2, empty3)
.addEqualityGroup(nonEmpty1, nonEmpty2, nonEmpty3, nonEmpty4)
- .addEqualityGroup(nonEmptySkipsAncestorDirectories1, nonEmptySkipsAncestorDirectories2)
.addEqualityGroup(ModifiedFileSet.EVERYTHING_MODIFIED)
.addEqualityGroup(ModifiedFileSet.EVERYTHING_DELETED)
.testEquals();
