Clean up `createRecursiveTraversalKeys` with pre-sizing, immutability and SkyKey Type. Then merge `createRecursiveTraversalKeys` into `traverseChildren` to be clearer. PiperOrigin-RevId: 436882926
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 972c15a..143931f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -1999,6 +1999,7 @@ ":action_execution_value", ":detailed_exceptions", ":directory_listing_value", + ":dirents", ":package_lookup_value", ":sky_functions", ":tree_artifact_value",
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 ed943ef..b723829 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
@@ -20,6 +20,7 @@ import com.google.common.base.Verify; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -64,7 +65,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -230,12 +230,8 @@ } // We are free to traverse this directory. - Collection<SkyKey> dependentKeys = createRecursiveTraversalKeys(env, traversal); - if (dependentKeys == null) { - return null; - } Collection<RecursiveFilesystemTraversalValue> subdirTraversals = - traverseChildren(env, dependentKeys, /*inline=*/ traversal.isRootGenerated); + traverseChildren(env, traversal); if (subdirTraversals == null) { return null; } @@ -563,55 +559,13 @@ } /** - * List the directory and create {@code SkyKey}s to request contents of its children recursively. - * - * <p>The returned keys are of type {@link SkyFunctions#RECURSIVE_FILESYSTEM_TRAVERSAL}. - */ - @Nullable - private static Collection<SkyKey> createRecursiveTraversalKeys( - Environment env, TraversalRequest traversal) throws InterruptedException, IOException { - // Use the traversal's path, even if it's a symlink. The contents of the directory, as listed - // in the result, must be relative to it. - Iterable<Dirent> dirents; - if (traversal.isRootGenerated) { - // If we're dealing with an output file, read the directory directly instead of creating - // filesystem nodes under the output tree. - List<Dirent> direntsCollection = - new ArrayList<>( - traversal.root.asRootedPath().asPath().readdir(Symlinks.FOLLOW)); - Collections.sort(direntsCollection); - dirents = direntsCollection; - } else { - DirectoryListingValue dirListingValue = - (DirectoryListingValue) - env.getValueOrThrow( - DirectoryListingValue.key(traversal.root.asRootedPath()), IOException.class); - if (dirListingValue == null) { - return null; - } - dirents = dirListingValue.getDirents(); - } - - List<SkyKey> result = new ArrayList<>(); - for (Dirent dirent : dirents) { - RootedPath childPath = - RootedPath.toRootedPath( - traversal.root.asRootedPath().getRoot(), - traversal.root.asRootedPath().getRootRelativePath().getRelative(dirent.getName())); - TraversalRequest childTraversal = traversal.forChildEntry(childPath); - result.add(childTraversal); - } - return result; - } - - /** * Creates result for a dangling symlink. * * @param linkName path to the symbolic link * @param info the {@link FileInfo} associated with the link file */ - private static RecursiveFilesystemTraversalValue resultForDanglingSymlink(RootedPath linkName, - FileInfo info) { + private static RecursiveFilesystemTraversalValue resultForDanglingSymlink( + RootedPath linkName, FileInfo info) { Preconditions.checkState(info.type.isSymlink() && !info.type.exists(), "{%s} {%s}", linkName, info.type); return RecursiveFilesystemTraversalValue.of( @@ -638,8 +592,10 @@ } } - private static RecursiveFilesystemTraversalValue resultForDirectory(TraversalRequest traversal, - FileInfo rootInfo, Collection<RecursiveFilesystemTraversalValue> subdirTraversals) { + private static RecursiveFilesystemTraversalValue resultForDirectory( + TraversalRequest traversal, + FileInfo rootInfo, + Collection<RecursiveFilesystemTraversalValue> subdirTraversals) { // Collect transitive closure of files in subdirectories. NestedSetBuilder<ResolvedFile> paths = NestedSetBuilder.stableOrder(); for (RecursiveFilesystemTraversalValue child : subdirTraversals) { @@ -677,22 +633,56 @@ return new HasDigest.ByteStringDigest(result); } - /** - * Requests Skyframe to compute the dependent values and returns them. - * - * <p>The keys must all be {@link SkyFunctions#RECURSIVE_FILESYSTEM_TRAVERSAL} keys. - */ + /** Requests Skyframe to compute the dependent values and returns them. */ @Nullable private Collection<RecursiveFilesystemTraversalValue> traverseChildren( - Environment env, Iterable<SkyKey> keys, boolean inline) - throws InterruptedException, RecursiveFilesystemTraversalFunctionException { - Map<SkyKey, SkyValue> values; - if (inline) { + Environment env, TraversalRequest traversal) + throws InterruptedException, RecursiveFilesystemTraversalFunctionException, IOException { + // Use the traversal's path, even if it's a symlink. The contents of the directory, as listed + // in the result, must be relative to it. + Iterable<Dirent> direntsIterable; + int direntsSize; + if (traversal.isRootGenerated) { + // If we're dealing with an output file, read the directory directly instead of creating + // filesystem nodes under the output tree. + List<Dirent> direntsCollection = + new ArrayList<>(traversal.root.asRootedPath().asPath().readdir(Symlinks.FOLLOW)); + Collections.sort(direntsCollection); + direntsIterable = direntsCollection; + direntsSize = direntsCollection.size(); + } else { + DirectoryListingValue dirListingValue = + (DirectoryListingValue) + env.getValueOrThrow( + DirectoryListingValue.key(traversal.root.asRootedPath()), IOException.class); + if (dirListingValue == null) { + return null; + } + Dirents dirents = dirListingValue.getDirents(); + direntsSize = dirents.size(); + direntsIterable = dirents; + } + + ImmutableList.Builder<TraversalRequest> keysBuilder = + ImmutableList.builderWithExpectedSize(direntsSize); + for (Dirent dirent : direntsIterable) { + RootedPath childPath = + RootedPath.toRootedPath( + traversal.root.asRootedPath().getRoot(), + traversal.root.asRootedPath().getRootRelativePath().getRelative(dirent.getName())); + TraversalRequest childTraversal = traversal.forChildEntry(childPath); + keysBuilder.add(childTraversal); + } + ImmutableList<TraversalRequest> keys = keysBuilder.build(); + if (keys == null) { + return null; + } + Map<SkyKey, SkyValue> values = Maps.newHashMapWithExpectedSize(keys.size()); + if (traversal.isRootGenerated) { // Don't create Skyframe nodes for a recursive traversal over the output tree. // Instead, inline the recursion in the top-level request. - values = new HashMap<>(); - for (SkyKey depKey : keys) { - values.put(depKey, compute(depKey, env)); + for (TraversalRequest traversalRequest : keys) { + values.put(traversalRequest, compute(traversalRequest, env)); } } else { values = env.getValues(keys);