Skip to content

Commit

Permalink
Parallelize TreeArtifactValue.visitTree across files instead of subdi…
Browse files Browse the repository at this point in the history
…rectories.

This performs better when the subdirectories are unbalanced (and doesn't degrade catastrophically for a flat hierarchy). Most tree artifacts are too small for this to matter, but some users have very large ones (with hundreds of thousands of files) for which this can reduce the overall traversal time by 30% or more (after other, more important optimizations such as f2512a0 have been made).

Also remove the edge case for the root directory; the code is cleaner that way.

Related to #17009.

PiperOrigin-RevId: 606897861
Change-Id: I143d55a844ac191543a856f73849a95560199468
  • Loading branch information
tjgq authored and copybara-github committed Feb 14, 2024
1 parent bb80319 commit df07d27
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,6 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
return TreeArtifactValue.MISSING_TREE_ARTIFACT;
}

if (chmod) {
setPathPermissions(treeDir);
}

AtomicBoolean anyRemote = new AtomicBoolean(false);

TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -518,38 +518,33 @@ static class Visitor extends AbstractQueueVisitor {
}

void run() throws IOException, InterruptedException {
execute(() -> visitTree(PathFragment.EMPTY_FRAGMENT));
execute(() -> visit(PathFragment.EMPTY_FRAGMENT, Dirent.Type.DIRECTORY));
try {
awaitQuiescence(true);
} catch (IORuntimeException e) {
throw e.getCauseIOException();
}
}

// IOExceptions are wrapped in IORuntimeException so that it can be propagated to the main
// thread
private void visitTree(PathFragment subdir) {
private void visit(PathFragment parentRelativePath, Dirent.Type type) {
try {
for (Dirent dirent : parentDir.getRelative(subdir).readdir(Symlinks.NOFOLLOW)) {
PathFragment parentRelativePath = subdir.getChild(dirent.getName());
Dirent.Type type = dirent.getType();

if (type == Dirent.Type.UNKNOWN) {
throw new IOException(
"Could not determine type of file for "
+ parentRelativePath
+ " under "
+ parentDir);
}
Path path = parentDir.getRelative(parentRelativePath);

if (type == Dirent.Type.SYMLINK) {
checkSymlink(subdir, parentDir.getRelative(parentRelativePath));
}
if (type == Dirent.Type.UNKNOWN) {
throw new IOException("Could not determine type of file for " + path.getPathString());
}

if (type == Dirent.Type.SYMLINK) {
checkSymlink(parentRelativePath.getParentDirectory(), path);
}

visitor.visit(parentRelativePath, type);
visitor.visit(parentRelativePath, type);

if (type == Dirent.Type.DIRECTORY) {
execute(() -> visitTree(parentRelativePath));
if (type == Dirent.Type.DIRECTORY) {
for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) {
PathFragment childPath = parentRelativePath.getChild(dirent.getName());
Dirent.Type childType = dirent.getType();
execute(() -> visit(childPath, childType));
}
}
} catch (IOException e) {
Expand All @@ -563,7 +558,7 @@ private void visitTree(PathFragment subdir) {
* Recursively visits all descendants under a directory.
*
* <p>{@link TreeArtifactVisitor#visit} is invoked on {@code visitor} for each directory, file,
* and symlink under the given {@code parentDir}.
* and symlink under the given {@code parentDir}, including {@code parentDir} itself.
*
* <p>This method is intended to provide uniform semantics for constructing a tree artifact,
* including special logic that validates directory entries. Invalid directory entries include a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.spy;

Expand Down Expand Up @@ -382,6 +381,7 @@ public void visitTree_visitsEachChild() throws Exception {

assertThat(children)
.containsExactly(
Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("a/b"), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("file1"), Dirent.Type.FILE),
Expand All @@ -405,17 +405,13 @@ public ImmutableList<Dirent> readdir(PathFragment path, boolean followSymlinks)

Exception e =
assertThrows(
IOException.class,
() ->
TreeArtifactValue.visitTree(
treeDir, (child, type) -> fail("Should not be called")));
assertThat(e).hasMessageThat().contains("Could not determine type of file for ? under /tree");
IOException.class, () -> TreeArtifactValue.visitTree(treeDir, (child, type) -> {}));
assertThat(e).hasMessageThat().contains("Could not determine type of file for /tree/?");
}

@Test
public void visitTree_propagatesIoExceptionFromVisitor() throws Exception {
Path treeDir = scratch.dir("tree");
scratch.file("tree/file");
IOException e = new IOException("From visitor");

IOException thrown =
Expand All @@ -425,8 +421,6 @@ public void visitTree_propagatesIoExceptionFromVisitor() throws Exception {
TreeArtifactValue.visitTree(
treeDir,
(child, type) -> {
assertThat(child).isEqualTo(PathFragment.create("file"));
assertThat(type).isEqualTo(Dirent.Type.FILE);
throw e;
}));
assertThat(thrown).isSameInstanceAs(e);
Expand All @@ -450,6 +444,7 @@ public void visitTree_pemitsUpLevelSymlinkInsideTree() throws Exception {

assertThat(children)
.containsExactly(
Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("file"), Dirent.Type.FILE),
Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("a/up_link"), Dirent.Type.SYMLINK));
Expand All @@ -470,21 +465,30 @@ public void visitTree_permitsAbsoluteSymlink() throws Exception {
});

assertThat(children)
.containsExactly(Pair.of(PathFragment.create("absolute_link"), Dirent.Type.SYMLINK));
.containsExactly(
Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("absolute_link"), Dirent.Type.SYMLINK));
}

@Test
public void visitTree_throwsOnSymlinkPointingOutsideTree() throws Exception {
Path treeDir = scratch.dir("tree");
scratch.file("outside");
scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../outside"));
List<Pair<PathFragment, Dirent.Type>> children = new ArrayList<>();

Exception e =
assertThrows(
IOException.class,
() ->
TreeArtifactValue.visitTree(
treeDir, (child, type) -> fail("Should not be called")));
treeDir,
(child, type) -> {
synchronized (children) {
children.add(Pair.of(child, type));
}
}));
assertThat(children).containsExactly(Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY));
assertThat(e).hasMessageThat().contains("/tree/link pointing to ../outside");
}

Expand All @@ -493,6 +497,7 @@ public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throw
Path treeDir = scratch.dir("tree");
scratch.file("tree/file");
scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../tree/file"));
List<Pair<PathFragment, Dirent.Type>> children = new ArrayList<>();

Exception e =
assertThrows(
Expand All @@ -501,9 +506,14 @@ public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throw
TreeArtifactValue.visitTree(
treeDir,
(child, type) -> {
assertThat(child).isEqualTo(PathFragment.create("file"));
assertThat(type).isEqualTo(Dirent.Type.FILE);
synchronized (children) {
children.add(Pair.of(child, type));
}
}));
assertThat(children)
.containsExactly(
Pair.of(PathFragment.create(""), Dirent.Type.DIRECTORY),
Pair.of(PathFragment.create("file"), Dirent.Type.FILE));
assertThat(e).hasMessageThat().contains("/tree/link pointing to ../tree/file");
}

Expand Down

0 comments on commit df07d27

Please sign in to comment.