From 3f11b34d4c614cbf1c9311984b48c0426d4280b1 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Thu, 1 Feb 2024 18:19:44 +0000 Subject: [PATCH] [7.1.0] Include the digest hash function in the compact execution log. (#21174) The digest function is the same for every digest computed by an invocation. To save space, it is stored once in the Invocation log entry. Also embed a Digest proto in File and Spawn messages, with the proviso that the hash_function_name field will be empty. The size is always required because the hash alone might not be sufficient to retrieve an entry from a remote cache. PiperOrigin-RevId: 602684865 Change-Id: I6f4882051aad9ea72bea0232f8ad4b2cd7b8e335 --- .../lib/exec/CompactSpawnLogContext.java | 54 +++++++++++++------ .../lib/exec/ExpandedSpawnLogContext.java | 17 ++++-- .../build/lib/exec/SpawnLogContext.java | 9 +++- src/main/protobuf/spawn.proto | 25 +++++---- .../lib/exec/CompactSpawnLogContextTest.java | 51 ++++++++++-------- .../lib/exec/SpawnLogContextTestBase.java | 2 +- 6 files changed, 103 insertions(+), 55 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 00b43d2f957ecd..93eab45bb15538 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 @@ -95,6 +95,8 @@ public CompactSpawnLogContext( this.digestHashFunction = digestHashFunction; this.xattrProvider = xattrProvider; this.outputStream = getOutputStream(outputPath); + + logInvocation(); } private static MessageOutputStream getOutputStream(Path path) throws IOException { @@ -104,6 +106,16 @@ private static MessageOutputStream getOutputStream(Path path) thro path.toString(), new ZstdOutputStream(new BufferedOutputStream(path.getOutputStream()))); } + private void logInvocation() throws IOException { + logEntry( + null, + () -> + ExecLogEntry.newBuilder() + .setInvocation( + ExecLogEntry.Invocation.newBuilder() + .setHashFunctionName(digestHashFunction.toString()))); + } + @Override public void logSpawn( Spawn spawn, @@ -156,7 +168,7 @@ public void logSpawn( builder.setRemoteCacheable(Spawns.mayBeCachedRemotely(spawn)); if (result.getDigest() != null) { - builder.setDigest(result.getDigest().getHash()); + builder.setDigest(result.getDigest().toBuilder().clearHashFunctionName().build()); } builder.setTimeoutMillis(timeout.toMillis()); @@ -306,9 +318,15 @@ private int logFile(ActionInput input, Path path, InputMetadataProvider inputMet builder.setPath(input.getExecPathString()); Digest digest = - computeDigest(input, path, inputMetadataProvider, xattrProvider, digestHashFunction); - builder.setDigest(digest.getHash()); - builder.setSizeBytes(digest.getSizeBytes()); + computeDigest( + input, + path, + inputMetadataProvider, + xattrProvider, + digestHashFunction, + /* includeHashFunctionName= */ false); + + builder.setDigest(digest); return ExecLogEntry.newBuilder().setFile(builder); }); @@ -381,13 +399,15 @@ private int logRunfilesDirectory( Digest digest = computeDigest( - input, path, inputMetadataProvider, xattrProvider, digestHashFunction); + input, + path, + inputMetadataProvider, + xattrProvider, + digestHashFunction, + /* includeHashFunctionName= */ false); builder.addFiles( - ExecLogEntry.File.newBuilder() - .setPath(runfilesPath) - .setSizeBytes(digest.getSizeBytes()) - .setDigest(digest.getHash())); + ExecLogEntry.File.newBuilder().setPath(runfilesPath).setDigest(digest)); } return ExecLogEntry.newBuilder().setDirectory(builder); @@ -422,14 +442,14 @@ private ImmutableList expandDirectory( Digest digest = computeDigest( - /* input= */ null, child, inputMetadataProvider, xattrProvider, digestHashFunction); - - builder.add( - ExecLogEntry.File.newBuilder() - .setPath(childPath) - .setSizeBytes(digest.getSizeBytes()) - .setDigest(digest.getHash()) - .build()); + /* input= */ null, + child, + inputMetadataProvider, + xattrProvider, + digestHashFunction, + /* includeHashFunctionName= */ false); + + builder.add(ExecLogEntry.File.newBuilder().setPath(childPath).setDigest(digest).build()); } } return builder.build(); 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 9725c08732d17a..be90501d3496d9 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 @@ -177,7 +177,12 @@ public void logSpawn( Digest digest = computeDigest( - input, contentPath, inputMetadataProvider, xattrProvider, digestHashFunction); + input, + contentPath, + inputMetadataProvider, + xattrProvider, + digestHashFunction, + /* includeHashFunctionName= */ true); builder .addInputsBuilder() @@ -213,7 +218,12 @@ public void logSpawn( .setPath(output.getExecPathString()) .setDigest( computeDigest( - output, path, inputMetadataProvider, xattrProvider, digestHashFunction)); + output, + path, + inputMetadataProvider, + xattrProvider, + digestHashFunction, + /* includeHashFunctionName= */ true)); } } catch (IOException ex) { logger.atWarning().withCause(ex).log("Error computing spawn output properties"); @@ -332,7 +342,8 @@ private void listDirectoryContents( childContentPath, inputMetadataProvider, xattrProvider, - digestHashFunction)) + digestHashFunction, + /* includeHashFunctionName= */ true)) .setIsTool(isTool) .build()); } 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 fdba2073bbd3d3..6b2967c241fe80 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 @@ -139,9 +139,14 @@ static Digest computeDigest( Path path, InputMetadataProvider inputMetadataProvider, XattrProvider xattrProvider, - DigestHashFunction digestHashFunction) + DigestHashFunction digestHashFunction, + boolean includeHashFunctionName) throws IOException { - Digest.Builder builder = Digest.newBuilder().setHashFunctionName(digestHashFunction.toString()); + Digest.Builder builder = Digest.newBuilder(); + + if (includeHashFunctionName) { + builder.setHashFunctionName(digestHashFunction.toString()); + } if (input != null) { if (input instanceof VirtualActionInput) { diff --git a/src/main/protobuf/spawn.proto b/src/main/protobuf/spawn.proto index e5494057f728b3..d5c3d21c9b04aa 100644 --- a/src/main/protobuf/spawn.proto +++ b/src/main/protobuf/spawn.proto @@ -28,16 +28,13 @@ option java_outer_classname = "Protos"; // Digest of a file or action cache entry. message Digest { - // The content hash. + // The content hash as a lowercase hex string including any leading zeroes. string hash = 1; // The original content size in bytes. int64 size_bytes = 2; - // The digest function that was used to generate the hash. - // This is not an enum for compatibility reasons, and also because the - // purpose of these logs is to enable analysis by comparison of multiple - // builds. So, from the programmatic perspective, this is an opaque field. + // The name of the digest function used to compute the hash. string hash_function_name = 3; } @@ -46,6 +43,7 @@ message File { string path = 1; // File digest. + // May be omitted for empty files. Digest digest = 2; // Whether the file is a tool. @@ -211,16 +209,19 @@ message SpawnExec { message ExecLogEntry { // Information pertaining to the entire invocation. // May appear at most once in the initial position. - message Invocation {} + message Invocation { + // The hash function used to compute digests. + string hash_function_name = 1; + } // An input or output file. message File { // The file path. string path = 1; - // The file size in bytes. - int64 size_bytes = 2; - // A digest of the file contents. Unset if the file is empty. - string digest = 3; + // A digest of the file contents. + // The hash function name is omitted. It can be obtained from Invocation. + // May be omitted for empty files. + Digest digest = 2; } // An input or output directory. @@ -305,7 +306,9 @@ message ExecLogEntry { bool remote_cacheable = 15; // See SpawnExec.digest. - string digest = 16; + // The hash function name is omitted. It can be obtained from Invocation. + // Unset if the file is empty. + Digest digest = 16; // See SpawnExec.timeout_millis. int64 timeout_millis = 17; diff --git a/src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java b/src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java index 52d21f3cf0e952..a60379acd31ed7 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.exec.Protos.Digest; import com.google.devtools.build.lib.exec.Protos.ExecLogEntry; import com.google.devtools.build.lib.exec.Protos.File; import com.google.devtools.build.lib.exec.Protos.SpawnExec; @@ -128,14 +127,20 @@ protected void closeAndAssertLog(SpawnLogContext context, SpawnExec... expected) context.close(); HashMap entryMap = new HashMap<>(); + String hashFunctionName = ""; ArrayList actual = new ArrayList<>(); try (InputStream in = new ZstdInputStream(logPath.getInputStream())) { ExecLogEntry e; while ((e = ExecLogEntry.parseDelimitedFrom(in)) != null) { entryMap.put(e.getId(), e); + + if (e.hasInvocation()) { + hashFunctionName = e.getInvocation().getHashFunctionName(); + } + if (e.hasSpawn()) { - actual.add(reconstructSpawnExec(e.getSpawn(), entryMap)); + actual.add(reconstructSpawnExec(e.getSpawn(), hashFunctionName, entryMap)); } } } @@ -144,7 +149,7 @@ protected void closeAndAssertLog(SpawnLogContext context, SpawnExec... expected) } private SpawnExec reconstructSpawnExec( - ExecLogEntry.Spawn entry, Map entryMap) { + ExecLogEntry.Spawn entry, String hashFunctionName, Map entryMap) { SpawnExec.Builder builder = SpawnExec.newBuilder() .addAllCommandArgs(entry.getArgsList()) @@ -165,8 +170,10 @@ private SpawnExec reconstructSpawnExec( builder.setPlatform(entry.getPlatform()); } - SortedMap inputs = reconstructInputs(entry.getInputSetId(), entryMap); - SortedMap toolInputs = reconstructInputs(entry.getToolSetId(), entryMap); + SortedMap inputs = + reconstructInputs(entry.getInputSetId(), hashFunctionName, entryMap); + SortedMap toolInputs = + reconstructInputs(entry.getToolSetId(), hashFunctionName, entryMap); for (Map.Entry e : inputs.entrySet()) { File file = e.getValue(); @@ -181,14 +188,14 @@ private SpawnExec reconstructSpawnExec( case FILE_ID: ExecLogEntry.File file = checkNotNull(entryMap.get(output.getFileId())).getFile(); builder.addListedOutputs(file.getPath()); - builder.addActualOutputs(reconstructFile(/* parentDir= */ null, file)); + builder.addActualOutputs(reconstructFile(/* parentDir= */ null, file, hashFunctionName)); break; case DIRECTORY_ID: ExecLogEntry.Directory dir = checkNotNull(entryMap.get(output.getDirectoryId())).getDirectory(); builder.addListedOutputs(dir.getPath()); for (ExecLogEntry.File dirFile : dir.getFilesList()) { - builder.addActualOutputs(reconstructFile(dir, dirFile)); + builder.addActualOutputs(reconstructFile(dir, dirFile, hashFunctionName)); } break; case MISSING_PATH: @@ -199,15 +206,16 @@ private SpawnExec reconstructSpawnExec( } } - if (!entry.getDigest().isEmpty()) { - builder.setDigest(Digest.newBuilder().setHash(entry.getDigest()).build()); + if (entry.hasDigest()) { + builder.setDigest( + entry.getDigest().toBuilder().setHashFunctionName(hashFunctionName).build()); } return builder.build(); } private SortedMap reconstructInputs( - int setId, Map entryMap) { + int setId, String hashFunctionName, Map entryMap) { TreeMap inputs = new TreeMap<>(); ArrayDeque setsToVisit = new ArrayDeque<>(); if (setId != 0) { @@ -218,12 +226,12 @@ private SortedMap reconstructInputs( checkNotNull(entryMap.get(setsToVisit.removeFirst())).getInputSet(); for (int fileId : set.getFileIdsList()) { ExecLogEntry.File file = checkNotNull(entryMap.get(fileId)).getFile(); - inputs.put(file.getPath(), reconstructFile(null, file)); + inputs.put(file.getPath(), reconstructFile(null, file, hashFunctionName)); } for (int dirId : set.getDirectoryIdsList()) { ExecLogEntry.Directory dir = checkNotNull(entryMap.get(dirId)).getDirectory(); for (ExecLogEntry.File dirFile : dir.getFilesList()) { - inputs.put(dirFile.getPath(), reconstructFile(dir, dirFile)); + inputs.put(dirFile.getPath(), reconstructFile(dir, dirFile, hashFunctionName)); } } setsToVisit.addAll(set.getTransitiveSetIdsList()); @@ -231,16 +239,17 @@ private SortedMap reconstructInputs( return inputs; } - private File reconstructFile(@Nullable ExecLogEntry.Directory parentDir, ExecLogEntry.File file) { - String path = parentDir != null ? parentDir.getPath() + "/" + file.getPath() : file.getPath(); - File.Builder builder = File.newBuilder().setPath(path); - if (file.getSizeBytes() > 0) { - builder.setDigest( - Digest.newBuilder() - .setSizeBytes(file.getSizeBytes()) - .setHash(file.getDigest()) - .setHashFunctionName(digestHashFunction.toString())); + private File reconstructFile( + @Nullable ExecLogEntry.Directory parentDir, ExecLogEntry.File file, String hashFunctionName) { + File.Builder builder = File.newBuilder(); + + builder.setPath( + parentDir != null ? parentDir.getPath() + "/" + file.getPath() : file.getPath()); + + if (file.hasDigest()) { + builder.setDigest(file.getDigest().toBuilder().setHashFunctionName(hashFunctionName).build()); } + return builder.build(); } } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java index 237386269801ef..bf414f1cbabb5c 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java @@ -630,7 +630,7 @@ public void testCacheHit() throws Exception { public void testDigest() throws Exception { SpawnLogContext context = createSpawnLogContext(); - Digest digest = Digest.newBuilder().setHash("deadbeef").build(); + Digest digest = getDigest("something"); SpawnResult result = defaultSpawnResultBuilder().setDigest(digest).build();