Skip to content

Commit

Permalink
Clean up createRecursiveTraversalKeys with pre-sizing, immutability…
Browse files Browse the repository at this point in the history 
… and SkyKey Type. Then merge `createRecursiveTraversalKeys` into `traverseChildren` to be clearer.

PiperOrigin-RevId: 436882926
  • Loading branch information
@emilyguo1
emilyguo1 authored and Copybara-Service committed Mar 24, 2022
1 parent 8820e84 commit c07e66d
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 64 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1999,6 +1999,7 @@ java_library(
":action_execution_value",
":detailed_exceptions",
":directory_listing_value",
":dirents",
":package_lookup_value",
":sky_functions",
":tree_artifact_value",
Expand Down
118 changes: 54 additions & 64 deletions ...ain/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -230,12 +230,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

// 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;
}
Expand Down Expand Up @@ -562,56 +558,14 @@ private static PkgLookupResult checkIfPackage(
}
}

/**
* 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(
Expand All @@ -638,8 +592,10 @@ private static RecursiveFilesystemTraversalValue resultForFileRoot(RootedPath pa
}
}

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) {
Expand Down Expand Up @@ -677,22 +633,56 @@ private static HasDigest hashDirectorySymlink(
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);
Expand Down

0 comments on commit c07e66d

Please sign in to comment.