From fd7108e996edd107f4995e30d7640ddebf0368f9 Mon Sep 17 00:00:00 2001
From: Jakob Buchgraber <buchgr@google.com>
Date: Wed, 31 Jul 2019 12:41:43 +0200
Subject: [PATCH] Streamline uploading of stdout and stderr

This change removes extra upload logic for stdout/stderr. Besides
the streamlined code this also saves two round trips uploading an
action that produced both stdout and stderr.
---
 .../lib/remote/AbstractRemoteActionCache.java | 28 ++++++++++++++---
 .../build/lib/remote/GrpcRemoteCache.java     | 30 ++++---------------
 2 files changed, 29 insertions(+), 29 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 716ee4db69e010..03b417d3fba400 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
@@ -640,8 +640,10 @@ static class UploadManifest {
     private final Path execRoot;
     private final boolean allowSymlinks;
     private final boolean uploadSymlinks;
-    private final Map<Digest, Path> digestToFile;
-    private final Map<Digest, Chunker> digestToChunkers;
+    private final Map<Digest, Path> digestToFile = new HashMap<>();
+    private final Map<Digest, Chunker> digestToChunkers = new HashMap<>();
+    private Digest stderrDigest;
+    private Digest stdoutDigest;
 
     /**
      * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult
@@ -658,9 +660,17 @@ public UploadManifest(
       this.execRoot = execRoot;
       this.uploadSymlinks = uploadSymlinks;
       this.allowSymlinks = allowSymlinks;
+    }
 
-      this.digestToFile = new HashMap<>();
-      this.digestToChunkers = new HashMap<>();
+    public void setStdoutStderr(FileOutErr outErr) throws IOException {
+      if (outErr.getErrorPath().exists()) {
+        stderrDigest = digestUtil.compute(outErr.getErrorPath());
+        addFile(stderrDigest, outErr.getErrorPath());
+      }
+      if (outErr.getOutputPath().exists()) {
+        stdoutDigest = digestUtil.compute(outErr.getOutputPath());
+        addFile(stdoutDigest, outErr.getOutputPath());
+      }
     }
 
     /**
@@ -747,6 +757,16 @@ public Map<Digest, Chunker> getDigestToChunkers() {
       return digestToChunkers;
     }
 
+    @Nullable
+    public Digest getStdoutDigest() {
+      return stdoutDigest;
+    }
+
+    @Nullable
+    public Digest getStderrDigest() {
+      return stderrDigest;
+    }
+
     private void addFileSymbolicLink(Path file, PathFragment target) throws IOException {
       result
           .addOutputFileSymlinksBuilder()
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
index cd4b6051d6f2e9..5a1dbc356d170f 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -419,6 +419,7 @@ void upload(
             options.incompatibleRemoteSymlinks,
             options.allowSymlinkUpload);
     manifest.addFiles(files);
+    manifest.setStdoutStderr(outErr);
     manifest.addAction(actionKey, action, command);
 
     Map<HashCode, Chunker> filesToUpload = Maps.newHashMap();
@@ -449,35 +450,14 @@ void upload(
       uploader.uploadBlobs(filesToUpload, /*forceUpload=*/true);
     }
 
-    // TODO(olaola): inline small stdout/stderr here.
-    if (outErr.getErrorPath().exists()) {
-      Digest stderr = uploadFileContents(outErr.getErrorPath());
-      result.setStderrDigest(stderr);
+    if (manifest.getStderrDigest() != null) {
+      result.setStderrDigest(manifest.getStderrDigest());
     }
-    if (outErr.getOutputPath().exists()) {
-      Digest stdout = uploadFileContents(outErr.getOutputPath());
-      result.setStdoutDigest(stdout);
+    if (manifest.getStdoutDigest() != null) {
+      result.setStdoutDigest(manifest.getStdoutDigest());
     }
   }
 
-  /**
-   * Put the file contents cache if it is not already in it. No-op if the file is already stored in
-   * cache. The given path must be a full absolute path.
-   *
-   * @return The key for fetching the file contents blob from cache.
-   */
-  private Digest uploadFileContents(Path file) throws IOException, InterruptedException {
-    Digest digest = digestUtil.compute(file);
-    ImmutableSet<Digest> missing = getMissingDigests(ImmutableList.of(digest));
-    if (!missing.isEmpty()) {
-      uploader.uploadBlob(
-          HashCode.fromString(digest.getHash()),
-          Chunker.builder().setInput(digest.getSizeBytes(), file).build(),
-          /* forceUpload=*/ true);
-    }
-    return digest;
-  }
-
   Digest uploadBlob(byte[] blob) throws IOException, InterruptedException {
     Digest digest = digestUtil.compute(blob);
     ImmutableSet<Digest> missing = getMissingDigests(ImmutableList.of(digest));