From df07d275ce17f14224af0f1a5862755a7db44397 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 14 Feb 2024 01:37:10 -0800 Subject: [PATCH] Parallelize TreeArtifactValue.visitTree across files instead of subdirectories. 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 https://github.com/bazelbuild/bazel/commit/f2512a0718f3a652ec536b513be82324361bbe77 have been made). Also remove the edge case for the root directory; the code is cleaner that way. Related to https://github.com/bazelbuild/bazel/issues/17009. PiperOrigin-RevId: 606897861 Change-Id: I143d55a844ac191543a856f73849a95560199468 --- .../skyframe/ActionOutputMetadataStore.java | 4 -- .../build/lib/skyframe/TreeArtifactValue.java | 39 ++++++++----------- .../lib/skyframe/TreeArtifactValueTest.java | 36 ++++++++++------- 3 files changed, 40 insertions(+), 39 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index 13697b03fcfc4d..5aac578fe3cd33 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -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); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 0604e6a6beb34e..d2f8df1839ede6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -518,7 +518,7 @@ 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) { @@ -526,30 +526,25 @@ void run() throws IOException, InterruptedException { } } - // 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) { @@ -563,7 +558,7 @@ private void visitTree(PathFragment subdir) { * Recursively visits all descendants under a directory. * *

{@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. * *

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 diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index ce6dac4ad0af46..53ca1c98bf6820 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -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; @@ -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), @@ -405,17 +405,13 @@ public ImmutableList 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 = @@ -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); @@ -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)); @@ -470,7 +465,9 @@ 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 @@ -478,13 +475,20 @@ public void visitTree_throwsOnSymlinkPointingOutsideTree() throws Exception { Path treeDir = scratch.dir("tree"); scratch.file("outside"); scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../outside")); + List> 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"); } @@ -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> children = new ArrayList<>(); Exception e = assertThrows( @@ -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"); }