Combine pathTo{Digest,Bytes} in SingleBuildFileCache

Instead of updating the file size cache as a side effect of updating
the digest cache we combine them into one cache to make it harder for
them to fall out of sync (mainly by way of programmer error). This
should also have a smaller memory footprint.

Also fixes a bug around cached exceptions.

--
MOS_MIGRATED_REVID=92928216

1 files changed, 50 insertions, 28 deletions

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 64df5c166b..cbe30564be 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
@@ -24,7 +24,6 @@ import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.DigestOfDirectoryException;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.protobuf.ByteString;
@@ -56,7 +55,7 @@ public class SingleBuildFileCache implements ActionInputFileCache {
   // If we can't get the digest, we store the exception. This avoids extra file IO for files
   // that are allowed to be missing, as we first check a likely non-existent content file
   // first.  Further we won't need to unwrap the exception in getDigest().
-  private final LoadingCache<ActionInput, Pair<ByteString, IOException>> pathToDigest =
+  private final LoadingCache<ActionInput, ActionInputMetadata> pathToMetadata =
       CacheBuilder.newBuilder()
       // We default to 10 disk read threads, but we don't expect them all to edit the map
       // simultaneously.
@@ -64,9 +63,9 @@ public class SingleBuildFileCache implements ActionInputFileCache {
       // Even small-ish builds, as of 11/21/2011 typically have over 10k artifacts, so it's
       // unlikely that this default will adversely affect memory in most cases.
       .initialCapacity(10000)
-      .build(new CacheLoader<ActionInput, Pair<ByteString, IOException>>() {
+      .build(new CacheLoader<ActionInput, ActionInputMetadata>() {
         @Override
-        public Pair<ByteString, IOException> load(ActionInput input) {
+        public ActionInputMetadata load(ActionInput input) {
           Path path = null;
           try {
             path = fs.getPath(fullPath(input));
@@ -74,29 +73,25 @@ public class SingleBuildFileCache implements ActionInputFileCache {
             ByteString digest = ByteString.copyFrom(
                 hex.encode(path.getMD5Digest())
                    .getBytes(US_ASCII));
-            pathToBytes.put(input, path.getFileSize());
             // Inject reverse mapping. Doing this unconditionally in getDigest() showed up
             // as a hotspot in CPU profiling.
             digestToPath.put(digest, input);
-            return Pair.of(digest, null);
+            return new ActionInputMetadata(digest, path.getFileSize());
           } catch (IOException e) {
             if (path != null && path.isDirectory()) {
-              pathToBytes.put(input, 0L);
-              return Pair.<ByteString, IOException>of(null, new DigestOfDirectoryException(
+              // TODO(bazel-team): This is rather presumptuous- it could have been another type of
+              // IOException.
+              return new ActionInputMetadata(new DigestOfDirectoryException(
                   "Input is a directory: " + input.getExecPathString()));
+            } else {
+              return new ActionInputMetadata(e);
             }
-
-            // Put value into size map to avoid trying to read file again later.
-            pathToBytes.put(input, 0L);
-            return Pair.of(null, e);
           }
         }
       });
 
   private final Map<ByteString, ActionInput> digestToPath = Maps.newConcurrentMap();
 
-  private final Map<ActionInput, Long> pathToBytes = Maps.newConcurrentMap();
-
   @Nullable
   @Override
   public ActionInput getInputFromDigest(ByteString digest) {
@@ -110,24 +105,12 @@ public class SingleBuildFileCache implements ActionInputFileCache {
 
   @Override
   public long getSizeInBytes(ActionInput input) throws IOException {
-    // TODO(bazel-team): this only works if pathToDigest has already been called.
-    Long sz = pathToBytes.get(input);
-    if (sz != null) {
-      return sz;
-    }
-    Path path = fs.getPath(fullPath(input));
-    sz = path.getFileSize();
-    pathToBytes.put(input, sz);
-    return sz;
+    return pathToMetadata.getUnchecked(input).getSize();
   }
 
   @Override
   public ByteString getDigest(ActionInput input) throws IOException {
-    Pair<ByteString, IOException> result = pathToDigest.getUnchecked(input);
-    if (result.second != null) {
-      throw result.second;
-    }
-    return result.first;
+    return pathToMetadata.getUnchecked(input).getDigest();
   }
 
   @Override
@@ -144,4 +127,43 @@ public class SingleBuildFileCache implements ActionInputFileCache {
     String relPath = input.getExecPathString();
     return relPath.startsWith("/") ? relPath : new File(cwd, relPath).getPath();
   }
+
+  /** Container class for caching I/O around ActionInputs. */
+  private static class ActionInputMetadata {
+    private final ByteString digest;
+    private final long size;
+    private final IOException exceptionOnAccess;
+
+    /** Constructor for a successful lookup. */
+    ActionInputMetadata(ByteString digest, long size) {
+      this.digest = digest;
+      this.size = size;
+      this.exceptionOnAccess = null;
+    }
+
+    /** Constructor for a failed lookup, size will be 0. */
+    ActionInputMetadata(IOException exceptionOnAccess) {
+      this.exceptionOnAccess = exceptionOnAccess;
+      this.digest = null;
+      this.size = 0;
+    }
+
+    /** Returns digest or throws the exception encountered calculating it/ */
+    ByteString getDigest() throws IOException {
+      maybeRaiseException();
+      return digest;
+    }
+
+    /** Returns the size. */
+    long getSize() throws IOException {
+      maybeRaiseException();
+      return size;
+    }
+
+    private void maybeRaiseException() throws IOException {
+      if (exceptionOnAccess != null) {
+        throw exceptionOnAccess;
+      }
+    }
+  }
 }