Optimize memory usage of `RegularFileValue`... by removing it. `RegularFileValue` (i.e. the path's real path is itself, and it's an existing file) is the common case situation for `FileValue`. In this situation we don't actually need to spend shallow heap to store a reference to the original `RootedPath` in order to service `FileValue#realRootedPath` and `FileValue#logicalChainDuringResolution`; instead we refactor all callers to pass in the original `RootedPath`. After making this change we notice that `RegularFileValue` is now just a wrapper around `FileStateValue`, so then we remove the need for the wrapper by turning `RegularFileValue` into an abstract class (an interface would also work), and then having `FileStateValue` extend it. PiperOrigin-RevId: 655774211 Change-Id: Icf1684c85c605a267a85c7917bb53dffaeb85fd7
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java index 245e9ed..dea15d0 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java
@@ -19,6 +19,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; +import com.google.devtools.build.lib.actions.FileValue.RegularFileValue; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.io.InconsistentFilesystemException; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; @@ -34,7 +35,6 @@ import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.XattrProvider; -import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; import java.util.Arrays; import java.util.Objects; @@ -57,9 +57,14 @@ * com.google.devtools.build.lib.skyframe.FileFunction}. Instead, {@link FileValue} should be used * by {@link com.google.devtools.build.skyframe.SkyFunction} consumers that care about files. * + * <p>The common case for {@link FileValue} is {@link RegularFileValue} (i.e. the path's real path + * is itself, and it's an existing file). As a memory optimization for this common case, we have + * {@link FileStateValue} be a {@link RegularFileValue} so that we don't need a wrapper object for + * the value of the corresponding {@link FileValue} node. + * * <p>All subclasses must implement {@link #equals} and {@link #hashCode} properly. */ -public abstract class FileStateValue implements HasDigest, SkyValue { +public abstract class FileStateValue extends RegularFileValue implements HasDigest { @SerializationConstant public static final DirectoryFileStateValue DIRECTORY_FILE_STATE_NODE = new DirectoryFileStateValue(); @@ -205,6 +210,11 @@ return rootedPath; } + @Override + public FileStateValue realFileStateValue() { + return this; + } + public abstract FileStateType getType(); /** Returns the target of the symlink, or throws an exception if this is not a symlink. */ @@ -212,7 +222,8 @@ throw new IllegalStateException(); } - long getSize() { + @Override + public long getSize() { throw new IllegalStateException(); } @@ -404,7 +415,7 @@ } @Override - long getSize() { + public long getSize() { return 0; }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java index a1a5253..698c062 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java
@@ -41,26 +41,26 @@ * File values for missing files will be created on purpose in order to facilitate incremental * builds in the case those files have reappeared. * - * <p>This class contains the relevant metadata for a file, although not the contents. Note that - * since a FileValue doesn't store its corresponding SkyKey, it's possible for the FileValues for - * two different paths to be the same. + * <p>This interface encapsulates the relevant metadata for a file, although not the contents. Note + * that since a FileValue doesn't necessarily store its corresponding SkyKey, it's possible for the + * FileValues for two different paths to be the same. * * <p>This should not be used for build outputs; use {@link ArtifactSkyKey} to create keys for * those. */ @Immutable @ThreadSafe -public abstract class FileValue implements SkyValue { +public interface FileValue extends SkyValue { // Depends non-hermetically on package path, but that is under the control of a flag, so use // semi-hermetic. public static final SkyFunctionName FILE = SkyFunctionName.createSemiHermetic("FILE"); - public boolean exists() { + default boolean exists() { return realFileStateValue().getType() != FileStateType.NONEXISTENT; } /** Returns true if the original path is a symlink; the target path can never be a symlink. */ - public boolean isSymlink() { + default boolean isSymlink() { return false; } @@ -68,7 +68,7 @@ * Returns true if this value corresponds to a file or symlink to an existing regular or special * file. If so, its parent directory is guaranteed to exist. */ - public boolean isFile() { + default boolean isFile() { return realFileStateValue().getType() == FileStateType.REGULAR_FILE || realFileStateValue().getType() == FileStateType.SPECIAL_FILE; } @@ -77,7 +77,7 @@ * Returns true if this value corresponds to a special file or symlink to a special file. If so, * its parent directory is guaranteed to exist. */ - public boolean isSpecialFile() { + default boolean isSpecialFile() { return realFileStateValue().getType() == FileStateType.SPECIAL_FILE; } @@ -85,7 +85,7 @@ * Returns true if the file is a directory or a symlink to an existing directory. If so, its * parent directory is guaranteed to exist. */ - public boolean isDirectory() { + default boolean isDirectory() { return realFileStateValue().getType() == FileStateType.DIRECTORY; } @@ -99,28 +99,28 @@ * -- this information is only needed for resolving ancestors, and an existing file or a * non-existent directory has no descendants, by definition. */ - public abstract ImmutableList<RootedPath> logicalChainDuringResolution(); + ImmutableList<RootedPath> logicalChainDuringResolution(RootedPath initialRootedPath); /** * If a symlink pointing back to its own ancestor was encountered during the resolution of this * {@link FileValue}, returns the path to it. Otherwise, returns null. */ - public abstract ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain(); + ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain(); /** * If a symlink pointing back to its own ancestor was encountered during the resolution of this * {@link FileValue}, returns the symlinks in the cycle. Otherwise, returns null. */ - public abstract ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain(); + ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain(); /** * Returns the real rooted path of the file, taking ancestor symlinks into account. For example, * the rooted path ['root']/['a/b'] is really ['root']/['c/b'] if 'a' is a symlink to 'c'. Note * that ancestor symlinks outside the root boundary are not taken into consideration. */ - public abstract RootedPath realRootedPath(); + RootedPath realRootedPath(RootedPath initialRootedPath); - public abstract FileStateValue realFileStateValue(); + FileStateValue realFileStateValue(); /** * Returns the unresolved link target if {@link #isSymlink()}. @@ -129,31 +129,31 @@ * example could be a build rule that copies a set of input files to the output directory, but * upon encountering symbolic links it can decide between copying or following them. */ - public PathFragment getUnresolvedLinkTarget() { + default PathFragment getUnresolvedLinkTarget() { throw new IllegalStateException(this.toString()); } - public long getSize() { + default long getSize() { Preconditions.checkState(isFile(), this); return realFileStateValue().getSize(); } @Nullable - public byte[] getDigest() { + default byte[] getDigest() { Preconditions.checkState(isFile(), this); return realFileStateValue().getDigest(); } /** Returns a key for building a file value for the given root-relative path. */ @ThreadSafe - public static Key key(RootedPath rootedPath) { + static Key key(RootedPath rootedPath) { return Key.create(rootedPath); } /** Key type for FileValue. */ @VisibleForSerialization @AutoCodec - public static class Key extends AbstractSkyKey<RootedPath> { + class Key extends AbstractSkyKey<RootedPath> { private static final SkyKeyInterner<Key> interner = SkyKey.newInterner(); private Key(RootedPath arg) { @@ -185,7 +185,7 @@ * Only intended to be used by {@link com.google.devtools.build.lib.skyframe.FileFunction}. Should * not be used for symlink cycles. */ - public static FileValue value( + static FileValue value( ImmutableList<RootedPath> logicalChainDuringResolution, ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain, ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain, @@ -209,7 +209,7 @@ "logicalChainDuringResolution: %s, originalRootedPath: %s", logicalChainDuringResolution, originalRootedPath); - return new RegularFileValue(originalRootedPath, fileStateValueFromAncestors); + return fileStateValueFromAncestors; } boolean shouldStoreChain = @@ -253,25 +253,14 @@ } /** - * Implementation of {@link FileValue} for paths whose fully resolved path is the same as the - * requested path. For example, this is the case for the path "foo/bar/baz" if neither 'foo' nor - * 'foo/bar' nor 'foo/bar/baz' are symlinks. + * A {@link FileValue} for paths whose fully resolved path is the same as the requested path. For + * example, this is the case for the path "foo/bar/baz" if neither 'foo' nor 'foo/bar' nor + * 'foo/bar/baz' are symlinks. */ - @VisibleForTesting - public static final class RegularFileValue extends FileValue { - - private final RootedPath rootedPath; - private final FileStateValue fileStateValue; - - @VisibleForTesting - public RegularFileValue(RootedPath rootedPath, FileStateValue fileStateValue) { - this.rootedPath = Preconditions.checkNotNull(rootedPath); - this.fileStateValue = Preconditions.checkNotNull(fileStateValue); - } - + abstract class RegularFileValue implements FileValue { @Override - public ImmutableList<RootedPath> logicalChainDuringResolution() { - return ImmutableList.of(rootedPath); + public ImmutableList<RootedPath> logicalChainDuringResolution(RootedPath initialRootedPath) { + return ImmutableList.of(initialRootedPath); } @Nullable @@ -287,34 +276,8 @@ } @Override - public RootedPath realRootedPath() { - return rootedPath; - } - - @Override - public FileStateValue realFileStateValue() { - return fileStateValue; - } - - @Override - public boolean equals(Object obj) { - if (obj == null) { - return false; - } - if (!(obj instanceof RegularFileValue other)) { - return false; - } - return rootedPath.equals(other.rootedPath) && fileStateValue.equals(other.fileStateValue); - } - - @Override - public int hashCode() { - return Objects.hash(rootedPath, fileStateValue); - } - - @Override - public String toString() { - return String.format("non-symlink (path=%s, state=%s)", rootedPath, fileStateValue); + public RootedPath realRootedPath(RootedPath initialRootedPath) { + return initialRootedPath; } } @@ -323,8 +286,7 @@ * required traversing a symlink chain caused by a symlink pointing to its own ancestor but which * eventually points to a real file. */ - @VisibleForTesting - public static class DifferentRealPathFileValueWithUnboundedAncestorExpansion + class DifferentRealPathFileValueWithUnboundedAncestorExpansion extends DifferentRealPathFileValueWithStoredChain { protected final ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain; protected final ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain; @@ -401,7 +363,7 @@ * path "foo/bar/baz" if at least one of {'foo', 'foo/bar'} is a symlink but 'foo/bar/baz' not. */ @VisibleForTesting - public static class DifferentRealPathFileValueWithStoredChain extends FileValue { + class DifferentRealPathFileValueWithStoredChain implements FileValue { protected final RootedPath realRootedPath; protected final FileStateValue realFileStateValue; protected final ImmutableList<RootedPath> logicalChainDuringResolution; @@ -417,7 +379,7 @@ } @Override - public RootedPath realRootedPath() { + public RootedPath realRootedPath(RootedPath initialRootedPath) { return realRootedPath; } @@ -427,7 +389,7 @@ } @Override - public ImmutableList<RootedPath> logicalChainDuringResolution() { + public ImmutableList<RootedPath> logicalChainDuringResolution(RootedPath initialRootedPath) { return logicalChainDuringResolution; } @@ -475,7 +437,7 @@ * #logicalChainDuringResolution}. */ @VisibleForTesting - public static class DifferentRealPathFileValueWithoutStoredChain extends FileValue { + class DifferentRealPathFileValueWithoutStoredChain implements FileValue { protected final RootedPath realRootedPath; protected final FileStateValue realFileStateValue; @@ -487,7 +449,7 @@ } @Override - public RootedPath realRootedPath() { + public RootedPath realRootedPath(RootedPath initialRootedPath) { return realRootedPath; } @@ -497,7 +459,7 @@ } @Override - public ImmutableList<RootedPath> logicalChainDuringResolution() { + public ImmutableList<RootedPath> logicalChainDuringResolution(RootedPath initialRootedPath) { throw new IllegalStateException(this.toString()); } @@ -543,7 +505,7 @@ * by a symlink pointing to its own ancestor and which eventually points to a symlink. */ @VisibleForTesting - public static final class SymlinkFileValueWithUnboundedAncestorExpansion + final class SymlinkFileValueWithUnboundedAncestorExpansion extends SymlinkFileValueWithStoredChain { private final ImmutableList<RootedPath> pathToUnboundedAncestorSymlinkExpansionChain; private final ImmutableList<RootedPath> unboundedAncestorSymlinkExpansionChain; @@ -620,8 +582,7 @@ /** Implementation of {@link FileValue} for paths that are themselves symlinks. */ @VisibleForTesting - public static class SymlinkFileValueWithStoredChain - extends DifferentRealPathFileValueWithStoredChain { + class SymlinkFileValueWithStoredChain extends DifferentRealPathFileValueWithStoredChain { protected final PathFragment linkTarget; @VisibleForTesting @@ -677,7 +638,7 @@ * #logicalChainDuringResolution}. */ @VisibleForTesting - public static final class SymlinkFileValueWithoutStoredChain + final class SymlinkFileValueWithoutStoredChain extends DifferentRealPathFileValueWithoutStoredChain { private final PathFragment linkTarget;
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 6e85a9e..8d2a0a3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java
@@ -1486,16 +1486,17 @@ if (repoCacheFriendlyPath == null) { return; } + var recordedInput = new RepoRecordedInput.File(repoCacheFriendlyPath); + var skyKey = recordedInput.getSkyKey(directories); try { - var recordedInput = new RepoRecordedInput.File(repoCacheFriendlyPath); - FileValue fileValue = - (FileValue) env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class); + FileValue fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class); if (fileValue == null) { throw new NeedsSkyframeRestartException(); } recordedFileInputs.put( - recordedInput, RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); + recordedInput, + RepoRecordedInput.File.fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 62bbe82..a6a3363 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD
@@ -346,6 +346,7 @@ "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value", + "//src/main/java/com/google/devtools/build/lib/util:pair", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java index aad74b6..1ed1b3b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; import com.google.devtools.build.lib.skyframe.PackageLookupValue; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; @@ -74,6 +75,7 @@ private abstract static class BaseFileHandler { private final String filename; + private RootedPath rootedPath; private FileValue fileValue; private String fileContent; @@ -112,7 +114,9 @@ Transience.PERSISTENT); } else if (hasFile) { - fileValue = getFileValue(rule, env); + Pair<RootedPath, FileValue> rootedPathAndFileValue = getFileValue(rule, env); + rootedPath = rootedPathAndFileValue.getFirst(); + fileValue = rootedPathAndFileValue.getSecond(); if (env.valuesMissing()) { return false; } @@ -145,14 +149,14 @@ throws RepositoryFunctionException { if (fileValue != null) { // Link x/FILENAME to <build_root>/x.FILENAME. - symlinkFile(fileValue, filename, outputDirectory); + symlinkFile(rootedPath, fileValue, filename, outputDirectory); try { Label label = getFileAttributeAsLabel(rule); recordedInputValues.put( new RepoRecordedInput.File( RepoRecordedInput.RepoCacheFriendlyPath.createInsideWorkspace( label.getRepository(), label.toPathFragment())), - RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); + RepoRecordedInput.File.fileValueToMarkerValue(rootedPath, fileValue)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } @@ -172,7 +176,7 @@ } @Nullable - private FileValue getFileValue(Rule rule, Environment env) + private Pair<RootedPath, FileValue> getFileValue(Rule rule, Environment env) throws RepositoryFunctionException, InterruptedException { Label label = getFileAttributeAsLabel(rule); SkyKey pkgSkyKey = PackageLookupValue.key(label.getPackageIdentifier()); @@ -223,20 +227,23 @@ Transience.PERSISTENT); } - return fileValue; + return Pair.of(rootedFile, fileValue); } /** * Symlinks a file from the local filesystem into the external repository's root. * + * @param rootedPath {@link RootedPath} of the file to be linked in * @param fileValue {@link FileValue} representing the file to be linked in * @param outputDirectory the directory of the remote repository * @throws RepositoryFunctionException if the file specified does not exist or cannot be linked. */ - private static void symlinkFile(FileValue fileValue, String filename, Path outputDirectory) + private static void symlinkFile( + RootedPath rootedPath, FileValue fileValue, String filename, Path outputDirectory) throws RepositoryFunctionException { Path filePath = outputDirectory.getRelative(filename); - RepositoryFunction.createSymbolicLink(filePath, fileValue.realRootedPath().asPath()); + RepositoryFunction.createSymbolicLink( + filePath, fileValue.realRootedPath(rootedPath).asPath()); } }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 8da9d50..7dc9bde 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java
@@ -305,7 +305,8 @@ * for placing in a repository marker file. The file need not exist, and can be a file or a * directory. */ - public static String fileValueToMarkerValue(FileValue fileValue) throws IOException { + public static String fileValueToMarkerValue(RootedPath rootedPath, FileValue fileValue) + throws IOException { if (fileValue.isDirectory()) { return "DIR"; } @@ -316,7 +317,7 @@ byte[] digest = fileValue.realFileStateValue().getDigest(); if (digest == null) { // Fast digest not available, or it would have been in the FileValue. - digest = fileValue.realRootedPath().asPath().getDigest(); + digest = fileValue.realRootedPath(rootedPath).asPath().getDigest(); } return BaseEncoding.base16().lowerCase().encode(digest); } @@ -331,13 +332,13 @@ public boolean isUpToDate( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { + var skyKey = getSkyKey(directories); try { - FileValue fileValue = - (FileValue) env.getValueOrThrow(getSkyKey(directories), IOException.class); + FileValue fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class); if (fileValue == null) { return false; } - return oldValue.equals(fileValueToMarkerValue(fileValue)); + return oldValue.equals(fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue)); } catch (IOException e) { return false; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingFunction.java index eb01dcd..b31c0fa 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingFunction.java
@@ -38,7 +38,7 @@ return null; } - RootedPath realDirRootedPath = dirFileValue.realRootedPath(); + RootedPath realDirRootedPath = dirFileValue.realRootedPath(dirRootedPath); if (!dirFileValue.isDirectory()) { // Recall that the directory is assumed to exist (see DirectoryListingValue#key). throw new DirectoryListingFunctionException(new InconsistentFilesystemException(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java index d9e9f91..02da3e5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java
@@ -95,10 +95,11 @@ static DirectoryListingValue value(RootedPath dirRootedPath, FileValue dirFileValue, DirectoryListingStateValue realDirectoryListingStateValue) { - return dirFileValue.realRootedPath().equals(dirRootedPath) + RootedPath realRootedPath = dirFileValue.realRootedPath(dirRootedPath); + return realRootedPath.equals(dirRootedPath) ? new RegularDirectoryListingValue(realDirectoryListingStateValue) - : new DifferentRealPathDirectoryListingValue(dirFileValue.realRootedPath(), - realDirectoryListingStateValue); + : new DifferentRealPathDirectoryListingValue( + realRootedPath, realDirectoryListingStateValue); } /** Normal {@link DirectoryListingValue}. */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryTreeDigestFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryTreeDigestFunction.java index ae23c37..80ad4ab 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryTreeDigestFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryTreeDigestFunction.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.util.Fingerprint; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; @@ -53,7 +54,8 @@ .collect(toImmutableSet()); // Turn each entry into a FileValue. - ImmutableList<FileValue> fileValues = getFileValues(env, sortedDirents, rootedPath); + ImmutableList<Pair<RootedPath, FileValue>> fileValues = + getFileValues(env, sortedDirents, rootedPath); if (fileValues == null) { return null; } @@ -70,13 +72,15 @@ fp.addStrings(sortedDirents); fp.addStrings(subDirTreeDigests); try { - for (FileValue fileValue : fileValues) { + for (Pair<RootedPath, FileValue> rootedPathAndFileValue : fileValues) { + RootedPath direntRootedPath = rootedPathAndFileValue.getFirst(); + FileValue fileValue = rootedPathAndFileValue.getSecond(); fp.addInt(fileValue.realFileStateValue().getType().ordinal()); if (fileValue.isFile()) { byte[] digest = fileValue.realFileStateValue().getDigest(); if (digest == null) { // Fast digest not available, or it would have been in the FileValue. - digest = fileValue.realRootedPath().asPath().getDigest(); + digest = fileValue.realRootedPath(direntRootedPath).asPath().getDigest(); } fp.addBytes(digest); } @@ -89,7 +93,7 @@ } @Nullable - private static ImmutableList<FileValue> getFileValues( + private static ImmutableList<Pair<RootedPath, FileValue>> getFileValues( Environment env, ImmutableSet<String> sortedDirents, RootedPath rootedPath) throws InterruptedException { ImmutableSet<FileValue.Key> fileValueKeys = @@ -105,10 +109,9 @@ if (env.valuesMissing()) { return null; } - ImmutableList<FileValue> fileValues = + ImmutableList<Pair<RootedPath, FileValue>> fileValues = fileValueKeys.stream() - .map(result::get) - .map(FileValue.class::cast) + .map(k -> Pair.of((RootedPath) k.argument(), (FileValue) result.get(k))) .collect(toImmutableList()); if (env.valuesMissing()) { return null; @@ -118,11 +121,12 @@ @Nullable private static ImmutableList<String> getSubDirTreeDigests( - Environment env, ImmutableList<FileValue> fileValues) throws InterruptedException { + Environment env, ImmutableList<Pair<RootedPath, FileValue>> fileValues) + throws InterruptedException { ImmutableSet<SkyKey> dirTreeDigestValueKeys = fileValues.stream() - .filter(FileValue::isDirectory) - .map(fv -> DirectoryTreeDigestValue.key(fv.realRootedPath())) + .filter(p -> p.getSecond().isDirectory()) + .map(p -> DirectoryTreeDigestValue.key(p.getSecond().realRootedPath(p.getFirst()))) .collect(toImmutableSet()); SkyframeLookupResult result = env.getValuesAndExceptions(dirTreeDigestValueKeys); if (env.valuesMissing()) {
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 4d7d97d..1439311 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
@@ -196,14 +196,19 @@ } RootedPath rootedPathFromAncestors = - getChild(parentFileValue.realRootedPath(), baseName, parentRootedPath, rootedPath); + getChild( + parentFileValue.realRootedPath(parentRootedPath), + baseName, + parentRootedPath, + rootedPath); if (!parentFileValue.exists() || !parentFileValue.isDirectory()) { return new PartialResolutionResult( rootedPathFromAncestors, FileStateValue.NONEXISTENT_FILE_STATE_NODE); } - for (RootedPath parentPartialRootedPath : parentFileValue.logicalChainDuringResolution()) { + for (RootedPath parentPartialRootedPath : + parentFileValue.logicalChainDuringResolution(parentRootedPath)) { checkAndNotePathSeenDuringPartialResolution( getChild(parentPartialRootedPath, baseName, parentRootedPath, rootedPath), symlinkResolutionState,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java index 0151bc0..6aec2b6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
@@ -386,21 +386,24 @@ } } else { // Stat the file. + RootedPath rootedPath = traversal.root().asRootedPath(); FileValue fileValue = - (FileValue) - env.getValueOrThrow( - FileValue.key(traversal.root().asRootedPath()), IOException.class); + (FileValue) env.getValueOrThrow(FileValue.key(rootedPath), IOException.class); if (env.valuesMissing()) { return null; } - return toFileInfo(fileValue, env, traversal.root().asPath(), syscallCache); + return toFileInfo(rootedPath, fileValue, env, traversal.root().asPath(), syscallCache); } } @Nullable private static FileInfo toFileInfo( - FileValue fileValue, Environment env, Path path, SyscallCache syscallCache) + RootedPath rootedPath, + FileValue fileValue, + Environment env, + Path path, + SyscallCache syscallCache) throws IOException, InterruptedException { if (fileValue.unboundedAncestorSymlinkExpansionChain() != null) { SkyKey uniquenessKey = @@ -437,7 +440,7 @@ return new FileInfo( type, withDigest(fileValue.realFileStateValue(), path, syscallCache), - fileValue.realRootedPath(), + fileValue.realRootedPath(rootedPath), unresolvedLinkTarget); } @@ -715,7 +718,12 @@ } if (key instanceof FileValue.Key fileKey) { FileInfo fileInfo = - toFileInfo((FileValue) value, env, fileKey.argument().asPath(), syscallCache); + toFileInfo( + fileKey.argument(), + (FileValue) value, + env, + fileKey.argument().asPath(), + syscallCache); if (fileInfo != null) { childValues.add(resultForFileRoot(fileKey.argument(), fileInfo)); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java index fd7f68a..f27aa6c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java
@@ -111,7 +111,8 @@ List<String> lines; try { - lines = FileSystemUtils.readLines(fileValue.realRootedPath().asPath(), UTF_8); + lines = + FileSystemUtils.readLines(fileValue.realRootedPath(rootedMappingPath).asPath(), UTF_8); } catch (IOException e) { throw new PlatformMappingFunctionException(e); }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java index be65b02..0318317 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java
@@ -19,11 +19,9 @@ import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileContentsProxy; -import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithContentsProxy; import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithDigest; import com.google.devtools.build.lib.actions.FileValue; -import com.google.devtools.build.lib.actions.FileValue.RegularFileValue; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.Root; @@ -61,18 +59,16 @@ RootedPath path = RootedPath.toRootedPath(Root.fromPath(rootDirectory), scratch.file("foo", "bar")); - // Digest should be returned if the FileStateValue has it. - FileStateValue fsv = new RegularFileStateValueWithDigest(3, new byte[] {1, 2, 3, 4}); - FileValue fv = new RegularFileValue(path, fsv); - assertThat(RepoRecordedInput.File.fileValueToMarkerValue(fv)).isEqualTo("01020304"); + // Digest should be returned if the FileValue has it. + FileValue fv = new RegularFileStateValueWithDigest(3, new byte[] {1, 2, 3, 4}); + assertThat(RepoRecordedInput.File.fileValueToMarkerValue(path, fv)).isEqualTo("01020304"); // Digest should also be returned if the FileStateValue doesn't have it. FileStatus status = Mockito.mock(FileStatus.class); when(status.getLastChangeTime()).thenReturn(100L); when(status.getNodeId()).thenReturn(200L); - fsv = new RegularFileStateValueWithContentsProxy(3, FileContentsProxy.create(status)); - fv = new RegularFileValue(path, fsv); + fv = new RegularFileStateValueWithContentsProxy(3, FileContentsProxy.create(status)); String expectedDigest = BaseEncoding.base16().lowerCase().encode(path.asPath().getDigest()); - assertThat(RepoRecordedInput.File.fileValueToMarkerValue(fv)).isEqualTo(expectedDigest); + assertThat(RepoRecordedInput.File.fileValueToMarkerValue(path, fv)).isEqualTo(expectedDigest); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 7294283..c159dd5 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -257,8 +257,16 @@ private static FileValue valueForPathHelper(Root root, Path path, MemoizingEvaluator evaluator) throws InterruptedException { - PathFragment pathFragment = root.relativize(path); - RootedPath rootedPath = RootedPath.toRootedPath(root, pathFragment); + return valueForRootedPathHelper( + RootedPath.toRootedPath(root, root.relativize(path)), evaluator); + } + + private FileValue valueForRootedPath(RootedPath rootedPath) throws InterruptedException { + return valueForRootedPathHelper(rootedPath, makeEvaluator()); + } + + private static FileValue valueForRootedPathHelper( + RootedPath rootedPath, MemoizingEvaluator evaluator) throws InterruptedException { SkyKey key = FileValue.key(rootedPath); EvaluationResult<FileValue> result = evaluator.evaluate(ImmutableList.of(key), EVALUATION_OPTIONS); @@ -852,7 +860,8 @@ FileValue value = (FileValue) result.get(key); assertThat(value).isNotNull(); assertThat(value.exists()).isTrue(); - assertThat(value.realRootedPath().getRootRelativePath().getPathString()) + assertThat( + value.realRootedPath((RootedPath) key.argument()).getRootRelativePath().getPathString()) .isEqualTo("insideroot"); } @@ -1404,7 +1413,7 @@ fail(String.format("Evaluation error for %s: %s", key, result.getError())); } FileValue fileValue = (FileValue) result.get(key); - assertThat(fileValue.realRootedPath().asPath().toString()) + assertThat(fileValue.realRootedPath((RootedPath) key.argument()).asPath().toString()) .isEqualTo(pkgRoot.getRelative(expectedRealPathString).toString()); } @@ -1413,10 +1422,11 @@ symlink("a", "b"); symlink("b", "c"); directory("c"); - FileValue fileValue = valueForPath(path("a")); + RootedPath rootedPath = rootedPath("a"); + FileValue fileValue = valueForRootedPath(rootedPath); assertThat(fileValue).isInstanceOf(SymlinkFileValueWithStoredChain.class); assertThat(fileValue.getUnresolvedLinkTarget()).isEqualTo(PathFragment.create("b")); - assertThat(fileValue.logicalChainDuringResolution()) + assertThat(fileValue.logicalChainDuringResolution(rootedPath)) .containsExactly(rootedPath("a"), rootedPath("b"), rootedPath("c")) .inOrder(); } @@ -1426,10 +1436,11 @@ symlink("a", "b"); symlink("b", "c"); directory("c/d"); - FileValue fileValue = valueForPath(path("a/d")); + RootedPath rootedPath = rootedPath("a/d"); + FileValue fileValue = valueForRootedPath(rootedPath); assertThat(fileValue).isInstanceOf(DifferentRealPathFileValueWithStoredChain.class); - assertThat(fileValue.realRootedPath()).isEqualTo(rootedPath("c/d")); - assertThat(fileValue.logicalChainDuringResolution()) + assertThat(fileValue.realRootedPath(rootedPath)).isEqualTo(rootedPath("c/d")); + assertThat(fileValue.logicalChainDuringResolution(rootedPath)) .containsExactly(rootedPath("a/d"), rootedPath("b/d"), rootedPath("c/d")) .inOrder(); } @@ -1439,10 +1450,12 @@ symlink("a", "b"); symlink("b", "c"); file("c"); - FileValue fileValue = valueForPath(path("a")); + RootedPath rootedPath = rootedPath("a"); + FileValue fileValue = valueForRootedPath(rootedPath); assertThat(fileValue).isInstanceOf(SymlinkFileValueWithoutStoredChain.class); assertThat(fileValue.getUnresolvedLinkTarget()).isEqualTo(PathFragment.create("b")); - assertThrows(IllegalStateException.class, fileValue::logicalChainDuringResolution); + assertThrows( + IllegalStateException.class, () -> fileValue.logicalChainDuringResolution(rootedPath)); } @Test @@ -1450,10 +1463,12 @@ symlink("a", "b"); symlink("b", "c"); file("c/d"); - FileValue fileValue = valueForPath(path("a/d")); + RootedPath rootedPath = rootedPath("a/d"); + FileValue fileValue = valueForRootedPath(rootedPath); assertThat(fileValue).isInstanceOf(DifferentRealPathFileValueWithoutStoredChain.class); - assertThat(fileValue.realRootedPath()).isEqualTo(rootedPath("c/d")); - assertThrows(IllegalStateException.class, fileValue::logicalChainDuringResolution); + assertThat(fileValue.realRootedPath(rootedPath)).isEqualTo(rootedPath("c/d")); + assertThrows( + IllegalStateException.class, () -> fileValue.logicalChainDuringResolution(rootedPath)); } @Test @@ -1466,10 +1481,11 @@ directory("g"); symlink("g/f", "../h"); directory("h"); - FileValue fileValue = valueForPath(path("a/d")); + RootedPath rootedPath = rootedPath("a/d"); + FileValue fileValue = valueForRootedPath(rootedPath); assertThat(fileValue).isInstanceOf(DifferentRealPathFileValueWithStoredChain.class); - assertThat(fileValue.realRootedPath()).isEqualTo(rootedPath("h")); - assertThat(fileValue.logicalChainDuringResolution()) + assertThat(fileValue.realRootedPath(rootedPath)).isEqualTo(rootedPath("h")); + assertThat(fileValue.logicalChainDuringResolution(rootedPath)) .containsExactly( rootedPath("a/d"), rootedPath("b/d"),
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java index 8b75d59..eea8ccf 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java
@@ -29,11 +29,9 @@ public class FileValueTest { @Test public void testCodec() throws Exception { + // This test case assumes we have adequate coverage for FileStateValue serialization. SerializationTester serializationTester = new SerializationTester( - // Assume we have adequate coverage for FileStateValue serialization. - new FileValue.RegularFileValue( - FsUtils.TEST_ROOTED_PATH, FileStateValue.NONEXISTENT_FILE_STATE_NODE), new FileValue.DifferentRealPathFileValueWithUnboundedAncestorExpansion( FsUtils.TEST_ROOTED_PATH, FileStateValue.DIRECTORY_FILE_STATE_NODE,