Make MetadataHandler not implement MetadataProvider anymore.

Before this change, ActionExecutionContext got two references to a MetadataProvider implementation: one directly, one behind MetadataHandler. These could and did get out sync after e.g. calls to withOutputsAsInputs(). This change fixes that. The game plan is to rename these two so that it's obvious that one deals with the inputs of the action, the other, with the outputs, but let's do that very mechanical change after this slightly less mechanical one. RELNOTES: None. PiperOrigin-RevId: 523408132 Change-Id: I557cc5ec939b7a49e33bb857c3b0916d23538048
fweikert · May 25, 2023 · 588f54d · 588f54d
1 parent 2640d3c
commit 588f54d
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 38 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -190,6 +190,7 @@ private static boolean validateArtifacts(
       ActionCache.Entry entry,
       Action action,
       NestedSet<Artifact> actionInputs,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       boolean checkOutput,
       @Nullable CachedOutputMetadata cachedOutputMetadata)
@@ -206,7 +207,7 @@ private static boolean validateArtifacts(
       }
     }
     for (Artifact artifact : actionInputs.toList()) {
-      mdMap.put(artifact.getExecPathString(), getInputMetadataMaybe(metadataHandler, artifact));
+      mdMap.put(artifact.getExecPathString(), getInputMetadataMaybe(metadataProvider, artifact));
     }
     return !Arrays.equals(MetadataDigestUtils.fromMetadata(mdMap), entry.getFileDigest());
   }
