commit 1038298d7a175161e932bfbd7eb35d268aa4a295
author janakr <janakr@google.com> Tue Sep 29 10:26:33 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Tue Sep 29 10:28:06 2020 -0700
tree 57aacf623e9d88ff1350b150b5f332971bf5ec47
parent 651596e2bc4fdb95e5f8279ea168df89a8f41efc

Avoid replaying events/postables of top-level nodes in the common case that there are no errors (or the build was keep-going). Nested set expansion can be a hot spot, noticed by @justinhorvitz.

PiperOrigin-RevId: 334408725

src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java

diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
index c687f8a..d6c50f9 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
@@ -124,10 +124,9 @@
           valueVersion,
           evaluatorContext.getGraphVersion());
 
-      if (value != null) {
-        ValueWithMetadata valueWithMetadata =
-            ValueWithMetadata.wrapWithMetadata(entry.getValueMaybeWithMetadata());
-        replay(valueWithMetadata);
+      SkyValue valueMaybeWithMetadata = entry.getValueMaybeWithMetadata();
+      if (valueMaybeWithMetadata != null) {
+        replay(ValueWithMetadata.wrapWithMetadata(valueMaybeWithMetadata));
       }
 
       // For most nodes we do not inform the progress receiver if they were already done when we
@@ -402,6 +401,10 @@
             "Current key %s has to be a top-level key: %s",
             errorKey,
             rootValues);
+        SkyValue valueMaybeWithMetadata = errorEntry.getValueMaybeWithMetadata();
+        if (valueMaybeWithMetadata != null) {
+          replay(ValueWithMetadata.wrapWithMetadata(valueMaybeWithMetadata));
+        }
         break;
       }
       SkyKey parent = Preconditions.checkNotNull(Iterables.getFirst(reverseDeps, null));
@@ -506,12 +509,13 @@
               ErrorInfo.fromException(reifiedBuilderException, /*isTransitivelyTransient=*/ false);
           Pair<NestedSet<TaggedEvents>, NestedSet<Postable>> eventsAndPostables =
               env.buildAndReportEventsAndPostables(parentEntry, /*expectDoneDeps=*/ false);
-          bubbleErrorInfo.put(
-              errorKey,
+          ValueWithMetadata valueWithMetadata =
               ValueWithMetadata.error(
                   ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)),
                   eventsAndPostables.first,
-                  eventsAndPostables.second));
+                  eventsAndPostables.second);
+          replay(valueWithMetadata);
+          bubbleErrorInfo.put(errorKey, valueWithMetadata);
           continue;
         }
       } finally {
@@ -530,12 +534,13 @@
       // Builder didn't throw its own exception, so just propagate this one up.
       Pair<NestedSet<TaggedEvents>, NestedSet<Postable>> eventsAndPostables =
           env.buildAndReportEventsAndPostables(parentEntry, /*expectDoneDeps=*/ false);
-      bubbleErrorInfo.put(
-          errorKey,
+      ValueWithMetadata valueWithMetadata =
           ValueWithMetadata.error(
               ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)),
               eventsAndPostables.first,
-              eventsAndPostables.second));
+              eventsAndPostables.second);
+      replay(valueWithMetadata);
+      bubbleErrorInfo.put(errorKey, valueWithMetadata);
     }
 
     // Reset the interrupt bit if there was an interrupt from outside this evaluator interrupt.
@@ -589,8 +594,6 @@
         }
         continue;
       }
-      // Replaying here is necessary for error bubbling and other cases.
-      replay(valueWithMetadata);
       SkyValue value = valueWithMetadata.getValue();
       ErrorInfo errorInfo = valueWithMetadata.getErrorInfo();
       Preconditions.checkState(value != null || errorInfo != null, skyKey);