From 638e72bac8ee240e9a237699893a8907aec6830d Mon Sep 17 00:00:00 2001
From: Googler <tjgq@google.com>
Date: Tue, 6 Feb 2024 01:38:45 -0800
Subject: [PATCH] [7.1.0] Optimize RemoteActionFileSystem#readdir for the tree
 artifact input case.

As explained in the comment, we don't need to fall back to other sources when the directory is determined to belong to an input tree artifact.

(This remains a valid optimization even when cherry-picked to 7.x or earlier, since two nested artifacts are always produced by the same action, so they're either both inputs or both outputs.)

PiperOrigin-RevId: 604571427
Change-Id: I7e28df8baed543e85a26041aa10c027dcd28d9ad
---
 .../lib/remote/RemoteActionFileSystem.java    | 37 +++++++++++--------
 .../remote/RemoteActionFileSystemTest.java    | 26 +++++++++++--
 2 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
index d7e130d8709bbc..ad29e61a593972 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -782,7 +782,7 @@ private <T extends Comparable<T>> ImmutableSortedSet<T> getDirectoryContents(
     path = resolveSymbolicLinks(path).asFragment();
 
     HashMap<String, Dirent> entries = new HashMap<>();
-    boolean found = false;
+    boolean exists = false;
 
     if (path.startsWith(execRoot)) {
       var execPath = path.relativeTo(execRoot);
@@ -791,31 +791,38 @@ private <T extends Comparable<T>> ImmutableSortedSet<T> getDirectoryContents(
         for (var entry : treeEntries) {
           entries.put(entry.getName(), entry);
         }
-        found = true;
+        exists = true;
       }
     }
 
-    if (isOutput(path)) {
+    // Since actions are assumed not to modify their inputs, a directory belonging to an input tree
+    // artifact cannot also contain an output, so we can safely skip the other sources.
+    if (!exists) {
+      if (isOutput(path)) {
+        try {
+          for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) {
+            entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
+            entries.put(entry.getName(), entry);
+          }
+          exists = true;
+        } catch (FileNotFoundException ignored) {
+          // Will be rethrown below if directory does not exist in any of the sources.
+        }
+      }
+
       try {
-        for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) {
+        for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) {
           entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
           entries.put(entry.getName(), entry);
         }
-        found = true;
+        exists = true;
       } catch (FileNotFoundException ignored) {
-        // Will be rethrown below if directory exists on neither side.
+        // Will be rethrown below if directory does not exist in any of the sources.
       }
     }
 
-    try {
-      for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) {
-        entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
-        entries.put(entry.getName(), entry);
-      }
-    } catch (FileNotFoundException e) {
-      if (!found) {
-        throw e;
-      }
+    if (!exists) {
+      throw new FileNotFoundException(path.getPathString() + " (No such file or directory)");
     }
 
     // Sort entries to get a deterministic order.
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
index 228f4c76adbe35..ca6ffae8eeaaf4 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
@@ -658,9 +658,17 @@ public void readdir_fromInputArtifactData() throws Exception {
   }
 
   @Test
-  public void readdir_fromMultipleSources() throws Exception {
+  public void readdir_fromInputArtifactData_emptyDir() throws Exception {
+    ActionInputMap inputs = new ActionInputMap(1);
+    createLocalTreeArtifact("tree", ImmutableMap.of(), inputs);
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);
+
+    assertReaddir(actionFs, getOutputPath("tree"), /* followSymlinks= */ true);
+  }
+
+  @Test
+  public void readdir_fromRemoteOutputTreeAndLocalFilesystem() throws Exception {
     ActionInputMap inputs = new ActionInputMap(1);
-    createLocalTreeArtifact("dir", ImmutableMap.of("subdir/subfile", ""), inputs);
     RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);
     writeLocalFile(actionFs, getOutputPath("dir/out1"), "contents1");
     injectRemoteFile(actionFs, getOutputPath("dir/out2"), "contents2");
@@ -671,8 +679,18 @@ public void readdir_fromMultipleSources() throws Exception {
         dirPath,
         /* followSymlinks= */ true,
         new Dirent("out1", Dirent.Type.FILE),
-        new Dirent("out2", Dirent.Type.FILE),
-        new Dirent("subdir", Dirent.Type.DIRECTORY));
+        new Dirent("out2", Dirent.Type.FILE));
+  }
+
+  @Test
+  public void readdir_fromRemoteOutputTreeAndLocalFilesystem_emptyDir() throws Exception {
+    ActionInputMap inputs = new ActionInputMap(1);
+    RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);
+    PathFragment dirPath = getOutputPath("dir");
+    actionFs.getRemoteOutputTree().getPath(dirPath).createDirectoryAndParents();
+    actionFs.getLocalFileSystem().getPath(dirPath).createDirectoryAndParents();
+
+    assertReaddir(actionFs, dirPath, /* followSymlinks= */ true);
   }
 
   @Test