Split FileAndMetadataCache into two classes, since most of the shared functionality is gone.

--
MOS_MIGRATED_REVID=90289916

6 files changed, 163 insertions, 121 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 9861aa1831..cf153439f3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -278,29 +278,28 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       return skyframeActionExecutor.executeAction(action, null, -1, null);
     }
     // This may be recreated if we discover inputs.
-    FileAndMetadataCache fileAndMetadataCache = new FileAndMetadataCache(
-          state.inputArtifactData,
-          state.expandedMiddlemen,
-          skyframeActionExecutor.getExecRoot(),
-          action.getOutputs(),
-        tsgm);
+    ActionMetadataHandler metadataHandler = new ActionMetadataHandler(state.inputArtifactData,
+        action.getOutputs(), tsgm);
     long actionStartTime = System.nanoTime();
     // We only need to check the action cache if we haven't done it on a previous run.
     if (!state.hasCheckedActionCache()) {
-      state.token = skyframeActionExecutor.checkActionCache(action, fileAndMetadataCache,
+      state.token = skyframeActionExecutor.checkActionCache(action, metadataHandler,
           actionStartTime, state.allInputs.actionCacheInputs);
     }
 
     if (state.token == null) {
       // We got a hit from the action cache -- no need to execute.
       return new ActionExecutionValue(
-          fileAndMetadataCache.getOutputData(),
-          fileAndMetadataCache.getAdditionalOutputData());
+          metadataHandler.getOutputData(),
+          metadataHandler.getAdditionalOutputData());
     }
 
     // This may be recreated if we discover inputs.
+    PerActionFileCache perActionFileCache =
+        new PerActionFileCache(state.inputArtifactData, skyframeActionExecutor.getExecRoot());
     ActionExecutionContext actionExecutionContext =
-        skyframeActionExecutor.constructActionExecutionContext(fileAndMetadataCache);
+        skyframeActionExecutor.constructActionExecutionContext(perActionFileCache,
+            metadataHandler, state.expandedMiddlemen);
     boolean inputsDiscoveredDuringActionExecution = false;
     Map<Artifact, FileArtifactValue> metadataFoundDuringActionExecution = null;
     try {
@@ -324,20 +323,18 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
             return null;
           }
           state.inputArtifactData = inputArtifactData;
-          fileAndMetadataCache = new FileAndMetadataCache(
-              state.inputArtifactData,
-              state.expandedMiddlemen,
-              skyframeActionExecutor.getExecRoot(),
-              action.getOutputs(),
-              tsgm
-          );
+          perActionFileCache =
+              new PerActionFileCache(state.inputArtifactData, skyframeActionExecutor.getExecRoot());
+          metadataHandler =
+              new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm);
           actionExecutionContext =
