Cover checking outputs in the profile.

Also extend the profile span of checking action cache hits to include all of the work and not only the actual cache lookup. PiperOrigin-RevId: 496958414 Change-Id: Ie3b6193827810c8b8b9f36da994e12e2b1d30c14
bazelbuild · Dec 21, 2022 · df3d00a · df3d00a
1 parent 4e8354c
commit df3d00a
Showing 1 changed file with 84 additions and 79 deletions.
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
@@ -609,62 +609,63 @@ Token checkActionCache(
               artifactExpander,
               remoteDefaultProperties,
               loadCachedOutputMetadata);
-    } catch (UserExecException e) {
-      throw ActionExecutionException.fromExecException(e, action);
-    }
-    if (token == null) {
-      boolean eventPosted = false;
-      // Notify BlazeRuntimeStatistics about the action middleman 'execution'.
-      if (action.getActionType().isMiddleman()) {
-        eventHandler.post(new ActionMiddlemanEvent(action, actionStartTime));
-        eventPosted = true;
-      }
 
-      if (action instanceof NotifyOnActionCacheHit) {
-        NotifyOnActionCacheHit notify = (NotifyOnActionCacheHit) action;
-        ExtendedEventHandler contextEventHandler = selectEventHandler(action);
-        ActionCachedContext context =
-            new ActionCachedContext() {
-              @Override
-              public ExtendedEventHandler getEventHandler() {
-                return contextEventHandler;
-              }
-
-              @Override
-              public Path getExecRoot() {
-                return executorEngine.getExecRoot();
-              }
-
-              @Override
-              public <T extends ActionContext> T getContext(Class<? extends T> type) {
-                return executorEngine.getContext(type);
-              }
-            };
-        boolean recordActionCacheHit = notify.actionCacheHit(context);
-        if (!recordActionCacheHit) {
-          token =
-              actionCacheChecker.getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
-                  action,
-                  resolvedCacheArtifacts,
-                  clientEnv,
-                  handler,
-                  metadataHandler,
-                  artifactExpander,
-                  remoteDefaultProperties,
-                  loadCachedOutputMetadata);
+      if (token == null) {
+        boolean eventPosted = false;
+        // Notify BlazeRuntimeStatistics about the action middleman 'execution'.
+        if (action.getActionType().isMiddleman()) {
+          eventHandler.post(new ActionMiddlemanEvent(action, actionStartTime));
+          eventPosted = true;
         }
-      }
 
-      // We still need to check the outputs so that output file data is available to the value.
-      // Filesets cannot be cached in the action cache, so it is fine to pass null here.
-      checkOutputs(
-          action,
-          metadataHandler,
-          /*filesetOutputSymlinksForMetrics=*/ null,
-          /*isActionCacheHitForMetrics=*/ true);
-      if (!eventPosted) {
-        eventHandler.post(new CachedActionEvent(action, actionStartTime));
+        if (action instanceof NotifyOnActionCacheHit) {
+          NotifyOnActionCacheHit notify = (NotifyOnActionCacheHit) action;
+          ExtendedEventHandler contextEventHandler = selectEventHandler(action);
+          ActionCachedContext context =
+              new ActionCachedContext() {
+                @Override
+                public ExtendedEventHandler getEventHandler() {
+                  return contextEventHandler;
+                }
+
+                @Override
+                public Path getExecRoot() {
+                  return executorEngine.getExecRoot();
+                }
+
+                @Override
+                public <T extends ActionContext> T getContext(Class<? extends T> type) {
+                  return executorEngine.getContext(type);
+                }
+              };
+          boolean recordActionCacheHit = notify.actionCacheHit(context);
+          if (!recordActionCacheHit) {
+            token =
+                actionCacheChecker.getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
+                    action,
+                    resolvedCacheArtifacts,
+                    clientEnv,
+                    handler,
+                    metadataHandler,
+                    artifactExpander,
+                    remoteDefaultProperties,
+                    loadCachedOutputMetadata);
+          }
+        }
+
+        // We still need to check the outputs so that output file data is available to the value.
+        // Filesets cannot be cached in the action cache, so it is fine to pass null here.
+        checkOutputs(
+            action,
+            metadataHandler,
+            /* filesetOutputSymlinksForMetrics= */ null,
+            /* isActionCacheHitForMetrics= */ true);
+        if (!eventPosted) {
+          eventHandler.post(new CachedActionEvent(action, actionStartTime));
+        }
       }
+    } catch (UserExecException e) {
+      throw ActionExecutionException.fromExecException(e, action);
     }
     return token;
   }
@@ -1437,33 +1438,37 @@ private boolean checkOutputs(
       @Nullable ImmutableList<FilesetOutputSymlink> filesetOutputSymlinksForMetrics,
       boolean isActionCacheHitForMetrics) {
     boolean success = true;
-    for (Artifact output : action.getOutputs()) {
-      // getMetadata has the side effect of adding the artifact to the cache if it's not there
-      // already (e.g., due to a previous call to MetadataHandler.injectDigest), therefore we only
-      // call it if we know the artifact is not omitted.
-      if (!metadataHandler.artifactOmitted(output)) {
-        try {
-          FileArtifactValue metadata = metadataHandler.getMetadata(output);
-
-          addOutputToMetrics(
-              output,
-              metadata,
-              metadataHandler,
-              filesetOutputSymlinksForMetrics,
-              isActionCacheHitForMetrics,
-              action);
-        } catch (IOException e) {
-          success = false;
-          if (output.isTreeArtifact()) {
-            reportOutputTreeArtifactErrors(action, output, reporter, e);
-          } else if (output.isSymlink() && e instanceof NotASymlinkException) {
-            reporter.handle(
-                Event.error(
-                    action.getOwner().getLocation(),
-                    String.format("declared output '%s' is not a symlink", output.prettyPrint())));
-          } else {
-            // Are all other exceptions caught due to missing files?
-            reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink(), e);
+    try (SilentCloseable c = profiler.profile(ProfilerTask.INFO, "checkOutputs")) {
+      for (Artifact output : action.getOutputs()) {
+        // getMetadata has the side effect of adding the artifact to the cache if it's not there
+        // already (e.g., due to a previous call to MetadataHandler.injectDigest), therefore we only
+        // call it if we know the artifact is not omitted.
+        if (!metadataHandler.artifactOmitted(output)) {
+          try {
+            FileArtifactValue metadata = metadataHandler.getMetadata(output);
+
+            addOutputToMetrics(
+                output,
+                metadata,
+                metadataHandler,
+                filesetOutputSymlinksForMetrics,
+                isActionCacheHitForMetrics,
+                action);
+          } catch (IOException e) {
+            success = false;
+            if (output.isTreeArtifact()) {
+              reportOutputTreeArtifactErrors(action, output, reporter, e);
+            } else if (output.isSymlink() && e instanceof NotASymlinkException) {
+              reporter.handle(
+                  Event.error(
+                      action.getOwner().getLocation(),
+                      String.format(
+                          "declared output '%s' is not a symlink", output.prettyPrint())));
+            } else {
+              // Are all other exceptions caught due to missing files?
+              reportMissingOutputFile(
+                  action, output, reporter, output.getPath().isSymbolicLink(), e);
+            }
           }
         }
       }