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);
