[7.1.0] Parallelize TreeArtifactValue.visitTree across files instead …

…of subdirectories. (#21347)

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

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

src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java

@@ -517,38 +517,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) {
@@ -562,7 +557,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

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<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 =
@@ -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,21 +466,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");
   }
 
@@ -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<Pair<PathFragment, Dirent.Type>> 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");
   }