-              skyframeActionExecutor.constructActionExecutionContext(fileAndMetadataCache);
+              skyframeActionExecutor.constructActionExecutionContext(perActionFileCache,
+                  metadataHandler, state.expandedMiddlemen);
         }
       }
       if (!state.hasExecutedAction()) {
         state.value = skyframeActionExecutor.executeAction(action,
-            fileAndMetadataCache, actionStartTime, actionExecutionContext);
+            metadataHandler, actionStartTime, actionExecutionContext);
       }
     } finally {
       try {
@@ -362,15 +359,10 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       inputArtifactData.putAll(state.inputArtifactData);
       inputArtifactData.putAll(metadataFoundDuringActionExecution);
       state.inputArtifactData = inputArtifactData;
-      fileAndMetadataCache = new FileAndMetadataCache(
-          state.inputArtifactData,
-          state.expandedMiddlemen,
-          skyframeActionExecutor.getExecRoot(),
-          action.getOutputs(),
-          tsgm
-      );
-    }
-    skyframeActionExecutor.afterExecution(action, fileAndMetadataCache, state.token);
+      metadataHandler =
+          new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm);
+    }
+    skyframeActionExecutor.afterExecution(action, metadataHandler, state.token);
     return state.value;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index de63c3b2be..714325adcd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -42,7 +42,7 @@ public class ActionExecutionValue implements SkyValue {
    * @param artifactData Map from Artifacts to corresponding FileValues.
    * @param additionalOutputData Map from Artifacts to values if the FileArtifactValue for this
    *     artifact cannot be derived from the corresponding FileValue (see {@link
-   *     FileAndMetadataCache#getAdditionalOutputData} for when this is necessary).
+   *     ActionMetadataHandler#getAdditionalOutputData} for when this is necessary).
    */
   ActionExecutionValue(Map<Artifact, FileValue> artifactData,
       Map<Artifact, FileArtifactValue> additionalOutputData) {
@@ -53,7 +53,7 @@ public class ActionExecutionValue implements SkyValue {
   /**
    * Returns metadata for a given artifact, if that metadata cannot be inferred from the
    * corresponding {@link #getData} call for that Artifact. See {@link
-   * FileAndMetadataCache#getAdditionalOutputData} for when that can happen.
+   * ActionMetadataHandler#getAdditionalOutputData} for when that can happen.
    */
   @Nullable
   FileArtifactValue getArtifactValue(Artifact artifact) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index b9b1284c64..3230af8e32 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -17,12 +17,8 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Interner;
-import com.google.common.collect.Interners;
 import com.google.common.collect.Sets;
-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.Artifact;
 import com.google.devtools.build.lib.actions.cache.Digest;
 import com.google.devtools.build.lib.actions.cache.DigestUtils;
@@ -35,12 +31,9 @@ import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
-import com.google.protobuf.ByteString;
 
-import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Map;
@@ -67,12 +60,9 @@ import javax.annotation.Nullable;
  * versions of the data for a value (see {@link #getAdditionalOutputData} for more.
  */
 @VisibleForTesting
-public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandler {
+public class ActionMetadataHandler implements MetadataHandler {
   /** This should never be read directly. Use {@link #getInputFileArtifactValue} instead. */
   private final Map<Artifact, FileArtifactValue> inputArtifactData;
-  private final Map<Artifact, Collection<Artifact>> expandedInputMiddlemen;
-  private final File execRoot;
-  private final Map<ByteString, Artifact> reverseMap = new ConcurrentHashMap<>();
   private final ConcurrentMap<Artifact, FileValue> outputArtifactData =
       new ConcurrentHashMap<>();
   private final Set<Artifact> omittedOutputs = Sets.newConcurrentHashSet();
@@ -83,16 +73,11 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl
   private final ImmutableSet<Artifact> outputs;
   private final TimestampGranularityMonitor tsgm;
 
-  private static final Interner<ByteString> BYTE_INTERNER = Interners.newWeakInterner();
-
   @VisibleForTesting
-  public FileAndMetadataCache(Map<Artifact, FileArtifactValue> inputArtifactData,
-      Map<Artifact, Collection<Artifact>> expandedInputMiddlemen, File execRoot,
+  public ActionMetadataHandler(Map<Artifact, FileArtifactValue> inputArtifactData,
       Iterable<Artifact> outputs,
       TimestampGranularityMonitor tsgm) {
     this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
-    this.expandedInputMiddlemen = Preconditions.checkNotNull(expandedInputMiddlemen);
-    this.execRoot = Preconditions.checkNotNull(execRoot);
     this.outputs = ImmutableSet.copyOf(outputs);
     this.tsgm = tsgm;
   }
@@ -188,14 +173,6 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl
     return maybeStoreAdditionalData(artifact, fileValue, null);
   }
 
-  /** Expands one of the input middlemen artifacts of the corresponding action. */
-  public Collection<Artifact> expandInputMiddleman(Artifact middlemanArtifact) {
-    Preconditions.checkState(middlemanArtifact.isMiddlemanArtifact(), middlemanArtifact);
-    Collection<Artifact> result = expandedInputMiddlemen.get(middlemanArtifact);
-    // Note that result may be null for non-aggregating middlemen.
-    return result == null ? ImmutableSet.<Artifact>of() : result;
-  }
-
   /**
    * Check that the new {@code data} we just calculated for an {@code artifact} agrees with the
    * {@code oldData} (presumably calculated concurrently), if it was present.
@@ -283,7 +260,7 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl
           // should throw, since all filesystem access has already been done.
           throw new IllegalStateException(
               "Filesystem should not have been accessed while injecting data for "
-          + artifact.prettyPrint(), e);
+                  + artifact.prettyPrint(), e);
         }
         // Ignore exceptions for empty files, as above.
       }
@@ -361,50 +338,9 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl
     return additionalOutputData;
   }
 
-  @Override
-  public long getSizeInBytes(ActionInput input) throws IOException {
-    FileArtifactValue metadata = getInputFileArtifactValue(input);
-    if (metadata != null) {
-      return metadata.getSize();
-    }
-    return -1;
-  }
-
-  @Nullable
-  @Override
-  public File getFileFromDigest(ByteString digest) throws IOException {
-    Artifact artifact = reverseMap.get(digest);
-    if (artifact != null) {
-      String relPath = artifact.getExecPathString();
-      return relPath.startsWith("/") ? new File(relPath) : new File(execRoot, relPath);
-    }
-    return null;
-  }
-
-  @Nullable
-  @Override
-  public ByteString 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 null;
-  }
-
-  @Override
-  public boolean contentsAvailableLocally(ByteString digest) {
-    return reverseMap.containsKey(digest);
-  }
-
   static FileValue fileValueFromArtifact(Artifact artifact,
       @Nullable FileStatusWithDigest statNoFollow, TimestampGranularityMonitor tsgm)
-          throws IOException {
+      throws IOException {
     Path path = artifact.getPath();
     RootedPath rootedPath =
         RootedPath.toRootedPath(artifact.getRoot().getPath(), artifact.getRootRelativePath());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
index d2e962a6af..d1592141b2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -214,7 +214,7 @@ class FilesystemValueChecker {
           SkyKey key = keyAndValue.getFirst();
           FileValue lastKnownData = actionValue.getAllOutputArtifactData().get(artifact);
           try {
-            FileValue newData = FileAndMetadataCache.fileValueFromArtifact(artifact, stat, tsgm);
+            FileValue newData = ActionMetadataHandler.fileValueFromArtifact(artifact, stat, tsgm);
             if (!newData.equals(lastKnownData)) {
               updateIntraBuildModifiedCounter(stat != null ? stat.getLastChangeTime() : -1);
               modifiedOutputFilesCounter.getAndIncrement();
@@ -272,7 +272,7 @@ class FilesystemValueChecker {
       Artifact artifact = entry.getKey();
       FileValue lastKnownData = entry.getValue();
       try {
-        FileValue fileValue = FileAndMetadataCache.fileValueFromArtifact(artifact, null, tsgm);
+        FileValue fileValue = ActionMetadataHandler.fileValueFromArtifact(artifact, null, tsgm);
         if (!fileValue.equals(lastKnownData)) {
           updateIntraBuildModifiedCounter(fileValue.exists()
               ? fileValue.realRootedPath().asPath().getLastModifiedTime()
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
new file mode 100644
index 0000000000..5dde5a5346
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
@@ -0,0 +1,107 @@
+// Copyright 2014 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.common.base.Preconditions;
+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;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.protobuf.ByteString;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import javax.annotation.Nullable;
+
+/**
+ * Cache provided by an {@link ActionExecutionFunction}, allowing Blaze to obtain artifact metadata
+ * from the graph.
+ *
+ * <p>Data for the action's inputs is injected into this cache on construction, using the graph as
+ * the source of truth.
+ */
+class PerActionFileCache implements ActionInputFileCache {
+  private final Map<Artifact, FileArtifactValue> inputArtifactData;
+  private final File execRoot;
+  // Populated lazily, on calls to #getDigest.
+  private final Map<ByteString, Artifact> reverseMap = new ConcurrentHashMap<>();
+
+  private static final Interner<ByteString> BYTE_INTERNER = Interners.newWeakInterner();
+
+  /**
+   * @param inputArtifactData Map from artifact to metadata, used to return metadata upon request.
+   * @param execRoot Path to the execution root, used to convert Artifacts' relative paths into
+   * absolute ones in the execution root.
+   */
+  PerActionFileCache(Map<Artifact, FileArtifactValue> inputArtifactData,
+      File execRoot) {
+    this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
+    this.execRoot = Preconditions.checkNotNull(execRoot);
+  }
+
+  @Nullable
+  private FileArtifactValue getInputFileArtifactValue(ActionInput input) {
+    if (!(input instanceof Artifact)) {
+      return null;
+    }
+    return Preconditions.checkNotNull(inputArtifactData.get(input), input);
+  }
+
+  @Override
+  public long getSizeInBytes(ActionInput input) throws IOException {
+    FileArtifactValue metadata = getInputFileArtifactValue(input);
+    if (metadata != null) {
+      return metadata.getSize();
+    }
+    return -1;
+  }
+
+  @Nullable
+  @Override
+  public File getFileFromDigest(ByteString digest) throws IOException {
+    Artifact artifact = reverseMap.get(digest);
+    if (artifact != null) {
+      String relPath = artifact.getExecPathString();
+      return new File(execRoot, relPath);
+    }
+    return null;
+  }
+
+  @Nullable
+  @Override
+  public ByteString 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 null;
+  }
+
+  @Override
+  public boolean contentsAvailableLocally(ByteString digest) {
+    return reverseMap.containsKey(digest);
+  }
+}
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 4dc14788e7..e76990c04a 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
@@ -367,7 +367,7 @@ public final class SkyframeActionExecutor {
    *
    * <p>For use from {@link ArtifactFunction} only.
    */
-  ActionExecutionValue executeAction(Action action, FileAndMetadataCache graphFileCache,
+  ActionExecutionValue executeAction(Action action, ActionMetadataHandler metadataHandler,
       long actionStartTime,
       ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
@@ -378,7 +378,7 @@ public final class SkyframeActionExecutor {
     }
     Artifact primaryOutput = action.getPrimaryOutput();
     FutureTask<ActionExecutionValue> actionTask =
-        new FutureTask<>(new ActionRunner(action, graphFileCache,
+        new FutureTask<>(new ActionRunner(action, metadataHandler,
             actionStartTime, actionExecutionContext));
     // Check to see if another action is already executing/has executed this value.
     Pair<Action, FutureTask<ActionExecutionValue>> oldAction =
@@ -418,33 +418,40 @@ public final class SkyframeActionExecutor {
     }
   }
 
+  private static class MiddlemanExpanderImpl implements MiddlemanExpander {
+    private final Map<Artifact, Collection<Artifact>> expandedInputMiddlemen;
+
+    private MiddlemanExpanderImpl(Map<Artifact, Collection<Artifact>> expandedInputMiddlemen) {
+      this.expandedInputMiddlemen = expandedInputMiddlemen;
+    }
+
+    @Override
+    public void expand(Artifact middlemanArtifact, Collection<? super Artifact> output) {
+      Preconditions.checkState(middlemanArtifact.isMiddlemanArtifact(), middlemanArtifact);
+      Collection<Artifact> result = expandedInputMiddlemen.get(middlemanArtifact);
+      // Note that result may be null for non-aggregating middlemen.
+      if (result != null) {
+        output.addAll(result);
+      }
+    }
+  }
+
   /**
    * Returns an ActionExecutionContext suitable for executing a particular action. The caller should
    * pass the returned context to {@link #executeAction}, and any other method that needs to execute
    * tasks related to that action.
    */
   ActionExecutionContext constructActionExecutionContext(
-      final FileAndMetadataCache graphFileCache) {
+      PerActionFileCache graphFileCache, MetadataHandler metadataHandler,
+      Map<Artifact, Collection<Artifact>> expandedInputMiddlemen) {
     // TODO(bazel-team): this should be closed explicitly somewhere.
     FileOutErr fileOutErr = actionLogBufferPathGenerator.generate();
     return new ActionExecutionContext(
         executorEngine,
         new DelegatingPairFileCache(graphFileCache, perBuildFileCache),
-        graphFileCache,
+        metadataHandler,
         fileOutErr,
-        new MiddlemanExpander() {
-          @Override
-          public void expand(Artifact middlemanArtifact,
-              Collection<? super Artifact> output) {
-            // Legacy code is more permissive regarding "mm" in that it expands any middleman,
-            // not just inputs of this action. Skyframe doesn't have access to a global action
-            // graph, therefore this implementation can't expand any middleman, only the
-            // inputs of this action.
-            // This is fine though: actions should only hold references to their input
-            // artifacts, otherwise hermeticity would be violated.
-            output.addAll(graphFileCache.expandInputMiddleman(middlemanArtifact));
-          }
-        });
+        new MiddlemanExpanderImpl(expandedInputMiddlemen));
   }
 
   /**
@@ -542,15 +549,15 @@ public final class SkyframeActionExecutor {
 
   private class ActionRunner implements Callable<ActionExecutionValue> {
     private final Action action;
-    private final FileAndMetadataCache graphFileCache;
+    private final ActionMetadataHandler metadataHandler;
     private long actionStartTime;
     private ActionExecutionContext actionExecutionContext;
 
-    ActionRunner(Action action, FileAndMetadataCache graphFileCache,
+    ActionRunner(Action action, ActionMetadataHandler metadataHandler,
         long actionStartTime,
         ActionExecutionContext actionExecutionContext) {
       this.action = action;
-      this.graphFileCache = graphFileCache;
+      this.metadataHandler = metadataHandler;
       this.actionStartTime = actionStartTime;
       this.actionExecutionContext = actionExecutionContext;
     }
@@ -580,7 +587,7 @@ public final class SkyframeActionExecutor {
 
         prepareScheduleExecuteAndCompleteAction(action, actionExecutionContext, actionStartTime);
         return new ActionExecutionValue(
-            graphFileCache.getOutputData(), graphFileCache.getAdditionalOutputData());
+            metadataHandler.getOutputData(), metadataHandler.getAdditionalOutputData());
       } finally {
         profiler.completeTask(ProfilerTask.ACTION);
       }