Streamline uploading of stdout and stderr

buchgr · copybara-github · commit a6b5b0540a01 · 2019-08-02T02:39:34.000-07:00
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. Closes #9025. PiperOrigin-RevId: 261288280
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
@@ -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
@@ -38,7 +38,6 @@
 import com.google.common.base.Ascii;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Throwables;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
@@ -419,6 +418,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,34 +449,13 @@ 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;
-  }
   
   // Execution Cache API
 
