Derive worker files hash from a self-describing layout

`WorkerFilesHash#getCombinedHash()` hashed bytes and strings without also hashing their length. It also reencoded path strings from the internally used UTF-8-in-Latin-1 format to UTF-8 on macOS, which results in key mismatches for the worker across platforms and is unnecessarily slow. Since this commit already changes the key, also use a slightly more efficient `Fingerprint` method to hash the args.
bazelbuild · Sep 21, 2024 · 7f84352 · 7f84352
1 parent b084956
commit 7f84352
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 7 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -690,8 +690,9 @@ private ToolSignature computePersistentWorkerSignature(Spawn spawn, SpawnExecuti
  execRoot, Options.getDefaults(WorkerOptions.class), LocalEnvProvider.NOOP, null);
  WorkerKey workerKey = workerParser.compute(spawn, context).getWorkerKey();
  Fingerprint fingerprint = new Fingerprint();
+ // getWorkerFilesCombinedHash always uses SHA-256, so the hash is always 32 bytes.
  fingerprint.addBytes(workerKey.getWorkerFilesCombinedHash().asBytes());
- fingerprint.addIterableStrings(workerKey.getArgs());
+ fingerprint.addStrings(workerKey.getArgs());
  fingerprint.addStringMap(workerKey.getEnv());
  return new ToolSignature(
  fingerprint.hexDigestAndReset(), workerKey.getWorkerFilesWithDigests().keySet());

diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD
@@ -36,6 +36,7 @@ java_library(
  "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
  "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
  "//src/main/java/com/google/devtools/build/lib/actions:runfiles_tree",
+ "//src/main/java/com/google/devtools/build/lib/unsafe:string",
  "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
  "//third_party:guava",
  "//third_party:jsr305",

diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
@@ -26,9 +26,9 @@
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.RunfilesTree;
 import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.unsafe.StringUnsafe;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
-import java.nio.charset.Charset;
 import java.util.List;
 import java.util.Map;
 import java.util.SortedMap;
@@ -47,7 +47,12 @@ public static HashCode getCombinedHash(SortedMap<PathFragment, byte[]> workerFil
  Hasher hasher = Hashing.sha256().newHasher();
  workerFilesMap.forEach(
  (execPath, digest) -> {
- hasher.putString(execPath.getPathString(), Charset.defaultCharset());
+ String execPathString = execPath.getPathString();
+ hasher.putByte(StringUnsafe.getInstance().getCoder(execPathString));
+ hasher.putInt(execPathString.length());
+ hasher.putBytes(StringUnsafe.getInstance().getByteArray(execPathString));
+
+ hasher.putInt(digest.length);
  hasher.putBytes(digest);
  });
  return hasher.hash();
@@ -71,7 +76,7 @@ public static SortedMap<PathFragment, byte[]> getWorkerFilesWithDigests(
  /* keepEmptyTreeArtifacts= */ false,
  /* keepMiddlemanArtifacts= */ true);
  for (ActionInput tool : tools) {
- if ((tool instanceof Artifact) && ((Artifact) tool).isMiddlemanArtifact()) {
+ if (tool instanceof Artifact artifact && artifact.isMiddlemanArtifact()) {
  RunfilesTree runfilesTree =
  actionInputFileCache.getRunfilesMetadata(tool).getRunfilesTree();
  PathFragment root = runfilesTree.getExecPath();

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -2350,7 +2350,7 @@ public void buildRemoteActionForRemotePersistentWorkers() throws Exception {
  Platform.Property.newBuilder()
  .setName("persistentWorkerKey")
  .setValue(
- "b22d48cd55755474eae27e63a79306a64146bd5947d5bd3423d78f001cf7b3de"))
+ "628637504c26bb74fb6bb3f60fb7132b3aa574b866db4181770774882a8853e5"))
  .build());
  var merkleTree = remoteAction.getMerkleTree();
  var outputDirectory =
@@ -2386,7 +2386,7 @@ public void buildRemoteActionForRemotePersistentWorkers() throws Exception {
  Platform.Property.newBuilder()
  .setName("persistentWorkerKey")
  .setValue(
- "b22d48cd55755474eae27e63a79306a64146bd5947d5bd3423d78f001cf7b3de"))
+ "628637504c26bb74fb6bb3f60fb7132b3aa574b866db4181770774882a8853e5"))
  .build());
 
  // Check that if a tool input changes, the persistent worker key changes.
@@ -2398,7 +2398,7 @@ public void buildRemoteActionForRemotePersistentWorkers() throws Exception {
  Platform.Property.newBuilder()
  .setName("persistentWorkerKey")
  .setValue(
- "997337de8dc20123cd7c8fcaed2c9c79cd8138831f9fbbf119f37d0859c9e83a"))
+ "98e07ff5afc8f4d127e93d326c87c132f89cfd009517422671e6abec2fe05e2b"))
  .build());
  }