From 45f06e631de453aec83a26d20fd602470faa87c0 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 14 Feb 2024 01:37:10 -0800 Subject: [PATCH] [7.1.0] 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 840068a35eb632..240b43d1e64911 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 99d1636b679180..fa19239b9330d2 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 @@ -517,7 +517,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) { @@ -525,30 +525,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) { @@ -562,7 +557,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 e950edc6189563..0e4dfb8de94d98 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 @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.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; @@ -383,6 +382,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), @@ -406,17 +406,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 = @@ -426,8 +422,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); @@ -451,6 +445,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)); @@ -471,7 +466,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 @@ -479,13 +476,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"); } @@ -494,6 +498,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( @@ -502,9 +507,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"); }