[7.1.0] Parallelize TreeArtifactValue.visitTree across files instead of subdirectories.

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