From 18e3dc07876536b98d93aca1997478700b9c8d9e Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 14 Oct 2024 14:08:53 -0700 Subject: [PATCH] Make FileSystemDependencies implementations classes instead of record. Record's implementation of hashCode and equals is inappropriate here due to the recursive nature of these types. Instead, use reference equality. These types are effectively interned via FileDependencyDeserializer anyway. PiperOrigin-RevId: 685830037 Change-Id: I00a28e80fa61805edd841f15391ea123c70af71e --- .../DirectoryListingDependencies.java | 19 ++- .../analysis/FileDependencies.java | 111 +++++++++++++----- 2 files changed, 99 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java index a6df57f05cf027..402694b39aaa92 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java @@ -13,14 +13,31 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.serialization.analysis; +import static com.google.common.base.MoreObjects.toStringHelper; + import com.google.common.collect.ImmutableSet; /** Type representing a directory listing operation. */ -record DirectoryListingDependencies(FileDependencies realDirectory) +final class DirectoryListingDependencies implements FileDependencyDeserializer.GetDirectoryListingDependenciesResult, FileSystemDependencies { + private final FileDependencies realDirectory; + + DirectoryListingDependencies(FileDependencies realDirectory) { + this.realDirectory = realDirectory; + } + /** True if this entry matches any directory name in {@code directoryPaths}. */ boolean matchesAnyDirectory(ImmutableSet directoryPaths) { return directoryPaths.contains(realDirectory.resolvedPath()); } + + FileDependencies realDirectory() { + return realDirectory; + } + + @Override + public String toString() { + return toStringHelper(this).add("realDirectory", realDirectory).toString(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java index 01ad942b810bc7..fa2cc52d3f120e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java @@ -13,9 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.serialization.analysis; +import static com.google.common.base.MoreObjects.toStringHelper; + import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; @@ -28,17 +31,17 @@ * #getDependencyCount} and {@link #getDependency}. If any matches are encountered, the associated * value is invalidated. */ -sealed interface FileDependencies - extends FileSystemDependencies, FileDependencyDeserializer.GetDependenciesResult +abstract sealed class FileDependencies + implements FileSystemDependencies, FileDependencyDeserializer.GetDependenciesResult permits FileDependencies.SingleResolvedPath, FileDependencies.SingleResolvedPathAndDependency, FileDependencies.MultiplePaths { - boolean containsMatch(ImmutableSet paths); + abstract boolean containsMatch(ImmutableSet paths); - int getDependencyCount(); + abstract int getDependencyCount(); - FileDependencies getDependency(int index); + abstract FileDependencies getDependency(int index); /** * The real path associated with this node after resolution. @@ -46,14 +49,11 @@ sealed interface FileDependencies *

This is used by {@link FileDependencyDeserializer} to retrieve resolved parent paths but * isn't directly used by invalidation. */ - String resolvedPath(); + abstract String resolvedPath(); /** Returns the resolved paths associated with the current node for testing. */ - // An interface must be used instead of an abstract class to allow records, which are helpful - // here. That means that this method must be public, triggering the warning. - @SuppressWarnings("VisibleForTestingMisused") @VisibleForTesting - ImmutableList getAllResolvedPathsForTesting(); + abstract ImmutableList getAllResolvedPathsForTesting(); static Builder builder(String firstResolvedPath) { return new Builder(firstResolvedPath); @@ -101,42 +101,65 @@ FileDependencies build() { // The implementations here exist to reduce indirection and memory use. - record SingleResolvedPath(String resolvedPath) implements FileDependencies { + static final class SingleResolvedPath extends FileDependencies { + private final String resolvedPath; + + private SingleResolvedPath(String resolvedPath) { + this.resolvedPath = resolvedPath; + } + @Override - public boolean containsMatch(ImmutableSet paths) { + boolean containsMatch(ImmutableSet paths) { return paths.contains(resolvedPath); } @Override - public int getDependencyCount() { + int getDependencyCount() { return 0; } @Override - public FileDependencies getDependency(int index) { + FileDependencies getDependency(int index) { throw new IndexOutOfBoundsException(this + " " + index); } @Override - public ImmutableList getAllResolvedPathsForTesting() { + String resolvedPath() { + return resolvedPath; + } + + @Override + ImmutableList getAllResolvedPathsForTesting() { return ImmutableList.of(resolvedPath); } + + @Override + public String toString() { + return toStringHelper(this).add("resolvedPath", resolvedPath).toString(); + } } - record SingleResolvedPathAndDependency(String resolvedPath, FileDependencies dependency) - implements FileDependencies { + static final class SingleResolvedPathAndDependency extends FileDependencies { + private final String resolvedPath; + private final FileDependencies dependency; + + private SingleResolvedPathAndDependency(String resolvedPath, FileDependencies dependency) { + this.resolvedPath = resolvedPath; + this.dependency = dependency; + } + @Override - public boolean containsMatch(ImmutableSet paths) { + boolean containsMatch(ImmutableSet paths) { return paths.contains(resolvedPath); } @Override - public int getDependencyCount() { + int getDependencyCount() { return 1; } @Override - public FileDependencies getDependency(int index) { + FileDependencies getDependency(int index) { if (index != 0) { throw new IndexOutOfBoundsException(this + " " + index); } @@ -144,16 +167,36 @@ public FileDependencies getDependency(int index) { } @Override - public ImmutableList getAllResolvedPathsForTesting() { + String resolvedPath() { + return resolvedPath; + } + + @Override + ImmutableList getAllResolvedPathsForTesting() { return ImmutableList.of(resolvedPath); } + + @Override + public String toString() { + return toStringHelper(this) + .add("resolvedPath", resolvedPath) + .add("dependency", dependency) + .toString(); + } } - record MultiplePaths( - ImmutableList resolvedPaths, ImmutableList dependencies) - implements FileDependencies { + static final class MultiplePaths extends FileDependencies { + private final ImmutableList resolvedPaths; + private final ImmutableList dependencies; + + private MultiplePaths( + ImmutableList resolvedPaths, ImmutableList dependencies) { + this.resolvedPaths = resolvedPaths; + this.dependencies = dependencies; + } + @Override - public boolean containsMatch(ImmutableSet paths) { + boolean containsMatch(ImmutableSet paths) { for (int i = 0; i < resolvedPaths.size(); i++) { if (paths.contains(resolvedPaths.get(i))) { return true; @@ -163,23 +206,31 @@ public boolean containsMatch(ImmutableSet paths) { } @Override - public int getDependencyCount() { + int getDependencyCount() { return dependencies.size(); } @Override - public FileDependencies getDependency(int index) { + FileDependencies getDependency(int index) { return dependencies.get(index); } @Override - public String resolvedPath() { - return resolvedPaths.get(resolvedPaths().size() - 1); + String resolvedPath() { + return Iterables.getLast(resolvedPaths); } @Override - public ImmutableList getAllResolvedPathsForTesting() { + ImmutableList getAllResolvedPathsForTesting() { return resolvedPaths; } + + @Override + public String toString() { + return toStringHelper(this) + .add("resolvedPaths", resolvedPaths) + .add("dependencies", dependencies) + .toString(); + } } }