Intern `RootedPath`.
The recently added `SkyKeyInterner` feature makes this much more worthwhile.
To offset the extra cost of creating a `RootedPath` instance, try to avoid it if possible.
Delete unused `RootedPathAndCasing` which relied on `RootedPath` not being interned on case-insensitive filesystems.
PiperOrigin-RevId: 516855266
Change-Id: I1d5365abe59b11e77305d247f24ff9a6677f30e9
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
index 76414eb..0ba6700 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
@@ -78,10 +78,10 @@
// In the course of resolving the real path of p, there will be a logical chain of paths we
// consider. Going with the example from above, the full chain of paths we consider is
// [a/b, c/b].
- ArrayList<RootedPath> logicalChain = new ArrayList<>();
- // Same contents as 'logicalChain', except stored as an sorted TreeSet for efficiency reasons.
+ final ArrayList<RootedPath> logicalChain = new ArrayList<>();
+ // Same contents as 'logicalChain', except stored as a sorted TreeSet for efficiency reasons.
// See the usage in checkPathSeenDuringPartialResolutionInternal.
- TreeSet<Path> sortedLogicalChain = Sets.newTreeSet();
+ final TreeSet<Path> sortedLogicalChain = Sets.newTreeSet();
ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain = null;
ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain = null;
@@ -145,9 +145,13 @@
realFileStateValue);
}
- private static RootedPath getChild(RootedPath parentRootedPath, String baseName) {
+ private static RootedPath getChild(
+ RootedPath parent, String baseName, RootedPath originalParent, RootedPath originalChild) {
+ if (parent.equals(originalParent)) {
+ return originalChild; // Avoid constructing a new instance if we already have the child.
+ }
return RootedPath.toRootedPath(
- parentRootedPath.getRoot(), parentRootedPath.getRootRelativePath().getChild(baseName));
+ parent.getRoot(), parent.getRootRelativePath().getChild(baseName));
}
private RootedPath toRootedPath(Path path) {
@@ -180,14 +184,15 @@
Environment env)
throws InterruptedException, FileFunctionException {
PathFragment relativePath = rootedPath.getRootRelativePath();
- RootedPath rootedPathFromAncestors;
String baseName = relativePath.getBaseName();
FileValue parentFileValue = (FileValue) env.getValue(FileValue.key(parentRootedPath));
if (parentFileValue == null) {
return null;
}
- rootedPathFromAncestors = getChild(parentFileValue.realRootedPath(), baseName);
+
+ RootedPath rootedPathFromAncestors =
+ getChild(parentFileValue.realRootedPath(), baseName, parentRootedPath, rootedPath);
if (!parentFileValue.exists() || !parentFileValue.isDirectory()) {
return new PartialResolutionResult(
@@ -196,7 +201,9 @@
for (RootedPath parentPartialRootedPath : parentFileValue.logicalChainDuringResolution()) {
checkAndNotePathSeenDuringPartialResolution(
- getChild(parentPartialRootedPath, baseName), symlinkResolutionState, env);
+ getChild(parentPartialRootedPath, baseName, parentRootedPath, rootedPath),
+ symlinkResolutionState,
+ env);
if (env.valuesMissing()) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index d3245b2..87ec403 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -57,7 +57,7 @@
/** Ignore the violation. */
IGNORE,
/** Generate an error. */
- ERROR;
+ ERROR
}
private final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages;
@@ -253,7 +253,6 @@
BuildFileName buildFileName)
throws InterruptedException, PackageLookupFunctionException {
PathFragment buildFileFragment = buildFileName.getBuildFileFragment(packageIdentifier);
- RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry, buildFileFragment);
if (crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.ERROR) {
// Is this path part of a local repository?
@@ -308,6 +307,7 @@
}
// Check for the existence of the build file.
+ RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry, buildFileFragment);
FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
if (fileValue == null) {
return null;
@@ -436,16 +436,15 @@
* recursive target pattern (like foo/...).
*/
private static final class PackageLookupFunctionException extends SkyFunctionException {
- public PackageLookupFunctionException(BuildFileNotFoundException e, Transience transience) {
+ PackageLookupFunctionException(BuildFileNotFoundException e, Transience transience) {
super(e, transience);
}
- public PackageLookupFunctionException(RepositoryFetchException e, Transience transience) {
+ PackageLookupFunctionException(RepositoryFetchException e, Transience transience) {
super(e, transience);
}
- public PackageLookupFunctionException(
- InconsistentFilesystemException e, Transience transience) {
+ PackageLookupFunctionException(InconsistentFilesystemException e, Transience transience) {
super(e, transience);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/RootedPath.java b/src/main/java/com/google/devtools/build/lib/vfs/RootedPath.java
index c11c398..50d3095 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/RootedPath.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/RootedPath.java
@@ -13,10 +13,11 @@
// limitations under the License.
package com.google.devtools.build.lib.vfs;
-import com.google.common.base.Preconditions;
+import static com.google.common.base.Preconditions.checkArgument;
+
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.skyframe.SkyKey;
import java.util.Comparator;
-import java.util.Objects;
import javax.annotation.Nullable;
/**
@@ -28,6 +29,9 @@
*/
@AutoCodec
public final class RootedPath implements Comparable<RootedPath>, FileStateKey {
+
+ private static final SkyKeyInterner<RootedPath> interner = SkyKey.newInterner();
+
private final Root root;
private final PathFragment rootRelativePath;
@@ -38,12 +42,16 @@
/** Constructs a {@link RootedPath} from a {@link Root} and path fragment relative to the root. */
@AutoCodec.Instantiator
@AutoCodec.VisibleForSerialization
- RootedPath(Root root, PathFragment rootRelativePath) {
- Preconditions.checkState(
+ static RootedPath createInternal(Root root, PathFragment rootRelativePath) {
+ checkArgument(
rootRelativePath.isAbsolute() == root.isAbsolute(),
"rootRelativePath: %s root: %s",
rootRelativePath,
root);
+ return interner.intern(new RootedPath(root, rootRelativePath));
+ }
+
+ private RootedPath(Root root, PathFragment rootRelativePath) {
this.root = root;
this.rootRelativePath = rootRelativePath;
this.hashCode = 31 * root.hashCode() + rootRelativePath.hashCode();
@@ -51,25 +59,20 @@
/** Returns a rooted path representing {@code rootRelativePath} relative to {@code root}. */
public static RootedPath toRootedPath(Root root, PathFragment rootRelativePath) {
- if (rootRelativePath.isAbsolute()) {
- if (root.isAbsolute()) {
- return new RootedPath(root, rootRelativePath);
- } else {
- Preconditions.checkArgument(
- root.contains(rootRelativePath),
- "rootRelativePath '%s' is absolute, but it's not under root '%s'",
- rootRelativePath,
- root);
- return new RootedPath(root, root.relativize(rootRelativePath));
- }
- } else {
- return new RootedPath(root, rootRelativePath);
+ if (rootRelativePath.isAbsolute() && !root.isAbsolute()) {
+ checkArgument(
+ root.contains(rootRelativePath),
+ "rootRelativePath '%s' is absolute, but it's not under root '%s'",
+ rootRelativePath,
+ root);
+ rootRelativePath = root.relativize(rootRelativePath);
}
+ return createInternal(root, rootRelativePath);
}
/** Returns a rooted path representing {@code path} under the root {@code root}. */
public static RootedPath toRootedPath(Root root, Path path) {
- Preconditions.checkState(root.contains(path), "path: %s root: %s", path, root);
+ checkArgument(root.contains(path), "path: %s root: %s", path, root);
return toRootedPath(root, path.asFragment());
}
@@ -105,7 +108,7 @@
if (rootRelativeParentDirectory == null) {
return null;
}
- return new RootedPath(root, rootRelativeParentDirectory);
+ return createInternal(root, rootRelativeParentDirectory);
}
@Override
@@ -117,8 +120,9 @@
return false;
}
RootedPath other = (RootedPath) obj;
- return Objects.equals(root, other.root)
- && Objects.equals(rootRelativePath, other.rootRelativePath);
+ return hashCode == other.hashCode
+ && root.equals(other.root)
+ && rootRelativePath.equals(other.rootRelativePath);
}
@Override
@@ -143,4 +147,9 @@
public RootedPath argument() {
return this;
}
+
+ @Override
+ public SkyKeyInterner<?> getSkyKeyInterner() {
+ return interner;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/RootedPathAndCasing.java b/src/main/java/com/google/devtools/build/lib/vfs/RootedPathAndCasing.java
deleted file mode 100644
index 0cbeb2a..0000000
--- a/src/main/java/com/google/devtools/build/lib/vfs/RootedPathAndCasing.java
+++ /dev/null
@@ -1,76 +0,0 @@
-// Copyright 2019 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.vfs;
-
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import java.util.Objects;
-
-/**
- * A {@link RootedPath} and its String representation.
- *
- * <p>This object stores not just a path but also the particular casing of it. On case-sensitive
- * filesystems, the paths "foo/Bar.txt" and "FOO/bar.TXT" are different files, but on
- * case-insensitive (or case-ignoring) they refer to the same file.
- *
- * <p>The {@code RootedPathAndCasing} class therefore allows comparing equal {@link RootedPath}s
- * that might use different casing.
- */
-@AutoCodec
-public final class RootedPathAndCasing {
- private final RootedPath path;
- private final String casing;
- private final int hash;
-
- /** Constructs a {@link RootedPath} from a {@link Root} and path fragment relative to the root. */
- @AutoCodec.Instantiator
- @AutoCodec.VisibleForSerialization
- RootedPathAndCasing(RootedPath path, String casing) {
- this.path = Preconditions.checkNotNull(path);
- this.casing = Preconditions.checkNotNull(casing);
- this.hash = Objects.hash(path, casing);
- }
-
- public static RootedPathAndCasing create(RootedPath path) {
- return new RootedPathAndCasing(path, path.getRootRelativePath().getPathString());
- }
-
- public RootedPath getPath() {
- return path;
- }
-
- @Override
- public boolean equals(Object obj) {
- if (this == obj) {
- return true;
- }
- if (!(obj instanceof RootedPathAndCasing)) {
- return false;
- }
- RootedPathAndCasing other = (RootedPathAndCasing) obj;
- return hash == other.hash // fast track for unequal objects
- && Objects.equals(path, other.path)
- && Objects.equals(casing, other.casing);
- }
-
- @Override
- public int hashCode() {
- return hash;
- }
-
- @Override
- public String toString() {
- return "[" + path.getRoot() + "]/[" + casing + "]";
- }
-}
