Use byte[] rather than ByteString for file digests.

ActionInputFileCache:
Change getDigest() to return the underlying byte[16] owned by each
FileArtifactValue.
Remove throws clause from getInputFromDigest(); this should be an
in-memory operation, and no implementation actually throws.

PerActionFileCache:
Invert mapping from artifact to digest only if needed.  Remove interner,
as it was used only for the reverse map keys, not the returned values.
This should be a significant cpu savings as eagerly constructing the
reverse maps was a noticeable hotspot.

--
MOS_MIGRATED_REVID=127972359

8 files changed, 68 insertions, 48 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java
index f37cc09384..decfc8d7bf 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java
@@ -34,6 +34,8 @@ public interface ActionInputFileCache {
    * then t >= p. Aside from these properties, t can be any value and may vary arbitrarily across
    * calls.
    *
+   * The return value is owned by the cache and must not be modified.
+   *
    * @param input the input to retrieve the digest for
    * @return the artifact's digest or null if digest cannot be obtained (due to artifact
    *         non-existence, lookup errors, or any other reason)
@@ -43,7 +45,7 @@ public interface ActionInputFileCache {
    *
    */
   @Nullable
-  ByteString getDigest(ActionInput input) throws IOException;
+  byte[] getDigest(ActionInput input) throws IOException;
 
   /**
    * Retrieves whether or not the input Artifact is a file or symlink to an existing file.
@@ -68,7 +70,7 @@ public interface ActionInputFileCache {
    * Checks if the file is available locally, based on the assumption that previous operations on
    * the ActionInputFileCache would have created a cache entry for it.
    *
-   * @param digest the digest to lookup.
+   * @param digest the digest to lookup (as lowercase hex).
    * @return true if the specified digest is backed by a locally-readable file, false otherwise
    */
   boolean contentsAvailableLocally(ByteString digest);
@@ -77,11 +79,11 @@ public interface ActionInputFileCache {
    * Concrete subclasses must implement this to provide a mapping from digest to file path,
    * based on files previously seen as inputs.
    *
-   * @param digest the digest.
+   * @param digest the digest (as lowercase hex).
    * @return an ActionInput corresponding to the given digest.
    */
   @Nullable
-  ActionInput getInputFromDigest(ByteString digest) throws IOException;
+  ActionInput getInputFromDigest(ByteString digest);
 
   /**
    * The absolute path that this input is located at. The usual {@link ActionInput} implementation
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java
index 03725d6054..48e6f95e17 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java
@@ -70,13 +70,12 @@ public class SingleBuildFileCache implements ActionInputFileCache {
           Path path = null;
           try {
             path = fs.getPath(fullPath(input));
+            byte[] digest = path.getMD5Digest();
             BaseEncoding hex = BaseEncoding.base16().lowerCase();
-            ByteString digest = ByteString.copyFrom(
-                hex.encode(path.getMD5Digest())
-                   .getBytes(US_ASCII));
+            ByteString hexDigest = ByteString.copyFrom(hex.encode(digest).getBytes(US_ASCII));
             // Inject reverse mapping. Doing this unconditionally in getDigest() showed up
             // as a hotspot in CPU profiling.
-            digestToPath.put(digest, input);
+            digestToPath.put(hexDigest, input);
             return new ActionInputMetadata(digest, path.getFileSize());
           } catch (IOException e) {
             if (path != null && path.isDirectory()) {
@@ -110,7 +109,7 @@ public class SingleBuildFileCache implements ActionInputFileCache {
   }
 
   @Override
-  public ByteString getDigest(ActionInput input) throws IOException {
+  public byte[] getDigest(ActionInput input) throws IOException {
     return pathToMetadata.getUnchecked(input).getDigest();
   }
 
@@ -137,12 +136,12 @@ public class SingleBuildFileCache implements ActionInputFileCache {
 
   /** Container class for caching I/O around ActionInputs. */
   private static class ActionInputMetadata {
-    private final ByteString digest;
+    private final byte[] digest;
     private final long size;
     private final IOException exceptionOnAccess;
 
     /** Constructor for a successful lookup. */
-    ActionInputMetadata(ByteString digest, long size) {
+    ActionInputMetadata(byte[] digest, long size) {
       this.digest = digest;
       this.size = size;
       this.exceptionOnAccess = null;
@@ -156,7 +155,7 @@ public class SingleBuildFileCache implements ActionInputFileCache {
     }
 
     /** Returns digest or throws the exception encountered calculating it/ */
-    ByteString getDigest() throws IOException {
+    byte[] getDigest() throws IOException {
       maybeRaiseException();
       return digest;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java
index e821f1289f..8d29bea68a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java
@@ -66,8 +66,7 @@ public final class MemcacheActionCache implements RemoteActionCache {
 
   @Override
   public String putFileIfNotExist(ActionInputFileCache cache, ActionInput file) throws IOException {
-    // PerActionFileCache already converted this to a lowercase ascii string.. it's not consistent!
-    String contentKey = new String(cache.getDigest(file).toByteArray());
+    String contentKey = HashCode.fromBytes(cache.getDigest(file)).toString();
     if (containsFile(contentKey)) {
       return contentKey;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
index 7184f79dfd..17b821af79 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
@@ -118,7 +118,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
         // changes. It might not be sufficient to identify the input file globally in the
         // remote action cache. Consider upgrading this to a better hash algorithm with
         // less collision.
-        hasher.putBytes(inputFileCache.getDigest(input).toByteArray());
+        hasher.putBytes(inputFileCache.getDigest(input));
       } catch (IOException e) {
         throw new UserExecException("Failed to get digest for input.", e);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
index 270090478d..c5576d9dfb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
@@ -13,8 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.Interner;
-import com.google.common.collect.Interners;
 import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
@@ -25,8 +23,8 @@ import com.google.protobuf.ByteString;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 import javax.annotation.Nullable;
 
@@ -39,10 +37,8 @@ import javax.annotation.Nullable;
  */
 class PerActionFileCache implements ActionInputFileCache {
   private final Map<Artifact, FileArtifactValue> inputArtifactData;
-  // Populated lazily, on calls to #getDigest.
-  private final Map<ByteString, Artifact> reverseMap = new ConcurrentHashMap<>();
-
-  private static final Interner<ByteString> BYTE_INTERNER = Interners.newWeakInterner();
+  // null until first call to getInputFromDigest()
+  private volatile HashMap<ByteString, Artifact> reverseMap;
 
   /**
    * @param inputArtifactData Map from artifact to metadata, used to return metadata upon request.
@@ -74,12 +70,6 @@ class PerActionFileCache implements ActionInputFileCache {
     return -1;
   }
 
-  @Nullable
-  @Override
-  public Artifact getInputFromDigest(ByteString digest) throws IOException {
-    return reverseMap.get(digest);
-  }
-
   @Override
   public Path getInputPath(ActionInput input) {
     return ((Artifact) input).getPath();
@@ -87,16 +77,10 @@ class PerActionFileCache implements ActionInputFileCache {
 
   @Nullable
   @Override
-  public ByteString getDigest(ActionInput input) throws IOException {
+  public byte[] getDigest(ActionInput input) throws IOException {
     FileArtifactValue value = getInputFileArtifactValue(input);
     if (value != null) {
-      byte[] bytes = value.getDigest();
-      if (bytes != null) {
-        ByteString digest = ByteString.copyFrom(BaseEncoding.base16().lowerCase().encode(bytes)
-            .getBytes(StandardCharsets.US_ASCII));
-        reverseMap.put(BYTE_INTERNER.intern(digest), (Artifact) input);
-        return digest;
-      }
+      return value.getDigest();
     }
     return null;
   }
@@ -109,6 +93,38 @@ class PerActionFileCache implements ActionInputFileCache {
 
   @Override
   public boolean contentsAvailableLocally(ByteString digest) {
-    return reverseMap.containsKey(digest);
+    return getInputFromDigest(digest) != null;
+  }
+
+  @Nullable
+  @Override
+  public Artifact getInputFromDigest(ByteString digest) {
+    HashMap<ByteString, Artifact> r = reverseMap;  // volatile load
+    if (r == null) {
+      r = buildReverseMap();
+    }
+    return r.get(digest);
+  }
+
+  private synchronized HashMap<ByteString, Artifact> buildReverseMap() {
+    HashMap<ByteString, Artifact> r = reverseMap;  // volatile load
+    if (r != null) {
+      return r;
+    }
+    r = new HashMap<>(inputArtifactData.size());
+    // It would be nice to have a view of the entries which treats them as a map keyed on digest but
+    // does not require constructing another wrapper object for each entry.  Java doesn't come with
+    // any collections which can do this, but cloning RegularImmutableSet and adding the necessary
+    // features wouldn't be too bad.
+    for (Map.Entry<Artifact, FileArtifactValue> e : inputArtifactData.entrySet()) {
+      byte[] bytes = e.getValue().getDigest();
+      if (bytes != null) {
+        ByteString digest = ByteString.copyFrom(BaseEncoding.base16().lowerCase().encode(bytes)
+            .getBytes(StandardCharsets.US_ASCII));
+        r.put(digest, e.getKey());
+      }
+    }
+    reverseMap = r;  // volatile store
+    return r;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index a9cf5833f1..b5c0863d98 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -1105,8 +1105,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     }
 
     @Override
-    public ByteString getDigest(ActionInput actionInput) throws IOException {
-      ByteString digest = perActionCache.getDigest(actionInput);
+    public byte[] getDigest(ActionInput actionInput) throws IOException {
+      byte[] digest = perActionCache.getDigest(actionInput);
       return digest != null ? digest : perBuildFileCache.getDigest(actionInput);
     }
 
@@ -1130,7 +1130,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
 
     @Nullable
     @Override
-    public ActionInput getInputFromDigest(ByteString digest) throws IOException {
+    public ActionInput getInputFromDigest(ByteString digest) {
       ActionInput file = perActionCache.getInputFromDigest(digest);
       return file != null ? file : perBuildFileCache.getInputFromDigest(digest);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
index b84e96991e..53b68f4744 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
@@ -152,9 +152,12 @@ public final class WorkerSpawnStrategy implements SpawnActionContext {
               spawn.getInputFiles(), actionExecutionContext.getArtifactExpander());
 
       for (ActionInput input : inputs) {
-        ByteString digest = inputFileCache.getDigest(input);
-        if (digest == null) {
+        byte[] digestBytes = inputFileCache.getDigest(input);
+        ByteString digest;
+        if (digestBytes == null) {
           digest = ByteString.EMPTY;
+        } else {
+          digest = ByteString.copyFromUtf8(HashCode.fromBytes(digestBytes).toString());
         }
 
         requestBuilder
@@ -208,7 +211,7 @@ public final class WorkerSpawnStrategy implements SpawnActionContext {
     Hasher hasher = Hashing.sha256().newHasher();
     for (ActionInput tool : toolFiles) {
       hasher.putString(tool.getExecPathString(), Charset.defaultCharset());
-      hasher.putBytes(actionInputFileCache.getDigest(tool).toByteArray());
+      hasher.putBytes(actionInputFileCache.getDigest(tool));
     }
     return hasher.hash();
   }
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java b/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java
index ee725adbca..579fe0c923 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java
@@ -111,7 +111,9 @@ public class SingleBuildFileCacheTest {
   public void testBasic() throws Exception {
     ActionInput empty = ActionInputHelper.fromPath("/empty");
     assertEquals(0, underTest.getSizeInBytes(empty));
-    ByteString digest = underTest.getDigest(empty);
+    byte[] digestBytes = underTest.getDigest(empty);
+    ByteString digest = ByteString.copyFromUtf8(
+        BaseEncoding.base16().lowerCase().encode(digestBytes));
 
     assertEquals(EMPTY_MD5, digest.toStringUtf8());
     assertEquals("/empty", underTest.getInputFromDigest(digest).getExecPathString());
@@ -126,15 +128,14 @@ public class SingleBuildFileCacheTest {
   public void testUnreadableFileWhenFileSystemSupportsDigest() throws Exception {
     byte[] expectedDigestRaw = MessageDigest.getInstance("md5").digest(
         "randomtext".getBytes(StandardCharsets.UTF_8));
-    ByteString expectedDigestEncoded = ByteString.copyFromUtf8(
-        BaseEncoding.base16().lowerCase().encode(expectedDigestRaw));
+    ByteString expectedDigest = ByteString.copyFrom(expectedDigestRaw);
     md5Overrides.put("/unreadable", expectedDigestRaw);
 
     ActionInput input = ActionInputHelper.fromPath("/unreadable");
     Path file = fs.getPath("/unreadable");
     file.getOutputStream().close();
     file.chmod(0);
-    ByteString actualDigest = underTest.getDigest(input);
-    assertThat(expectedDigestEncoded).isEqualTo(actualDigest);
+    ByteString actualDigest = ByteString.copyFrom(underTest.getDigest(input));
+    assertThat(expectedDigest).isEqualTo(actualDigest);
   }
 }