Avoid recomputing hashes of Action and Command.
To check the cache for an action, we have to hash its protobuf. Therefore, when uploading an action, we can avoid rehashing it and its command.
Closes #6227.
PiperOrigin-RevId: 214980888
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 34c9cc6..cc82591 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
@@ -486,13 +486,22 @@
* Adds an action and command protos to upload. They need to be uploaded as part of the action
* result.
*/
- public void addAction(Action action, Command command) throws IOException {
- for (byte[] blob : new byte[][]{action.toByteArray(), command.toByteArray()}) {
- Digest digest = digestUtil.compute(blob);
- Chunker chunker =
- Chunker.builder(digestUtil).setInput(digest, blob).setChunkSize(blob.length).build();
- digestToChunkers.put(digest, chunker);
- }
+ public void addAction(DigestUtil.ActionKey actionKey, Action action, Command command)
+ throws IOException {
+ byte[] actionBlob = action.toByteArray();
+ digestToChunkers.put(
+ actionKey.getDigest(),
+ Chunker.builder(digestUtil)
+ .setInput(actionKey.getDigest(), actionBlob)
+ .setChunkSize(actionBlob.length)
+ .build());
+ byte[] commandBlob = command.toByteArray();
+ digestToChunkers.put(
+ action.getCommandDigest(),
+ Chunker.builder(digestUtil)
+ .setInput(action.getCommandDigest(), commandBlob)
+ .setChunkSize(commandBlob.length)
+ .build());
}
/** Map of digests to file paths to upload. */
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 bf1bb2a..4c97ce1 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
@@ -261,7 +261,7 @@
boolean uploadAction)
throws ExecException, IOException, InterruptedException {
ActionResult.Builder result = ActionResult.newBuilder();
- upload(execRoot, action, command, files, outErr, uploadAction, result);
+ upload(execRoot, actionKey, action, command, files, outErr, uploadAction, result);
if (!uploadAction) {
return;
}
@@ -286,6 +286,7 @@
void upload(
Path execRoot,
+ ActionKey actionKey,
Action action,
Command command,
Collection<Path> files,
@@ -297,7 +298,7 @@
new UploadManifest(digestUtil, result, execRoot, options.allowSymlinkUpload);
manifest.addFiles(files);
if (uploadAction) {
- manifest.addAction(action, command);
+ manifest.addAction(actionKey, action, command);
}
List<Chunker> filesToUpload = new ArrayList<>();
diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
index e9cf903..59b4ac9 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
@@ -129,7 +129,7 @@
boolean uploadAction)
throws ExecException, IOException, InterruptedException {
ActionResult.Builder result = ActionResult.newBuilder();
- upload(result, action, command, execRoot, files, uploadAction);
+ upload(result, actionKey, action, command, execRoot, files, uploadAction);
if (outErr.getErrorPath().exists()) {
Digest stderr = uploadFileContents(outErr.getErrorPath());
result.setStderrDigest(stderr);
@@ -145,6 +145,7 @@
public void upload(
ActionResult.Builder result,
+ DigestUtil.ActionKey actionKey,
Action action,
Command command,
Path execRoot,
@@ -155,7 +156,7 @@
new UploadManifest(digestUtil, result, execRoot, options.allowSymlinkUpload);
manifest.addFiles(files);
if (uploadAction) {
- manifest.addAction(action, command);
+ manifest.addAction(actionKey, action, command);
}
for (Map.Entry<Digest, Path> entry : manifest.getDigestToFile().entrySet()) {
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
index 21f3a25..a383719 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
@@ -661,7 +661,7 @@
ActionResult.Builder result = ActionResult.newBuilder();
client.upload(
- execRoot, null, null, ImmutableList.<Path>of(fooFile, barDir), outErr, false, result);
+ execRoot, null, null, null, ImmutableList.<Path>of(fooFile, barDir), outErr, false, result);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
@@ -690,7 +690,8 @@
});
ActionResult.Builder result = ActionResult.newBuilder();
- client.upload(execRoot, null, null, ImmutableList.<Path>of(barDir), outErr, false, result);
+ client.upload(
+ execRoot, null, null, null, ImmutableList.<Path>of(barDir), outErr, false, result);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
assertThat(result.build()).isEqualTo(expectedResult.build());
@@ -741,7 +742,8 @@
});
ActionResult.Builder result = ActionResult.newBuilder();
- client.upload(execRoot, null, null, ImmutableList.<Path>of(barDir), outErr, false, result);
+ client.upload(
+ execRoot, null, null, null, ImmutableList.<Path>of(barDir), outErr, false, result);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
assertThat(result.build()).isEqualTo(expectedResult.build());
@@ -777,7 +779,14 @@
ActionResult.Builder result = ActionResult.newBuilder();
client.upload(
- execRoot, action, command, ImmutableList.<Path>of(fooFile, barFile), outErr, true, result);
+ execRoot,
+ DIGEST_UTIL.asActionKey(actionDigest),
+ action,
+ command,
+ ImmutableList.<Path>of(fooFile, barFile),
+ outErr,
+ true,
+ result);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
expectedResult
diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
index 68ea866..2c05941 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
@@ -364,7 +364,14 @@
final SimpleBlobStoreActionCache client = newClient(map);
ActionResult.Builder result = ActionResult.newBuilder();
- client.upload(result, action, cmd, execRoot, ImmutableList.<Path>of(fooFile, barDir), true);
+ client.upload(
+ result,
+ DIGEST_UTIL.asActionKey(actionDigest),
+ action,
+ cmd,
+ execRoot,
+ ImmutableList.<Path>of(fooFile, barDir),
+ true);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
@@ -391,7 +398,7 @@
final SimpleBlobStoreActionCache client = newClient(map);
ActionResult.Builder result = ActionResult.newBuilder();
- client.upload(result, null, null, execRoot, ImmutableList.<Path>of(barDir), false);
+ client.upload(result, null, null, null, execRoot, ImmutableList.<Path>of(barDir), false);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
assertThat(result.build()).isEqualTo(expectedResult.build());
@@ -434,7 +441,7 @@
final Path barDir = execRoot.getRelative("bar");
ActionResult.Builder result = ActionResult.newBuilder();
- client.upload(result, null, null, execRoot, ImmutableList.<Path>of(barDir), false);
+ client.upload(result, null, null, null, execRoot, ImmutableList.<Path>of(barDir), false);
ActionResult.Builder expectedResult = ActionResult.newBuilder();
expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
assertThat(result.build()).isEqualTo(expectedResult.build());
diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
index d464ec8..de76ed8 100644
--- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
+++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
@@ -250,6 +250,7 @@
throws IOException, InterruptedException, StatusException {
Command command = null;
Action action = null;
+ ActionKey actionKey = digestUtil.asActionKey(actionDigest);
try {
action = Action.parseFrom(getFromFuture(cache.downloadBlob(actionDigest)));
command = Command.parseFrom(getFromFuture(cache.downloadBlob(action.getCommandDigest())));
@@ -328,7 +329,7 @@
ActionResult.Builder result = ActionResult.newBuilder();
boolean setResult = exitCode == 0 && !action.getDoNotCache();
try {
- cache.upload(result, action, command, execRoot, outputs, setResult);
+ cache.upload(result, actionKey, action, command, execRoot, outputs, setResult);
} catch (ExecException e) {
if (errStatus == null) {
errStatus =
@@ -347,7 +348,6 @@
resp.setStatus(errStatus);
throw new ExecutionStatusException(errStatus, resp.build());
} else if (setResult) {
- ActionKey actionKey = digestUtil.computeActionKey(action);
cache.setCachedActionResult(actionKey, finalResult);
}
return finalResult;
