From 01c046899cc38cf96f4b93ee921e64fe19a800e8 Mon Sep 17 00:00:00 2001
From: Googler <tjgq@google.com>
Date: Tue, 9 Jan 2024 08:13:38 -0800
Subject: [PATCH] [7.1.0] Avoid unnecessary overhead when determining whether
 an action input is a directory.

When building Bazel against a fully populated disk cache, this halves the CPU overhead incurred by the compact log (4% to 2%), mostly due to avoiding symlink resolution in the RemoteActionFileSystem.

PiperOrigin-RevId: 596938086
Change-Id: I6d548894fad5bf97f6a54516c5dca87d0b8d7a8f
---
 .../lib/exec/CompactSpawnLogContext.java      |  5 ++--
 .../lib/exec/ExpandedSpawnLogContext.java     |  3 ++-
 .../build/lib/exec/SpawnLogContext.java       | 26 +++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
index 771ad0ec199ab5..00b43d2f957ecd 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
@@ -18,6 +18,7 @@
 import static com.google.devtools.build.lib.exec.SpawnLogContext.getEnvironmentVariables;
 import static com.google.devtools.build.lib.exec.SpawnLogContext.getPlatform;
 import static com.google.devtools.build.lib.exec.SpawnLogContext.getSpawnMetricsProto;
+import static com.google.devtools.build.lib.exec.SpawnLogContext.isInputDirectory;
 
 import com.github.luben.zstd.ZstdOutputStream;
 import com.google.common.collect.ImmutableList;
@@ -274,7 +275,7 @@ private int logNestedSet(
               continue;
             }
             Path path = fileSystem.getPath(execRoot.getRelative(input.getExecPath()));
-            if (path.isDirectory()) {
+            if (isInputDirectory(input, inputMetadataProvider)) {
               builder.addDirectoryIds(logDirectory(input, path, inputMetadataProvider));
             } else {
               builder.addFileIds(logFile(input, path, inputMetadataProvider));
@@ -373,7 +374,7 @@ private int logRunfilesDirectory(
 
             Path path = fileSystem.getPath(execRoot.getRelative(input.getExecPath()));
 
-            if (path.isDirectory()) {
+            if (isInputDirectory(input, inputMetadataProvider)) {
               builder.addAllFiles(expandDirectory(path, runfilesPath, inputMetadataProvider));
               continue;
             }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java
index 233d7e87797d99..4cb72217e5d61e 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java
@@ -17,6 +17,7 @@
 import static com.google.devtools.build.lib.exec.SpawnLogContext.getEnvironmentVariables;
 import static com.google.devtools.build.lib.exec.SpawnLogContext.getPlatform;
 import static com.google.devtools.build.lib.exec.SpawnLogContext.getSpawnMetricsProto;
+import static com.google.devtools.build.lib.exec.SpawnLogContext.isInputDirectory;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.GoogleLogger;
@@ -168,7 +169,7 @@ public void logSpawn(
 
           Path contentPath = fileSystem.getPath(execRoot.getRelative(input.getExecPathString()));
 
-          if (contentPath.isDirectory()) {
+          if (isInputDirectory(input, inputMetadataProvider)) {
             listDirectoryContents(
                 displayPath, contentPath, builder::addInputs, inputMetadataProvider, isTool);
             continue;
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java
index d8d47e6e2e9580..fdba2073bbd3d3 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java
@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -20,6 +22,7 @@
 import com.google.common.hash.HashCode;
 import com.google.devtools.build.lib.actions.ActionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
@@ -102,6 +105,29 @@ static Platform getPlatform(Spawn spawn, RemoteOptions remoteOptions) throws Use
     return builder.build();
   }
 
+  /**
+   * Determines whether an action input is a directory, avoiding I/O if possible.
+   *
+   * <p>Do not call for action outputs.
+   */
+  static boolean isInputDirectory(ActionInput input, InputMetadataProvider inputMetadataProvider)
+      throws IOException {
+    if (input.isDirectory()) {
+      return true;
+    }
+    if (!(input instanceof SourceArtifact)) {
+      return false;
+    }
+    // A source artifact may be a directory in spite of claiming to be a file. Avoid unnecessary I/O
+    // by inspecting its metadata, which should have already been collected and cached.
+    FileArtifactValue metadata =
+        checkNotNull(
+            inputMetadataProvider.getInputMetadata(input),
+            "missing metadata for %s",
+            input.getExecPath());
+    return metadata.getType().isDirectory();
+  }
+
   /**
    * Computes the digest of an ActionInput or its path.
    *