Remove unnecessary I/O operations from handling remotely executed actions (fixes performance regression #4749). Also adding Skylark tests for input/output directories.

TESTED=locally
RELNOTES: Fix performance regression
PiperOrigin-RevId: 188346410

4 files changed, 110 insertions, 11 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
index d0a05fa473..2828424d83 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
@@ -132,13 +132,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
         downloadFile(path, file.getDigest(), file.getIsExecutable(), file.getContent());
       }
       for (OutputDirectory dir : result.getOutputDirectoriesList()) {
-        Digest treeDigest = dir.getTreeDigest();
-        byte[] b = downloadBlob(treeDigest);
-        Digest receivedTreeDigest = digestUtil.compute(b);
-        if (!receivedTreeDigest.equals(treeDigest)) {
-          throw new IOException(
-              "Digest does not match " + receivedTreeDigest + " != " + treeDigest);
-        }
+        byte[] b = downloadBlob(dir.getTreeDigest());
         Tree tree = Tree.parseFrom(b);
         Map<Digest, Directory> childrenMap = new HashMap<>();
         for (Directory child : tree.getChildrenList()) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index fc27127ec7..1b7a0809d6 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -19,6 +19,7 @@ import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Spawn;
@@ -296,7 +297,7 @@ class RemoteSpawnRunner implements SpawnRunner {
     ArrayList<String> outputDirectoryPaths = new ArrayList<>();
     for (ActionInput output : outputs) {
       String pathString = output.getExecPathString();
-      if (execRoot.getRelative(pathString).isDirectory()) {
+      if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
         outputDirectoryPaths.add(pathString);
       } else {
         outputPaths.add(pathString);
diff --git a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java
index 2d2de7e92a..5763b25e51 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java
@@ -25,6 +25,7 @@ import com.google.common.collect.TreeTraverser;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.DigestOfDirectoryException;
 import com.google.devtools.build.lib.actions.cache.Metadata;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
@@ -311,10 +312,17 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
       // Leaf node reached. Must be unique.
       Preconditions.checkArgument(
           inputsStart == inputsEnd - 1, "Encountered two inputs with the same path.");
-      // TODO: check that the actionInput is a single file!
       ActionInput input = inputs.get(inputsStart);
-      Path leafPath = execRoot.getRelative(input.getExecPathString());
-      if (leafPath.isDirectory()) {
+      try {
+        if (!(input instanceof VirtualActionInput)
+            && Preconditions.checkNotNull(inputFileCache.getMetadata(input))
+                .getType()
+                .isDirectory()) {
+          Path leafPath = execRoot.getRelative(input.getExecPathString());
+          return interner.intern(new TreeNode(buildInputDirectoryEntries(leafPath), input));
+        }
+      } catch (DigestOfDirectoryException e) {
+        Path leafPath = execRoot.getRelative(input.getExecPathString());
         return interner.intern(new TreeNode(buildInputDirectoryEntries(leafPath), input));
       }
       return interner.intern(new TreeNode(input));
diff --git a/src/test/shell/bazel/remote_execution_http_test.sh b/src/test/shell/bazel/remote_execution_http_test.sh
index 5b978bd129..ddec68109f 100755
--- a/src/test/shell/bazel/remote_execution_http_test.sh
+++ b/src/test/shell/bazel/remote_execution_http_test.sh
@@ -205,4 +205,100 @@ function test_directory_artifact_rest_cache() {
       || fail "Remote cache generated different result"
 }
 
+function set_directory_artifact_skylark_testfixtures() {
+  mkdir -p a
+  cat > a/rule.bzl <<'EOF'
+def _gen_output_dir_impl(ctx):
+  output_dir = ctx.actions.declare_directory(ctx.attr.outdir)
+
+  ctx.actions.run_shell(
+      outputs = [output_dir],
+      inputs = [],
+      command = """
+        mkdir -p $1/sub1; \
+        echo "Hello, world!" > $1/foo.txt; \
+        echo "Shuffle, duffle, muzzle, muff" > $1/sub1/bar.txt
+      """,
+      arguments = [output_dir.path],
+  )
+  return [
+      DefaultInfo(files=depset(direct=[output_dir])),
+  ]
+
+gen_output_dir = rule(
+    implementation = _gen_output_dir_impl,
+    attrs = {
+        "outdir": attr.string(mandatory = True),
+    },
+)
+EOF
+  cat > a/BUILD <<'EOF'
+package(default_visibility = ["//visibility:public"])
+load("//a:rule.bzl", "gen_output_dir")
+
+gen_output_dir(
+    name = "output_dir",
+    outdir = "dir",
+)
+
+genrule(
+    name = "test",
+    srcs = [":output_dir"],
+    outs = ["qux"],
+    cmd = "mkdir $@ && paste -d\"\n\" $(location :output_dir)/foo.txt $(location :output_dir)/sub1/bar.txt > $@/out.txt",
+)
+EOF
+
+  cat > a/test_expected <<EOF
+Hello, world!
+Shuffle, duffle, muzzle, muff
+EOF
+}
+
+function test_directory_artifact_skylark_local() {
+  set_directory_artifact_skylark_testfixtures
+
+  bazel build //a:test >& $TEST_log \
+    || fail "Failed to build //a:test without remote execution"
+  diff bazel-genfiles/a/qux/out.txt a/test_expected \
+      || fail "Local execution generated different result"
+}
+
+function test_directory_artifact_skylark() {
+  set_directory_artifact_skylark_testfixtures
+
+  bazel build \
+      --spawn_strategy=remote \
+      --remote_executor=localhost:${worker_port} \
+      --remote_cache=localhost:${worker_port} \
+      //a:test >& $TEST_log \
+      || fail "Failed to build //a:test with remote execution"
+  diff bazel-genfiles/a/qux/out.txt a/test_expected \
+      || fail "Remote execution generated different result"
+}
+
+function test_directory_artifact_skylark_grpc_cache() {
+  set_directory_artifact_skylark_testfixtures
+
+  bazel build \
+      --spawn_strategy=remote \
+      --remote_cache=localhost:${worker_port} \
+      //a:test >& $TEST_log \
+      || fail "Failed to build //a:test with remote gRPC cache"
+  diff bazel-genfiles/a/qux/out.txt a/test_expected \
+      || fail "Remote cache generated different result"
+}
+
+function test_directory_artifact_skylark_rest_cache() {
+  set_directory_artifact_skylark_testfixtures
+
+  bazel build \
+      --spawn_strategy=remote \
+      --remote_rest_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
+      //a:test >& $TEST_log \
+      || fail "Failed to build //a:test with remote REST cache"
+  diff bazel-genfiles/a/qux/out.txt a/test_expected \
+      || fail "Remote cache generated different result"
+}
+
 run_suite "Remote execution and remote cache tests"