@@ -438,6 +439,7 @@ public Token getTokenIfNeedToExecute(
       Map<String, String> clientEnv,
       OutputPermissions outputPermissions,
       EventHandler handler,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       Map<String, String> remoteDefaultPlatformProperties,
@@ -454,7 +456,7 @@ public Token getTokenIfNeedToExecute(
       // Some types of middlemen are not checked because they should not
       // propagate invalidation of their inputs.
       if (middlemanType != MiddlemanType.SCHEDULING_DEPENDENCY_MIDDLEMAN) {
-        checkMiddlemanAction(action, handler, metadataHandler);
+        checkMiddlemanAction(action, handler, metadataProvider, metadataHandler);
       }
       return null;
     }
@@ -495,6 +497,7 @@ public Token getTokenIfNeedToExecute(
         action,
         entry,
         handler,
+        metadataProvider,
         metadataHandler,
         artifactExpander,
         actionInputs,
@@ -525,6 +528,7 @@ private boolean mustExecute(
       Action action,
       @Nullable ActionCache.Entry entry,
       EventHandler handler,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       NestedSet<Artifact> actionInputs,
@@ -551,7 +555,13 @@ private boolean mustExecute(
       actionCache.accountMiss(MissReason.CORRUPTED_CACHE_ENTRY);
       return true;
     } else if (validateArtifacts(
-        entry, action, actionInputs, metadataHandler, true, cachedOutputMetadata)) {
+        entry,
+        action,
+        actionInputs,
+        metadataProvider,
+        metadataHandler,
+        true,
+        cachedOutputMetadata)) {
       reportChanged(handler, action);
       actionCache.accountMiss(MissReason.DIFFERENT_FILES);
       return true;
@@ -574,8 +584,8 @@ private boolean mustExecute(
   }
 
   private static FileArtifactValue getInputMetadataOrConstant(
-      MetadataHandler metadataHandler, Artifact artifact) throws IOException {
-    FileArtifactValue metadata = metadataHandler.getInputMetadata(artifact);
+      MetadataProvider metadataProvider, Artifact artifact) throws IOException {
+    FileArtifactValue metadata = metadataProvider.getInputMetadata(artifact);
     return (metadata != null && artifact.isConstantMetadata())
         ? ConstantMetadataValue.INSTANCE
         : metadata;
@@ -594,9 +604,9 @@ private static FileArtifactValue getOutputMetadataOrConstant(
   // should propagate the exception, because it is unexpected (e.g., bad file system state).
   @Nullable
   private static FileArtifactValue getInputMetadataMaybe(
-      MetadataHandler metadataHandler, Artifact artifact) {
+      MetadataProvider metadataProvider, Artifact artifact) {
     try {
-      return getInputMetadataOrConstant(metadataHandler, artifact);
+      return getInputMetadataOrConstant(metadataProvider, artifact);
     } catch (IOException e) {
       return null;
     }
@@ -618,6 +628,7 @@ private static FileArtifactValue getOutputMetadataMaybe(
   public void updateActionCache(
       Action action,
       Token token,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       Map<String, String> clientEnv,
@@ -673,7 +684,7 @@ public void updateActionCache(
     for (Artifact input : action.getInputs().toList()) {
       entry.addInputFile(
           input.getExecPath(),
-          getInputMetadataMaybe(metadataHandler, input),
+          getInputMetadataMaybe(metadataProvider, input),
           /* saveExecPath= */ !excludePathsFromActionCache.contains(input));
     }
     entry.getFileDigest();
@@ -755,7 +766,10 @@ public List<Artifact> getCachedInputs(Action action, PackageRootResolver resolve
    * actions, it consults with the aggregated middleman digest computed here.
    */
   private void checkMiddlemanAction(
-      Action action, EventHandler handler, MetadataHandler metadataHandler)
+      Action action,
+      EventHandler handler,
+      MetadataProvider metadataProvider,
+      MetadataHandler metadataHandler)
       throws InterruptedException {
     if (!cacheConfig.enabled()) {
       // Action cache is disabled, don't generate digests.
@@ -774,9 +788,10 @@ private void checkMiddlemanAction(
           entry,
           action,
           action.getInputs(),
+          metadataProvider,
           metadataHandler,
           false,
-          /*cachedOutputMetadata=*/ null)) {
+          /* cachedOutputMetadata= */ null)) {
         reportChanged(handler, action);
         actionCache.accountMiss(MissReason.DIFFERENT_FILES);
         changed = true;
@@ -794,7 +809,7 @@ private void checkMiddlemanAction(
       for (Artifact input : action.getInputs().toList()) {
         entry.addInputFile(
             input.getExecPath(),
-            getInputMetadataMaybe(metadataHandler, input),
+            getInputMetadataMaybe(metadataProvider, input),
             /* saveExecPath= */ true);
       }
     }
@@ -816,6 +831,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
       Map<String, String> clientEnv,
       OutputPermissions outputPermissions,
       EventHandler handler,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       Map<String, String> remoteDefaultPlatformProperties,
@@ -830,6 +846,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
         clientEnv,
         outputPermissions,
         handler,
+        metadataProvider,
         metadataHandler,
         artifactExpander,
         remoteDefaultPlatformProperties,

diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
@@ -20,14 +20,12 @@
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileStateType;
-import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import java.io.IOException;
 
-/** Handles metadata of inputs and outputs during the execution phase. */
-public interface MetadataHandler extends MetadataProvider, MetadataInjector {
-
+/** Handles the metadata of the outputs of the action during its execution. */
+public interface MetadataHandler extends MetadataInjector {
   /**
    * Returns a {@link FileArtifactValue} for the given {@link ActionInput}.
    *

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
@@ -758,6 +758,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
               env.getListener(),
               action,
               metadataHandler,
+              metadataHandler,
               artifactExpander,
               actionStartTime,
               state.allInputs.actionCacheInputs,
@@ -787,7 +788,12 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
         try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.INFO, "discoverInputs")) {
           state.discoveredInputs =
               skyframeActionExecutor.discoverInputs(
-                  action, actionLookupData, metadataHandler, env, state.actionFileSystem);
+                  action,
+                  actionLookupData,
+                  metadataHandler,
+                  metadataHandler,
+                  env,
+                  state.actionFileSystem);
         }
         discoveredInputsDuration = Duration.ofNanos(BlazeClock.nanoTime() - actionStartTime);
         if (env.valuesMissing()) {
@@ -877,6 +883,7 @@ public void run(
       skyframeActionExecutor.updateActionCache(
           action,
           metadataHandler,
+          metadataHandler,
           new Artifact.ArtifactExpanderImpl(
               // Skipping the filesets in runfiles since those cannot participate in command line
               // creation.

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.actions.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
+import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.RemoteFileStatus;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
@@ -79,7 +80,7 @@
  * (except those that were {@linkplain #artifactOmitted omitted}) to ensure that declared outputs
  * were in fact created and are valid.
  */
-final class ActionMetadataHandler implements MetadataHandler {
+final class ActionMetadataHandler implements MetadataProvider, MetadataHandler {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   /**

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
@@ -479,6 +479,7 @@ ActionExecutionValue executeAction(
             env,
             action,
             metadataHandler,
+            metadataHandler,
             artifactExpander,
             topLevelFilesets,
             actionFileSystem,
@@ -541,6 +542,7 @@ private ExtendedEventHandler selectEventHandler(boolean emitProgressEvents) {
   private ActionExecutionContext getContext(
       Environment env,
       Action action,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
@@ -553,7 +555,7 @@ private ActionExecutionContext getContext(
     FileOutErr fileOutErr = actionLogBufferPathGenerator.generate(artifactPathResolver);
     return new ActionExecutionContext(
         executorEngine,
-        createFileCache(metadataHandler, actionFileSystem),
+        createFileCache(metadataProvider, actionFileSystem),
         actionInputPrefetcher,
         actionKeyContext,
         metadataHandler,
@@ -596,6 +598,7 @@ private static void closeContext(
   Token checkActionCache(
       ExtendedEventHandler eventHandler,
       Action action,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       long actionStartTime,
@@ -631,6 +634,7 @@ Token checkActionCache(
               clientEnv,
               getOutputPermissions(),
               handler,
+              metadataProvider,
               metadataHandler,
               artifactExpander,
               remoteDefaultProperties,
@@ -673,6 +677,7 @@ public <T extends ActionContext> T getContext(Class<? extends T> type) {
                     clientEnv,
                     getOutputPermissions(),
                     handler,
+                    metadataProvider,
                     metadataHandler,
                     artifactExpander,
                     remoteDefaultProperties,
@@ -703,6 +708,7 @@ public <T extends ActionContext> T getContext(Class<? extends T> type) {
 
   void updateActionCache(
       Action action,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       Token token,
@@ -726,6 +732,7 @@ void updateActionCache(
       actionCacheChecker.updateActionCache(
           action,
           token,
+          metadataProvider,
           metadataHandler,
           artifactExpander,
           clientEnv,
@@ -771,6 +778,7 @@ List<Artifact> getActionCachedInputs(Action action, PackageRootResolver resolver
   NestedSet<Artifact> discoverInputs(
       Action action,
       ActionLookupData actionLookupData,
+      MetadataProvider metadataProvider,
       MetadataHandler metadataHandler,
       Environment env,
       @Nullable FileSystem actionFileSystem)
@@ -783,7 +791,7 @@ NestedSet<Artifact> discoverInputs(
     ActionExecutionContext actionExecutionContext =
         ActionExecutionContext.forInputDiscovery(
             executorEngine,
-            createFileCache(metadataHandler, actionFileSystem),
+            createFileCache(metadataProvider, actionFileSystem),
             actionInputPrefetcher,
             actionKeyContext,
             metadataHandler,