diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java index f401ea2a446284..908d6bcfffa63b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryDirectPackageProviderFactory.java @@ -13,11 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.rules.genquery; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.base.Preconditions; import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; +import com.google.common.collect.LinkedHashMultimap; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.concurrent.BlazeInterners; @@ -37,8 +40,12 @@ import com.google.devtools.build.lib.skyframe.TargetLoadingUtil.TargetAndErrorIfAny; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.skyframe.AbstractSkyKey; +import com.google.devtools.build.skyframe.PartialReevaluationMailbox; +import com.google.devtools.build.skyframe.PartialReevaluationMailbox.Causes; +import com.google.devtools.build.skyframe.PartialReevaluationMailbox.Mail; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunction.Environment; +import com.google.devtools.build.skyframe.SkyFunction.Environment.ClassToInstanceMapSkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; @@ -58,7 +65,6 @@ * information. */ public class GenQueryDirectPackageProviderFactory implements GenQueryPackageProviderFactory { - public static final SkyFunctionName GENQUERY_SCOPE = SkyFunctionName.createHermetic("GENQUERY_SCOPE"); @@ -142,7 +148,7 @@ protected BrokenQueryScopeSkyFunctionException( *

([0] In the future, {@code collectedPackages} might also contain packages needed to evaluate * "buildfiles" functions; see b/123795023.) * - *

The {@code labelsToVisitNextRestart} field contains labels of targets belonging to + *

The {@code labelsToVisitInLaterRestart} field contains labels of targets belonging to * previously unloaded packages, the "frontier" of the last Skyframe evaluation attempt's * traversal. */ @@ -150,11 +156,10 @@ private static class ScopeTraversal implements SkyKeyComputeState { private final LinkedHashMap collectedPackages = new LinkedHashMap<>(); private final LinkedHashMap collectedTargets = new LinkedHashMap<>(); - private final LinkedHashSet

Establishes all Skyframe dependencies needed for incremental correctness. * - *

Returns {@code null} if {@code env.valuesMissing()}. + *

Returns {@link TargetAndErrorIfAny} if no dep was mising; otherwise, returns the {@link + * SkyKey} specifying the missing dep. */ - @Nullable - public static TargetAndErrorIfAny loadTarget(Environment env, Label label) + public static Object loadTarget(Environment env, Label label) throws NoSuchTargetException, NoSuchPackageException, InterruptedException { if (label.getName().contains("/")) { // This target is in a subdirectory, therefore it could potentially be invalidated by @@ -50,18 +50,19 @@ public static TargetAndErrorIfAny loadTarget(Environment env, Label label) PackageIdentifier newPkgId = PackageIdentifier.create(label.getRepository(), containingDirectory); ContainingPackageLookupValue containingPackageLookupValue; + SkyKey containingPackageKey = ContainingPackageLookupValue.key(newPkgId); try { containingPackageLookupValue = (ContainingPackageLookupValue) env.getValueOrThrow( - ContainingPackageLookupValue.key(newPkgId), + containingPackageKey, BuildFileNotFoundException.class, InconsistentFilesystemException.class); } catch (InconsistentFilesystemException e) { throw new NoSuchTargetException(label, e.getMessage()); } if (containingPackageLookupValue == null) { - return null; + return containingPackageKey; } if (!containingPackageLookupValue.hasContainingPackage()) { @@ -88,7 +89,7 @@ public static TargetAndErrorIfAny loadTarget(Environment env, Label label) PackageValue packageValue = (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class); if (packageValue == null) { - return null; + return packageKey; } Package pkg = packageValue.getPackage(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java index b90bcc9e12849f..b1a57d82277db2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java @@ -220,7 +220,8 @@ private boolean hasDepThatSatisfies( @Nullable TargetAndErrorIfAny loadTarget(Environment env, Label label) throws NoSuchTargetException, NoSuchPackageException, InterruptedException { - return TargetLoadingUtil.loadTarget(env, label); + Object o = TargetLoadingUtil.loadTarget(env, label); + return o instanceof TargetAndErrorIfAny ? (TargetAndErrorIfAny) o : null; } /** diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java index 269fdeb51b45a4..dca30380bfa547 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java @@ -158,8 +158,8 @@ abstract class AbstractParallelEvaluator { graphInconsistencyReceiver, () -> new NodeEntryVisitor( - quiescingExecutorSupplier.get(), progressReceiver, Evaluate::new), - /*mergingSkyframeAnalysisExecutionPhases=*/ executionJobsThreadPoolSize > 0, + quiescingExecutorSupplier.get(), progressReceiver, Evaluate::new, stateCache), + /* mergingSkyframeAnalysisExecutionPhases= */ executionJobsThreadPoolSize > 0, stateCache); } @@ -171,7 +171,7 @@ private static Supplier getQuiescingExecutorSupplier( return () -> AbstractQueueVisitor.createWithExecutorService( executorService.get(), - /*failFastOnException=*/ true, + /* failFastOnException= */ true, NodeEntryVisitor.NODE_ENTRY_VISITOR_ERROR_CLASSIFIER); } if (executionJobsThreadPoolSize <= 0) { @@ -179,11 +179,11 @@ private static Supplier getQuiescingExecutorSupplier( MultiExecutorQueueVisitor.createWithExecutorServices( executorService.get(), AbstractQueueVisitor.createExecutorService( - /*parallelism=*/ cpuHeavySkyKeysThreadPoolSize, + /* parallelism= */ cpuHeavySkyKeysThreadPoolSize, "skyframe-evaluator-cpu-heavy", // FJP performs much better on machines with many cores. - /*useForkJoinPool=*/ true), - /*failFastOnException=*/ true, + /* useForkJoinPool= */ true), + /* failFastOnException= */ true, NodeEntryVisitor.NODE_ENTRY_VISITOR_ERROR_CLASSIFIER); } // We only consider the experimental case of merged Skyframe phases WITH a separate pool for @@ -192,16 +192,16 @@ private static Supplier getQuiescingExecutorSupplier( MultiExecutorQueueVisitor.createWithExecutorServices( executorService.get(), AbstractQueueVisitor.createExecutorService( - /*parallelism=*/ cpuHeavySkyKeysThreadPoolSize, + /* parallelism= */ cpuHeavySkyKeysThreadPoolSize, "skyframe-evaluator-cpu-heavy", // FJP performs much better on machines with many cores. - /*useForkJoinPool=*/ true), + /* useForkJoinPool= */ true), AbstractQueueVisitor.createExecutorService( - /*parallelism=*/ executionJobsThreadPoolSize, + /* parallelism= */ executionJobsThreadPoolSize, "skyframe-evaluator-execution", // FJP performs much better on machines with many cores. - /*useForkJoinPool=*/ true), - /*failFastOnException=*/ true, + /* useForkJoinPool= */ true), + /* failFastOnException= */ true, NodeEntryVisitor.NODE_ENTRY_VISITOR_ERROR_CLASSIFIER); } @@ -284,6 +284,7 @@ private int evenHigherPriority() { * child or the parent, returning whether the parent has both been signalled and also is ready * for evaluation. */ + @CanIgnoreReturnValue private boolean enqueueChild( SkyKey skyKey, NodeEntry entry, @@ -291,7 +292,8 @@ private boolean enqueueChild( NodeEntry childEntry, boolean depAlreadyExists, int childEvaluationPriority, - boolean enqueueParentIfReady) + boolean enqueueParentIfReady, + @Nullable SkyFunctionEnvironment environmentIfEnqueuing) throws InterruptedException { checkState(!entry.isDone(), "%s %s", skyKey, entry); DependencyState dependencyState; @@ -308,15 +310,28 @@ private boolean enqueueChild( case DONE: if (entry.signalDep(childEntry.getVersion(), child)) { if (enqueueParentIfReady) { - evaluatorContext.getVisitor().enqueueEvaluation(skyKey, determineRestartPriority()); + evaluatorContext + .getVisitor() + .enqueueEvaluation(skyKey, determineRestartPriority(), child); } return true; + } else { + if (skyKey.supportsPartialReevaluation() + && environmentIfEnqueuing != null + && environmentIfEnqueuing.wasNewlyRequestedDepNullForPartialReevaluation(child)) { + // If a dep was observed not-done by its parent when the parent tried to read its + // value, but that dep is now done, then this is the only chance the parent has to be + // signalled by that dep. + evaluatorContext + .getVisitor() + .enqueueEvaluation(skyKey, determineRestartPriority(), child); + } } break; case ALREADY_EVALUATING: break; case NEEDS_SCHEDULING: - evaluatorContext.getVisitor().enqueueEvaluation(child, childEvaluationPriority); + evaluatorContext.getVisitor().enqueueEvaluation(child, childEvaluationPriority, null); break; } return false; @@ -438,7 +453,8 @@ private DirtyOutcome maybeHandleDirtyNode(NodeEntry nodeEntry) throws Interrupte entriesToCheck, nodeEntry, determineChildPriority(), - /*enqueueParentIfReady=*/ false); + /* enqueueParentIfReady= */ false, + /* environmentIfEnqueuing= */ null); if (!parentIsSignalledAndReady || evaluatorContext.getVisitor().shouldPreventNewEvaluations()) { return DirtyOutcome.ALREADY_PROCESSED; @@ -462,8 +478,8 @@ private DirtyOutcome maybeHandleDirtyNode(NodeEntry nodeEntry) throws Interrupte .getProgressReceiver() .evaluated( skyKey, - /*newValue=*/ null, - /*newError=*/ null, + /* newValue= */ null, + /* newError= */ null, new EvaluationSuccessStateSupplier(nodeEntry), EvaluationState.CLEAN); if (!evaluatorContext.keepGoing() && nodeEntry.getErrorInfo() != null) { @@ -496,7 +512,8 @@ private boolean handleKnownChildrenForDirtyNode( NodeBatch oldChildren, NodeEntry nodeEntry, int childEvaluationPriority, - boolean enqueueParentIfReady) + boolean enqueueParentIfReady, + @Nullable SkyFunctionEnvironment environmentIfEnqueuing) throws InterruptedException { boolean parentIsSignalledAndReady = false; for (SkyKey directDep : knownChildren) { @@ -513,9 +530,10 @@ private boolean handleKnownChildrenForDirtyNode( nodeEntry, directDep, directDepEntry, - /*depAlreadyExists=*/ true, + /* depAlreadyExists= */ true, childEvaluationPriority, - enqueueParentIfReady); + enqueueParentIfReady, + environmentIfEnqueuing); } return parentIsSignalledAndReady; } @@ -549,10 +567,7 @@ public void run() { SkyFunctionEnvironment.create( skyKey, nodeEntry.getTemporaryDirectDeps(), oldDeps, evaluatorContext); } catch (UndonePreviouslyRequestedDeps undonePreviouslyRequestedDeps) { - // If a previously requested dep is no longer done, restart this node from scratch. - stateCache.invalidate(skyKey); - resetEntry(skyKey, nodeEntry); - evaluatorContext.getVisitor().enqueueEvaluation(skyKey, determineRestartPriority()); + handleUndonePreviouslyRequestedDep(nodeEntry); return; } finally { evaluatorContext @@ -615,6 +630,13 @@ public void run() { return; } + try { + env.ensurePreviouslyRequestedDepsFetched(); + } catch (UndonePreviouslyRequestedDeps e) { + handleUndonePreviouslyRequestedDep(nodeEntry); + return; + } + boolean shouldFailFast = !evaluatorContext.keepGoing() || builderException.isCatastrophic(); if (shouldFailFast) { @@ -671,7 +693,7 @@ public void run() { dirtyRewindGraphAndResetEntry(skyKey, nodeEntry, (Restart) value); stateCache.invalidate(skyKey); cancelExternalDeps(env); - evaluatorContext.getVisitor().enqueueEvaluation(skyKey, determineRestartPriority()); + evaluatorContext.getVisitor().enqueueEvaluation(skyKey, determineRestartPriority(), null); return; } @@ -683,6 +705,13 @@ public void run() { return; } + try { + env.ensurePreviouslyRequestedDepsFetched(); + } catch (UndonePreviouslyRequestedDeps e) { + handleUndonePreviouslyRequestedDep(nodeEntry); + return; + } + checkState( !env.valuesMissing(), "Evaluation of %s returned non-null value but requested dependencies that weren't " @@ -840,7 +869,8 @@ public void run() { graph.getBatch(skyKey, Reason.ENQUEUING_CHILD, newDepsThatWereInTheLastEvaluation), nodeEntry, childEvaluationPriority, - /*enqueueParentIfReady=*/ true); + /* enqueueParentIfReady= */ true, + env); // Due to multi-threading, this can potentially cause the current node to be re-enqueued if // all 'new' children of this node are already done. Therefore, there should not be any code @@ -852,9 +882,10 @@ public void run() { nodeEntry, newDirectDep, newNodes.get(newDirectDep), - /*depAlreadyExists=*/ false, + /* depAlreadyExists= */ false, childEvaluationPriority, - /*enqueueParentIfReady=*/ true); + /* enqueueParentIfReady= */ true, + env); } if (externalDeps != null) { // This can cause the current node to be re-enqueued if all futures are already done. @@ -881,10 +912,17 @@ public void run() { } } + private void handleUndonePreviouslyRequestedDep(NodeEntry nodeEntry) { + // If a previously requested dep is no longer done, restart this node from scratch. + stateCache.invalidate(skyKey); + resetEntry(skyKey, nodeEntry); + evaluatorContext.getVisitor().enqueueEvaluation(skyKey, determineRestartPriority(), null); + } + private void cancelExternalDeps(SkyFunctionEnvironment env) { if (env != null && env.externalDeps != null) { for (ListenableFuture future : env.externalDeps) { - future.cancel(/*mayInterruptIfRunning=*/ true); + future.cancel(/* mayInterruptIfRunning= */ true); } } } @@ -1014,7 +1052,7 @@ private void dirtyRewindGraphAndResetEntry(SkyKey key, NodeEntry entry, Restart private void resetEntry(SkyKey key, NodeEntry entry) { evaluatorContext .getGraphInconsistencyReceiver() - .noteInconsistencyAndMaybeThrow(key, /*otherKeys=*/ null, Inconsistency.RESET_REQUESTED); + .noteInconsistencyAndMaybeThrow(key, /* otherKeys= */ null, Inconsistency.RESET_REQUESTED); entry.resetForRestartFromScratch(); } @@ -1191,7 +1229,7 @@ private MaybeHandleUndoneDepResult maybeHandleUndoneDepForDoneEntry( .noteInconsistencyAndMaybeThrow( skyKey, ImmutableList.of(depKey), Inconsistency.BUILDING_PARENT_FOUND_UNDONE_CHILD); if (triState == DependencyState.NEEDS_SCHEDULING) { - evaluatorContext.getVisitor().enqueueEvaluation(depKey, FIRST_RESTART_PRIORITY); + evaluatorContext.getVisitor().enqueueEvaluation(depKey, FIRST_RESTART_PRIORITY, null); } return MaybeHandleUndoneDepResult.DEP_NOT_DONE; } diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntryVisitor.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntryVisitor.java index e7e7392a592331..0aaab1643dc639 100644 --- a/src/main/java/com/google/devtools/build/skyframe/NodeEntryVisitor.java +++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntryVisitor.java @@ -15,6 +15,7 @@ import static com.google.common.base.Preconditions.checkNotNull; +import com.github.benmanes.caffeine.cache.Cache; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Sets; import com.google.common.util.concurrent.Futures; @@ -26,12 +27,15 @@ import com.google.devtools.build.lib.concurrent.QuiescingExecutor; import com.google.devtools.build.skyframe.ParallelEvaluatorContext.ComparableRunnable; import com.google.devtools.build.skyframe.ParallelEvaluatorContext.RunnableMaker; +import com.google.devtools.build.skyframe.SkyFunction.Environment.ClassToInstanceMapSkyKeyComputeState; +import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; import java.util.Collection; import java.util.List; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicBoolean; +import javax.annotation.Nullable; /** * Threadpool manager for {@link ParallelEvaluator}. Wraps a {@link QuiescingExecutor} and keeps @@ -67,6 +71,7 @@ protected ErrorClassification classifyException(Exception e) { private final RunnableMaker runnableMaker; private final RunnableMaker partialReevaluationRunnableMaker; + private final Cache stateCache; /** * This state enum is used with {@link #partialReevaluationStates} to describe, for each {@link @@ -139,11 +144,13 @@ public void run() { NodeEntryVisitor( QuiescingExecutor quiescingExecutor, DirtyTrackingProgressReceiver progressReceiver, - RunnableMaker runnableMaker) { + RunnableMaker runnableMaker, + Cache stateCache) { this.quiescingExecutor = quiescingExecutor; this.progressReceiver = progressReceiver; this.runnableMaker = runnableMaker; this.partialReevaluationRunnableMaker = new PartialReevaluationRunnableMaker(); + this.stateCache = stateCache; } void waitForCompletion() throws InterruptedException { @@ -170,9 +177,9 @@ void waitForCompletion() throws InterruptedException { * Similarly, prioritizing deeper nodes (depth-first search of the evaluation graph) also has good * results experimentally, since it minimizes sprawl. */ - void enqueueEvaluation(SkyKey key, int evaluationPriority) { + void enqueueEvaluation(SkyKey key, int evaluationPriority, @Nullable SkyKey signalingDep) { if (key.supportsPartialReevaluation()) { - enqueuePartialReevaluation(key, evaluationPriority); + enqueuePartialReevaluation(key, evaluationPriority, signalingDep); } else { innerEnqueueEvaluation(key, evaluationPriority, runnableMaker); } @@ -200,7 +207,7 @@ void registerExternalDeps( .run( () -> { if (entry.signalDep(entry.getVersion(), null)) { - enqueueEvaluation(skyKey, evaluationPriority); + enqueueEvaluation(skyKey, evaluationPriority, null); } }, MoreExecutors.directExecutor()); @@ -241,7 +248,15 @@ CountDownLatch getExceptionLatchForTestingOnly() { return quiescingExecutor.getExceptionLatchForTestingOnly(); } - private void enqueuePartialReevaluation(SkyKey key, int evaluationPriority) { + private void enqueuePartialReevaluation( + SkyKey key, int evaluationPriority, @Nullable SkyKey signalingDep) { + PartialReevaluationMailbox mailbox = getMailbox(key); + if (signalingDep != null) { + mailbox.signal(signalingDep); + } else { + mailbox.enqueuedNotByDeps(); + } + PartialReevaluationState reevaluationState = partialReevaluationStates.compute( key, @@ -254,6 +269,12 @@ private void enqueuePartialReevaluation(SkyKey key, int evaluationPriority) { } } + private PartialReevaluationMailbox getMailbox(SkyKey key) { + return PartialReevaluationMailbox.from( + (ClassToInstanceMapSkyKeyComputeState) + stateCache.get(key, k -> new ClassToInstanceMapSkyKeyComputeState())); + } + private void innerEnqueueEvaluation( SkyKey key, int evaluationPriority, RunnableMaker runnableMakerToUse) { if (shouldPreventNewEvaluations()) { diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java index 853759d89c9000..15fb78885638f4 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java @@ -148,7 +148,7 @@ private EvaluationResult doMutatingEvaluation( case NEEDS_SCHEDULING: // Low priority because this node is not needed by any other currently evaluating node. // So keep it at the back of the queue as long as there's other useful work to be done. - evaluatorContext.getVisitor().enqueueEvaluation(skyKey, Integer.MIN_VALUE); + evaluatorContext.getVisitor().enqueueEvaluation(skyKey, Integer.MIN_VALUE, null); break; case DONE: informProgressReceiverThatValueIsDone(skyKey, entry); diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java index 588497caf7262b..79dc6cb6dbd534 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java @@ -130,7 +130,7 @@ void signalParentsAndEnqueueIfReady( NodeEntry entry = checkNotNull(batch.get(parent), parent); boolean evaluationRequired = entry.signalDep(version, skyKey); if (evaluationRequired || parent.supportsPartialReevaluation()) { - getVisitor().enqueueEvaluation(parent, evaluationPriority); + getVisitor().enqueueEvaluation(parent, evaluationPriority, skyKey); } } } diff --git a/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java b/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java new file mode 100644 index 00000000000000..d5e9c17093305c --- /dev/null +++ b/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java @@ -0,0 +1,202 @@ +// Copyright 2022 The Bazel Authors. 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.skyframe; + +import com.google.auto.value.AutoOneOf; +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.skyframe.SkyFunction.Environment.ClassToInstanceMapSkyKeyComputeState; +import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; +import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; + +/** + * Contains the causes describing why a node, which opted into partial reevaluation, is getting + * reevaluated. + * + *

Accessible via {@link SkyKeyComputeState}. Nodes opting into partial reevaluation must access + * compute states via {@link ClassToInstanceMapSkyKeyComputeState}. + * + *

A node's mailbox may be in one of three general states: + * + *

    + *
  1. "freshly initialized", + *
  2. containing causes for the node's partial reevaluation, or, + *
  3. empty of such causes. + *
+ * + *

See {@link Kind} for details. + * + *

The "Mailbox" naming convention comes from actor models, where concurrent processors of work + * coordinate by sending each other messages that get stored in "mailboxes" until consumed; see + * https://wikipedia.org/wiki/Erlang_(programming_language)#Concurrency_and_distribution_orientation + * for discussion. + */ +public class PartialReevaluationMailbox implements SkyKeyComputeState { + + /** Will be {@code null} only before the first call to {@link #getMail()}. */ + @GuardedBy("this") + @Nullable + private ImmutableList.Builder signaledDeps; + + @GuardedBy("this") + private boolean other; + + /** General states that a mailbox may be in. */ + public enum Kind { + /** + * Represents the first time a mailbox is accessed by its node. A mailbox may also be in this + * state because the mailbox's data was dropped due to memory pressure, or because of other + * Skyframe nodes completing in error. A {@link SkyFunction} that observes this state should + * (re)evaluate "from scratch"; its other {@link SkyKeyComputeState} data will be in a freshly + * initialized state too. + */ + FRESHLY_INITIALIZED, + + /** + * Represents a nonempty set of causes for a node's reevaluation. See {@link Causes} for + * details. + */ + CAUSES, + + /** + * Represents an empty set of causes for a node's reevaluation. + * + *

Reading from a mailbox, via {@link #getMail()}, empties it. Thereafter, it will no longer + * be {@link #FRESHLY_INITIALIZED}, unless Skyframe drops its {@link SkyKeyComputeState}. + * Reading empties its list of signaled dep keys and sets its {@link Causes#other} flag back to + * {@code false}. + * + *

This empty state may be observed during a reevaluation, even from a reevaluation's first + * read from its mailbox. When an event occurs that may cause a reevaluation (e.g., when a dep + * completes) adding that cause (e.g., that dep's key) to a parent's mailbox can race with that + * parent reading its mailbox if the parent is reevaluating at the same time. If such an add + * wins the race, then the parent consumes the cause during that reevaluation. The event may + * then schedule a subsequent reevaluation for that parent, which is necessary to handle the + * case in which the add lost the race. If no other causes get added before the parent reads its + * mailbox in that subsequent reevaluation, then the mailbox may be empty. + */ + EMPTY, + } + + /** + * A mailbox's detailed state, including whether it was freshly initialized, and the causes it + * contains for its node's partial reevaluation, if any. + */ + @AutoOneOf(Kind.class) + public abstract static class Mail { + public abstract Kind kind(); + + abstract void freshlyInitialized(); + + abstract void empty(); + + public abstract Causes causes(); + + static Mail ofFreshlyInitialized() { + return AutoOneOf_PartialReevaluationMailbox_Mail.freshlyInitialized(); + } + + static Mail ofEmpty() { + return AutoOneOf_PartialReevaluationMailbox_Mail.empty(); + } + + static Mail ofCauses(Causes causes) { + return AutoOneOf_PartialReevaluationMailbox_Mail.causes(causes); + } + } + + /** + * A nonempty set of causes for a node's partial reevaluation. + * + *

A dep which a parent node previously requested and observed to not be done will have its key + * added to that parent's mailbox after the dep completes and before the dep signals the parent. + * {@link #signaledDeps} returns that list of keys. + * + *

Skyframe may enqueue a node for evaluation for several other reasons, such as when the node + * declared an external dependency (via {@link SkyFunction.Environment#dependOnFuture}) that + * completes, or when the node's {@link SkyFunction#compute} method returns a {@link + * SkyFunction.Restart} value and the node is restarted. In some of these cases (e.g. returning a + * {@link SkyFunction.Restart} value), the node's {@link SkyKeyComputeState} will be invalidated, + * which also drops its mailbox, and the next time that mailbox is read it will return a "freshly + * initialized" state. But in others (e.g. an external dependency completes), the node's {@link + * SkyKeyComputeState} is retained. In any of these cases in which a node is enqueued for + * evaluation and its mailbox is retained, a flag will be set in the node's mailbox to indicate + * that the node's {@link SkyFunction} should try its best to make progress, by, e.g., checking + * whether its external dep futures have completed, checking whether its previously requested deps + * are done, or reevaluating from scratch. ({@link Causes#other}) returns the value of that flag. + */ + @AutoValue + public abstract static class Causes { + static Causes create(ImmutableList signaledDeps, boolean other) { + return new AutoValue_PartialReevaluationMailbox_Causes(signaledDeps, other); + } + + /** + * {@link SkyKey}s of previously requested deps which have completed since the last time the + * mailbox was read. + */ + public abstract ImmutableList signaledDeps(); + + /** + * Whether Skyframe enqueued a reevaluation for any other reason besides a dep completing + * normally, in such a way that the dep's key would be added to {@link #signaledDeps}. + */ + public abstract boolean other(); + } + + private PartialReevaluationMailbox() {} + + public static PartialReevaluationMailbox from(ClassToInstanceMapSkyKeyComputeState computeState) { + return computeState.getInstance( + PartialReevaluationMailbox.class, PartialReevaluationMailbox::new); + } + + /** Used by Skyframe to record that a dep has signaled a node opting into partial reevaluation. */ + synchronized void signal(SkyKey dep) { + if (signaledDeps != null) { + signaledDeps.add(dep); + } + } + + /** + * Used by Skyframe to record that a node opting into partial reevaluation has been enqueued for + * evaluation in contexts where that happens for reasons other than a dep signaling it. + */ + synchronized void enqueuedNotByDeps() { + other = true; + } + + /** Gets and clears the current causes for a node's partial reevaluation. */ + public Mail getMail() { + @Nullable ImmutableList.Builder signaledDeps; + boolean other; + ImmutableList.Builder newBuilder = new ImmutableList.Builder<>(); + synchronized (this) { + signaledDeps = this.signaledDeps; + this.signaledDeps = newBuilder; + + other = this.other; + this.other = false; + } + if (signaledDeps == null) { + return Mail.ofFreshlyInitialized(); + } + ImmutableList signaledDepsList = signaledDeps.build(); + if (signaledDepsList.isEmpty() && !other) { + return Mail.ofEmpty(); + } + return Mail.ofCauses(Causes.create(signaledDepsList, other)); + } +} diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index d9adc6ee0a8002..cdc1a6389c3940 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java @@ -39,9 +39,11 @@ import com.google.devtools.build.skyframe.QueryableGraph.Reason; import com.google.devtools.build.skyframe.proto.GraphInconsistency.Inconsistency; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.errorprone.annotations.ForOverride; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -53,7 +55,7 @@ import javax.annotation.Nullable; /** A {@link SkyFunction.Environment} implementation for {@link ParallelEvaluator}. */ -final class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment +class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment implements SkyframeLookupResult, ExtendedEventHandler { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @@ -103,8 +105,12 @@ final class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment *

Values in this map were generally retrieved via {@link NodeEntry#getValueMaybeWithMetadata} * from done nodes. In some cases, values may be {@link #NULL_MARKER} (see {@link #batchPrefetch} * for more details). + * + *

In {@link PartialReevaluation}, this map is not exhaustive. It populates as the {@link + * SkyFunction} re-requests dep values, and will contain {@link #PENDING_MARKER}s when a key is + * about to be requested from the graph. */ - private final ImmutableMap previouslyRequestedDepsValues; + private final Map previouslyRequestedDepsValues; /** * The values newly requested from the graph during the {@link SkyFunction#compute} call for this @@ -151,13 +157,18 @@ static SkyFunctionEnvironment create( Set oldDeps, ParallelEvaluatorContext evaluatorContext) throws InterruptedException, UndonePreviouslyRequestedDeps { + if (skyKey.supportsPartialReevaluation()) { + return new SkyFunctionEnvironment.PartialReevaluation( + skyKey, previouslyRequestedDeps, oldDeps, evaluatorContext); + } + return new SkyFunctionEnvironment( skyKey, previouslyRequestedDeps, /* bubbleErrorInfo= */ null, oldDeps, evaluatorContext, - /* throwIfPreviouslyRequestedDepsUndone= */ !skyKey.supportsPartialReevaluation()); + /* throwIfPreviouslyRequestedDepsUndone= */ true); } static SkyFunctionEnvironment createForError( @@ -174,7 +185,7 @@ static SkyFunctionEnvironment createForError( checkNotNull(bubbleErrorInfo), oldDeps, evaluatorContext, - /*throwIfPreviouslyRequestedDepsUndone=*/ false); + /* throwIfPreviouslyRequestedDepsUndone= */ false); } catch (UndonePreviouslyRequestedDeps undonePreviouslyRequestedDeps) { throw new IllegalStateException(undonePreviouslyRequestedDeps); } @@ -201,13 +212,10 @@ private SkyFunctionEnvironment( ? evaluatorContext.getMinimalVersion() : null; this.previouslyRequestedDepsValues = batchPrefetch(throwIfPreviouslyRequestedDepsUndone); - checkState( - !this.previouslyRequestedDepsValues.containsKey(ErrorTransienceValue.KEY), - "%s cannot have a dep on ErrorTransienceValue during building", - skyKey); } - private ImmutableMap batchPrefetch(boolean throwIfPreviouslyRequestedDepsUndone) + @ForOverride + Map batchPrefetch(boolean throwIfPreviouslyRequestedDepsUndone) throws InterruptedException, UndonePreviouslyRequestedDeps { ImmutableSet excludedKeys = evaluatorContext.getGraph().prefetchDeps(skyKey, oldDeps, previouslyRequestedDeps); @@ -256,7 +264,12 @@ private ImmutableMap batchPrefetch(boolean throwIfPreviouslyRe throw new UndonePreviouslyRequestedDeps(allMissingDeps); } - return depValuesBuilder.buildOrThrow(); + ImmutableMap prefetched = depValuesBuilder.buildOrThrow(); + checkState( + !prefetched.containsKey(ErrorTransienceValue.KEY), + "%s cannot have a dep on ErrorTransienceValue during building", + skyKey); + return prefetched; } private void checkActive() { @@ -449,32 +462,25 @@ SkyValue maybeGetValueFromErrorOrDeps(SkyKey key) { return directDepsValue == MANUALLY_REGISTERED_MARKER ? null : directDepsValue; } - /** - * Similar to {@link #maybeGetValueFromErrorOrDeps} but tracks new dependencies by mutating {@link - * #newlyRequestedDepsValues}. - * - *

A return of {@code null} indicates that the key should be requested from the graph. May also - * return {@link #NULL_MARKER} or {@link #PENDING_MARKER}, but translates {@link - * #MANUALLY_REGISTERED_MARKER} to {@code null}. - */ + @ForOverride @Nullable - private SkyValue lookupRequestedDep(SkyKey key) { + SkyValue lookupRequestedDep(SkyKey depKey) { checkArgument( - !key.equals(ErrorTransienceValue.KEY), + !depKey.equals(ErrorTransienceValue.KEY), "Error transience key cannot be in requested deps of %s", skyKey); if (bubbleErrorInfo != null) { - ValueWithMetadata bubbleErrorInfoValue = bubbleErrorInfo.get(key); + ValueWithMetadata bubbleErrorInfoValue = bubbleErrorInfo.get(depKey); if (bubbleErrorInfoValue != null) { - newlyRequestedDepsValues.put(key, bubbleErrorInfoValue); + newlyRequestedDepsValues.put(depKey, bubbleErrorInfoValue); return bubbleErrorInfoValue; } } - SkyValue directDepsValue = previouslyRequestedDepsValues.get(key); + SkyValue directDepsValue = previouslyRequestedDepsValues.get(depKey); if (directDepsValue != null) { return directDepsValue; } - directDepsValue = newlyRequestedDepsValues.putIfAbsent(key, PENDING_MARKER); + directDepsValue = newlyRequestedDepsValues.putIfAbsent(depKey, PENDING_MARKER); return directDepsValue == MANUALLY_REGISTERED_MARKER ? null : directDepsValue; } @@ -534,21 +540,21 @@ public SkyframeLookupResult getValuesAndExceptions(Iterable de List missingKeys = null; int sizeBeforeRequest = newlyRequestedDepsValues.size(); - for (SkyKey key : depKeys) { - SkyValue value = lookupRequestedDep(key); + for (SkyKey depKey : depKeys) { + SkyValue value = lookupRequestedDep(depKey); if (value == PENDING_MARKER) { continue; // Duplicate key in this request. } if (value != null) { - processDepValue(key, value); - } else if (evaluatorContext.getGraph().getLookupHint(key) == LookupHint.BATCH) { + processDepValue(depKey, value); + } else if (evaluatorContext.getGraph().getLookupHint(depKey) == LookupHint.BATCH) { if (missingKeys == null) { missingKeys = new ArrayList<>(); } - missingKeys.add(key); + missingKeys.add(depKey); } else { - NodeEntry depEntry = evaluatorContext.getGraph().get(skyKey, Reason.DEP_REQUESTED, key); - processDepEntry(key, depEntry); + NodeEntry depEntry = evaluatorContext.getGraph().get(skyKey, Reason.DEP_REQUESTED, depKey); + processDepEntry(depKey, depEntry); } } endDepGroup(sizeBeforeRequest); @@ -564,8 +570,9 @@ public SkyframeLookupResult getValuesAndExceptions(Iterable de return this; } + @ForOverride @CanIgnoreReturnValue - private SkyValue processDepEntry(SkyKey depKey, @Nullable NodeEntry depEntry) + SkyValue processDepEntry(SkyKey depKey, @Nullable NodeEntry depEntry) throws InterruptedException { SkyValue valueOrNullMarker = getValueOrNullMarker(depEntry); processDepValue(depKey, valueOrNullMarker); @@ -576,7 +583,7 @@ private SkyValue processDepEntry(SkyKey depKey, @Nullable NodeEntry depEntry) return valueOrNullMarker; } - private void processDepValue(SkyKey depKey, SkyValue depValue) { + void processDepValue(SkyKey depKey, SkyValue depValue) { if (depValue == NULL_MARKER) { valuesMissing = true; return; @@ -972,7 +979,7 @@ public void injectVersionForNonHermeticFunction(Version version) { maxTransitiveSourceVersion = version; } - private void maybeUpdateMaxTransitiveSourceVersion(NodeEntry depEntry) { + void maybeUpdateMaxTransitiveSourceVersion(NodeEntry depEntry) { if (maxTransitiveSourceVersion == null || skyKey.functionName().getHermeticity() == FunctionHermeticity.NONHERMETIC) { return; @@ -1028,6 +1035,16 @@ public Version getMaxTransitiveSourceVersionSoFar() { return maxTransitiveSourceVersion; } + void ensurePreviouslyRequestedDepsFetched() + throws UndonePreviouslyRequestedDeps, InterruptedException { + // Do nothing; previously requested deps were already fetched and checked for done-ness in + // batchPrefetch. + } + + boolean wasNewlyRequestedDepNullForPartialReevaluation(SkyKey newlyRequestedDep) { + return false; + } + /** Thrown during environment construction if a previously requested dep is no longer done. */ static final class UndonePreviouslyRequestedDeps extends Exception { private final ImmutableList depKeys; @@ -1040,4 +1057,154 @@ ImmutableList getDepKeys() { return depKeys; } } + + /** + * The environment for a partial reevaluation differs from a regular environment in the following + * ways: + * + *

Previously requested deps are not eagerly prefetched, for performance reasons. Instead, + * their values are read from the graph on demand, in the same way as newly requested deps. + * + *

The {@link #ensurePreviouslyRequestedDepsFetched} method, which gets called prior to node + * completion, isn't a no-op, because they weren't prefetched. They're needed for version, error, + * and event data during node completion. + * + *

The {@link #wasNewlyRequestedDepNullForPartialReevaluation} method may return {@code true}, + * when the evaluator checks for a newly requested done dep to which the current node is being + * added as an rdep, to ensure that dep's key gets delivered to this node's mailbox. + */ + private static final class PartialReevaluation extends SkyFunctionEnvironment { + + private final ImmutableSet previouslyRequestedDepsSet; + + private PartialReevaluation( + SkyKey skyKey, + GroupedList previouslyRequestedDeps, + Set oldDeps, + ParallelEvaluatorContext evaluatorContext) + throws UndonePreviouslyRequestedDeps, InterruptedException { + super( + skyKey, + previouslyRequestedDeps, + /* bubbleErrorInfo= */ null, + oldDeps, + evaluatorContext, + false); + this.previouslyRequestedDepsSet = previouslyRequestedDeps.toSet(); + } + + @Override + Map batchPrefetch(boolean throwIfPreviouslyRequestedDepsUndone) { + // Partial reevaluations don't prefetch all previously requested deps, because doing so is too + // expensive, with how many more times those nodes get reevaluated. + return new HashMap<>(); + } + + @Nullable + @Override + SkyValue lookupRequestedDep(SkyKey depKey) { + SkyFunctionEnvironment env = this; + checkArgument( + !depKey.equals(ErrorTransienceValue.KEY), + "Error transience key cannot be in requested deps of %s", + env.skyKey); + if (previouslyRequestedDepsSet.contains(depKey)) { + return env.previouslyRequestedDepsValues.putIfAbsent(depKey, PENDING_MARKER); + } + SkyValue directDepsValue = env.newlyRequestedDepsValues.putIfAbsent(depKey, PENDING_MARKER); + return directDepsValue == MANUALLY_REGISTERED_MARKER ? null : directDepsValue; + } + + @CanIgnoreReturnValue + @Override + SkyValue processDepEntry(SkyKey depKey, @Nullable NodeEntry depEntry) + throws InterruptedException { + SkyFunctionEnvironment env = this; + SkyValue valueOrNullMarker = getValueOrNullMarker(depEntry); + processDepValue(depKey, valueOrNullMarker); + if (previouslyRequestedDepsSet.contains(depKey)) { + env.previouslyRequestedDepsValues.put(depKey, valueOrNullMarker); + } else { + env.newlyRequestedDepsValues.put(depKey, valueOrNullMarker); + } + if (valueOrNullMarker != NULL_MARKER) { + maybeUpdateMaxTransitiveSourceVersion(depEntry); + } + return valueOrNullMarker; + } + + @Nullable + @Override + public Version getMaxTransitiveSourceVersionSoFar() { + // Currently, this is only used by ActionSketchFunction and that is not a PartialReevaluation + // node. It could return the wrong value for PartialReevaluation nodes, because they do not + // batchPrefetch all their previously requested deps during each restart. + // + // Note that, if we wanted to support this method for PartialReevaluation nodes, then we have + // a couple options: + // * we could implement a version that returns a definitely correct answer by first checking + // all previously requested deps (by, e.g., calling #ensurePreviouslyRequestedDepsFetched), + // with the understanding that doing so may be inefficient. + // * we could delegate storage of the "max transitive source version so far" to one of the + // objects whose lifetime extends across all of a node's reevaluations within a single + // Skyframe request, such as the SkyKeyComputeState cache. + throw new UnsupportedOperationException(); + } + + @Override + void ensurePreviouslyRequestedDepsFetched() + throws UndonePreviouslyRequestedDeps, InterruptedException { + SkyFunctionEnvironment env = this; + ImmutableList keysToFetch = + ImmutableList.copyOf( + Sets.difference( + previouslyRequestedDepsSet, env.previouslyRequestedDepsValues.keySet())); + NodeBatch batch = + env.evaluatorContext.getGraph().getBatch(env.skyKey, Reason.PREFETCH, keysToFetch); + ImmutableList.Builder missingRequestedDeps = null; + for (SkyKey depKey : keysToFetch) { + NodeEntry entry = batch.get(depKey); + if (entry == null) { + if (missingRequestedDeps == null) { + missingRequestedDeps = ImmutableList.builder(); + } + missingRequestedDeps.add(depKey); + continue; + } + SkyValue valueMaybeWithMetadata = entry.getValueMaybeWithMetadata(); + boolean depDone = valueMaybeWithMetadata != null; + if (!depDone) { + // A previously requested dep may have transitioned from done to dirty between when the + // node was read during a previous attempt to build this node and now. Notify the graph + // inconsistency receiver so that we can crash if that's unexpected. + env.evaluatorContext + .getGraphInconsistencyReceiver() + .noteInconsistencyAndMaybeThrow( + env.skyKey, + ImmutableList.of(depKey), + Inconsistency.BUILDING_PARENT_FOUND_UNDONE_CHILD); + throw new UndonePreviouslyRequestedDeps(ImmutableList.of(depKey)); + } + env.previouslyRequestedDepsValues.put(depKey, valueMaybeWithMetadata); + maybeUpdateMaxTransitiveSourceVersion(entry); + } + + if (missingRequestedDeps != null) { + // Notify `GraphInconsistencyReceiver` when there are some dependencies missing from the + // graph to check whether this is expected. + ImmutableList allMissingDeps = missingRequestedDeps.build(); + env.evaluatorContext + .getGraphInconsistencyReceiver() + .noteInconsistencyAndMaybeThrow( + env.skyKey, allMissingDeps, Inconsistency.ALREADY_DECLARED_CHILD_MISSING); + throw new UndonePreviouslyRequestedDeps(ImmutableList.copyOf(allMissingDeps)); + } + } + + @Override + boolean wasNewlyRequestedDepNullForPartialReevaluation(SkyKey newlyRequestedDep) { + SkyFunctionEnvironment env = this; + return env.newlyRequestedDepsValues.get(newlyRequestedDep) == NULL_MARKER; + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java index 2595ee6e097dda..f79ed5f87534ab 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/GenQueryIntegrationTest.java @@ -83,7 +83,7 @@ public void testDeterministicGraphless() throws Exception { } @Test - public void testiDuplicateName() throws Exception { + public void testDuplicateName() throws Exception { write("one/BUILD", "sh_library(name='foo')"); write("two/BUILD", "sh_library(name='foo')"); write( diff --git a/src/test/java/com/google/devtools/build/skyframe/BUILD b/src/test/java/com/google/devtools/build/skyframe/BUILD index b0e8024ffd4da8..0081bdced59e75 100644 --- a/src/test/java/com/google/devtools/build/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/skyframe/BUILD @@ -70,6 +70,7 @@ java_library( "//src/test/java/com/google/devtools/build/lib/testutil:TestThread", "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:auto_value", + "//third_party:caffeine", "//third_party:guava", "//third_party:guava-testlib", "//third_party:jsr305", diff --git a/src/test/java/com/google/devtools/build/skyframe/NodeEntryVisitorTest.java b/src/test/java/com/google/devtools/build/skyframe/NodeEntryVisitorTest.java index 4b0c1aea03522a..e4013a62f88811 100644 --- a/src/test/java/com/google/devtools/build/skyframe/NodeEntryVisitorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/NodeEntryVisitorTest.java @@ -18,38 +18,47 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import com.github.benmanes.caffeine.cache.Cache; import com.google.devtools.build.lib.concurrent.MultiThreadPoolsQuiescingExecutor; import com.google.devtools.build.lib.concurrent.MultiThreadPoolsQuiescingExecutor.ThreadPoolType; import com.google.devtools.build.skyframe.ParallelEvaluatorContext.RunnableMaker; +import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; /** Tests for {@link NodeEntryVisitor}. */ @RunWith(JUnit4.class) public class NodeEntryVisitorTest { + @Rule public final MockitoRule mockito = MockitoJUnit.rule(); + + @Mock private MultiThreadPoolsQuiescingExecutor executor; + @Mock private DirtyTrackingProgressReceiver receiver; + @Mock private RunnableMaker runnableMaker; + @Mock private Cache stateCache; + @Test public void enqueueEvaluation_multiThreadPoolsQuiescingExecutor_nonCPUHeavyKey() { - MultiThreadPoolsQuiescingExecutor executor = mock(MultiThreadPoolsQuiescingExecutor.class); NodeEntryVisitor nodeEntryVisitor = - new NodeEntryVisitor( - executor, mock(DirtyTrackingProgressReceiver.class), mock(RunnableMaker.class)); + new NodeEntryVisitor(executor, receiver, runnableMaker, stateCache); SkyKey nonCPUHeavyKey = mock(SkyKey.class); - nodeEntryVisitor.enqueueEvaluation(nonCPUHeavyKey, Integer.MAX_VALUE); + nodeEntryVisitor.enqueueEvaluation(nonCPUHeavyKey, Integer.MAX_VALUE, null); verify(executor).execute(any(), eq(ThreadPoolType.REGULAR)); } @Test public void enqueueEvaluation_multiThreadPoolsQuiescingExecutor_cpuHeavyKey() { - MultiThreadPoolsQuiescingExecutor executor = mock(MultiThreadPoolsQuiescingExecutor.class); NodeEntryVisitor nodeEntryVisitor = - new NodeEntryVisitor( - executor, mock(DirtyTrackingProgressReceiver.class), mock(RunnableMaker.class)); + new NodeEntryVisitor(executor, receiver, runnableMaker, stateCache); CPUHeavySkyKey cpuHeavyKey = mock(CPUHeavySkyKey.class); - nodeEntryVisitor.enqueueEvaluation(cpuHeavyKey, Integer.MAX_VALUE); + nodeEntryVisitor.enqueueEvaluation(cpuHeavyKey, Integer.MAX_VALUE, null); verify(executor).execute(any(), eq(ThreadPoolType.CPU_HEAVY)); } diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java index 34d802af4c752c..e66bfb515c9cc3 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java @@ -66,6 +66,8 @@ import com.google.devtools.build.skyframe.GraphTester.StringValue; import com.google.devtools.build.skyframe.NotifyingHelper.EventType; import com.google.devtools.build.skyframe.NotifyingHelper.Order; +import com.google.devtools.build.skyframe.PartialReevaluationMailbox.Kind; +import com.google.devtools.build.skyframe.PartialReevaluationMailbox.Mail; import com.google.devtools.build.skyframe.SkyFunction.Environment.ClassToInstanceMapSkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; @@ -206,7 +208,8 @@ public void enqueueDoneFuture() throws Exception { } }); graph = new InMemoryGraphImpl(); - EvaluationResult result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey)); + EvaluationResult result = + eval(/* keepGoing= */ false, ImmutableList.of(parentKey)); assertThat(result.hasError()).isFalse(); assertThat(result.get(parentKey)).isEqualTo(new StringValue("good")); } @@ -253,7 +256,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) { } }) .transform(new InMemoryGraphImpl()); - EvaluationResult result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey)); + EvaluationResult result = + eval(/* keepGoing= */ false, ImmutableList.of(parentKey)); assertThat(result.hasError()).isFalse(); assertThat(result.get(parentKey)).isEqualTo(new StringValue("Caught!")); } @@ -308,7 +312,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept } }) .transform(new InMemoryGraphImpl()); - EvaluationResult result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey)); + EvaluationResult result = + eval(/* keepGoing= */ false, ImmutableList.of(parentKey)); assertThat(result.hasError()).isFalse(); assertThat(result.get(parentKey)).isEqualTo(new StringValue("All done!")); } @@ -428,7 +433,7 @@ protected InMemoryNodeEntry newNodeEntry(SkyKey key) { new TestThread( () -> assertThrows( - InterruptedException.class, () -> eval(/*keepGoing=*/ true, keyA, keyB))); + InterruptedException.class, () -> eval(/* keepGoing= */ true, keyA, keyB))); // Then when we start that thread, evalThread.start(); @@ -477,7 +482,7 @@ public void runPartialResultOnInterruption(@TestParameter boolean buildFastFirst ImmutableList.of(leafKey))); tester.set(leafKey, new StringValue("leaf")); if (buildFastFirst) { - eval(/*keepGoing=*/ false, fastKey); + eval(/* keepGoing= */ false, fastKey); } final Set receivedValues = Sets.newConcurrentHashSet(); revalidationReceiver = @@ -497,7 +502,8 @@ public void evaluated( new TestThread( () -> assertThrows( - InterruptedException.class, () -> eval(/*keepGoing=*/ true, waitKey, fastKey))); + InterruptedException.class, + () -> eval(/* keepGoing= */ true, waitKey, fastKey))); evalThread.start(); assertThat(allValuesReady.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)).isTrue(); evalThread.interrupt(); @@ -773,7 +779,7 @@ public void fullEventStorage_unstoredEvent_reportedImmediately_notReplayed( }); ParallelEvaluator evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.FULL_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.FULL_STORAGE); evaluator.eval(ImmutableList.of(key)); assertThat(evaluated.get()).isTrue(); assertThat(eventType.getResults(reportedEvents)).containsExactly(unstoredEvent); @@ -783,7 +789,7 @@ public void fullEventStorage_unstoredEvent_reportedImmediately_notReplayed( evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.FULL_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.FULL_STORAGE); evaluator.eval(ImmutableList.of(key)); assertThat(evaluated.get()).isFalse(); assertThat(eventType.getResults(reportedEvents)).isEmpty(); @@ -838,7 +844,7 @@ public String extractTag(SkyKey skyKey) { tester.getOrCreate(bottom).setConstantValue(new StringValue("depValue")); ParallelEvaluator evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.FULL_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.FULL_STORAGE); evaluator.eval(ImmutableList.of(top)); assertThat(evaluatedMid.get()).isTrue(); assertThat(eventType.getResults(reportedEvents)).containsExactly(taggedEvent); @@ -848,7 +854,7 @@ public String extractTag(SkyKey skyKey) { evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.FULL_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.FULL_STORAGE); evaluator.eval(ImmutableList.of(top)); assertThat(evaluatedMid.get()).isFalse(); assertThat(eventType.getResults(reportedEvents)).containsExactly(taggedEvent); @@ -873,7 +879,7 @@ public void noEventStorage_unstoredEvent_reportedImmediately_notReplayed( }); ParallelEvaluator evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.NO_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.NO_STORAGE); evaluator.eval(ImmutableList.of(key)); assertThat(evaluated.get()).isTrue(); assertThat(eventType.getResults(reportedEvents)).containsExactly(unstoredEvent); @@ -883,7 +889,7 @@ public void noEventStorage_unstoredEvent_reportedImmediately_notReplayed( evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.NO_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.NO_STORAGE); evaluator.eval(ImmutableList.of(key)); assertThat(evaluated.get()).isFalse(); assertThat(eventType.getResults(reportedEvents)).isEmpty(); @@ -938,7 +944,7 @@ public String extractTag(SkyKey skyKey) { tester.getOrCreate(bottom).setConstantValue(new StringValue("depValue")); ParallelEvaluator evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.NO_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.NO_STORAGE); evaluator.eval(ImmutableList.of(top)); assertThat(evaluatedMid.get()).isTrue(); assertThat(eventType.getResults(reportedEvents)).containsExactly(taggedEvent); @@ -948,7 +954,7 @@ public String extractTag(SkyKey skyKey) { evaluator = makeEvaluator( - graph, tester.getSkyFunctionMap(), /*keepGoing=*/ false, EventFilter.NO_STORAGE); + graph, tester.getSkyFunctionMap(), /* keepGoing= */ false, EventFilter.NO_STORAGE); evaluator.eval(ImmutableList.of(top)); assertThat(evaluatedMid.get()).isFalse(); assertThat(eventType.getResults(reportedEvents)).isEmpty(); @@ -1063,7 +1069,7 @@ public boolean isCatastrophic() { }); EvaluationResult result = - eval(/*keepGoing=*/ true, ImmutableList.of(catastropheKey)); + eval(/* keepGoing= */ true, ImmutableList.of(catastropheKey)); assertThat(result.getCatastrophe()).isEqualTo(catastrophe); } @@ -1100,13 +1106,13 @@ public SkyValue compute(SkyKey skyKey, Environment env) env.getValueOrThrow(catastropheKey, SomeErrorException.class); } catch (SomeErrorException e) { throw new SkyFunctionException( - new SomeErrorException("We got: " + e.getMessage()), Transience.PERSISTENT) { - }; + new SomeErrorException("We got: " + e.getMessage()), + Transience.PERSISTENT) {}; } return null; } }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThat(result.getError(topKey).getException()).isInstanceOf(SomeErrorException.class); assertThat(result.getError(topKey).getException()).hasMessageThat().isEqualTo("We got: bad"); @@ -1160,7 +1166,7 @@ public boolean isCatastrophic() { }; } }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThatEvaluationResult(result).hasError(); assertThatEvaluationResult(result) .hasErrorEntryForKeyThat(topKey) @@ -1176,7 +1182,7 @@ public void parentFailureDoesntAffectChild() throws Exception { SkyKey childKey = GraphTester.toSkyKey("child"); set("child", "onions"); tester.getOrCreate(parentKey).addDependency(childKey).setComputedValue(CONCATENATE); - EvaluationResult result = eval(/*keepGoing=*/ true, parentKey, childKey); + EvaluationResult result = eval(/* keepGoing= */ true, parentKey, childKey); // Child is guaranteed to complete successfully before parent can run (and fail), // since parent depends on it. assertThatEvaluationResult(result).hasEntryThat(childKey).isEqualTo(new StringValue("onions")); @@ -1219,12 +1225,12 @@ public void valueNotUsedInFailFastErrorRecovery() throws Exception { .setComputedValue(CONCATENATE); tester.getOrCreate(badKey).setHasError(true); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(recoveryKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(recoveryKey)); assertThat(result.errorMap()).isEmpty(); assertThatEvaluationResult(result).hasNoError(); assertThat(result.get(recoveryKey)).isEqualTo(new StringValue("i recovered")); - result = eval(/*keepGoing=*/ false, ImmutableList.of(topKey)); + result = eval(/* keepGoing= */ false, ImmutableList.of(topKey)); assertThatEvaluationResult(result).hasError(); assertThat(result.keyNames()).isEmpty(); assertThat(result.errorMap()).hasSize(1); @@ -1287,10 +1293,15 @@ public void errorBubblesToParentsOfTopLevelValue() throws Exception { .getOrCreate(errorKey) .setBuilder( new ChainedFunction( - null, /*waitToFinish=*/ latch, null, false, /*value=*/ null, ImmutableList.of())); + null, + /* waitToFinish= */ latch, + null, + false, + /* value= */ null, + ImmutableList.of())); tester.getOrCreate(parentKey).addDependency(errorKey).setComputedValue(CONCATENATE); EvaluationResult result = - eval(/*keepGoing=*/ false, ImmutableList.of(parentKey, errorKey)); + eval(/* keepGoing= */ false, ImmutableList.of(parentKey, errorKey)); assertWithMessage(result.toString()).that(result.errorMap().size()).isEqualTo(2); } @@ -1324,9 +1335,9 @@ public void twoErrors() throws Exception { new ChainedFunction( firstStart, secondStart, - /*notifyFinish=*/ null, - /*waitForException=*/ false, - /*value=*/ null, + /* notifyFinish= */ null, + /* waitForException= */ false, + /* value= */ null, ImmutableList.of())); tester .getOrCreate(secondError) @@ -1334,11 +1345,11 @@ public void twoErrors() throws Exception { new ChainedFunction( secondStart, firstStart, - /*notifyFinish=*/ null, - /*waitForException=*/ false, - /*value=*/ null, + /* notifyFinish= */ null, + /* waitForException= */ false, + /* value= */ null, ImmutableList.of())); - EvaluationResult result = eval(/*keepGoing=*/ false, firstError, secondError); + EvaluationResult result = eval(/* keepGoing= */ false, firstError, secondError); assertWithMessage(result.toString()).that(result.hasError()).isTrue(); // With keepGoing=false, the eval call will terminate with exactly one error (the first one // thrown). But the first one thrown here is non-deterministic since we synchronize the @@ -1530,7 +1541,7 @@ public void selfEdgeWithExtraChildrenUnderCycle() throws Exception { tester.getOrCreate(aKey).addDependency(zKey); tester.getOrCreate(zKey).addDependency(cKey).addDependency(zKey); tester.getOrCreate(cKey).addDependency(aKey); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(aKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(aKey)); assertThat(result.get(aKey)).isNull(); ErrorInfo errorInfo = result.getError(aKey); CycleInfo cycleInfo = Iterables.getOnlyElement(errorInfo.getCycleInfo()); @@ -1550,7 +1561,7 @@ public void cycleWithExtraChildrenUnderCycle() throws Exception { tester.getOrCreate(bKey).addDependency(cKey).addDependency(dKey); tester.getOrCreate(cKey).addDependency(aKey); tester.getOrCreate(dKey).addDependency(bKey); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(aKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(aKey)); assertThat(result.get(aKey)).isNull(); ErrorInfo errorInfo = result.getError(aKey); CycleInfo cycleInfo = Iterables.getOnlyElement(errorInfo.getCycleInfo()); @@ -1568,7 +1579,7 @@ public void cycleAboveIndependentCycle() throws Exception { tester.getOrCreate(aKey).addDependency(bKey); tester.getOrCreate(bKey).addDependency(cKey); tester.getOrCreate(cKey).addDependency(aKey).addDependency(bKey); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(aKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(aKey)); assertThat(result.get(aKey)).isNull(); assertThat(result.getError(aKey).getCycleInfo()) .containsExactly( @@ -1585,7 +1596,7 @@ public void valueAboveCycleAndExceptionReportsException() throws Exception { tester.getOrCreate(aKey).addDependency(bKey).addDependency(errorKey); tester.getOrCreate(bKey).addDependency(bKey); tester.getOrCreate(errorKey).setHasError(true); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(aKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(aKey)); assertThat(result.get(aKey)).isNull(); assertThat(result.getError(aKey).getException()).isNotNull(); CycleInfo cycleInfo = Iterables.getOnlyElement(result.getError(aKey).getCycleInfo()); @@ -1620,9 +1631,9 @@ public void manyCycles() throws Exception { tester.getOrCreate(topKey).addDependency(dep); tester.getOrCreate(dep).addDependency(dep); } - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThat(result.get(topKey)).isNull(); - assertManyCycles(result.getError(topKey), topKey, /*selfEdge=*/ false); + assertManyCycles(result.getError(topKey), topKey, /* selfEdge= */ false); } /** @@ -1642,7 +1653,7 @@ public void manyPathsToCycle() throws Exception { tester.getOrCreate(midKey).addDependency(dep); tester.getOrCreate(dep).addDependency(cycleKey); } - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThat(result.get(topKey)).isNull(); CycleInfo cycleInfo = Iterables.getOnlyElement(result.getError(topKey).getCycleInfo()); assertThat(cycleInfo.getCycle()).hasSize(1); @@ -1697,7 +1708,7 @@ public void manyUnprocessedValuesInCycle() throws Exception { // All the deps will be cleared from lastSelf. tester.getOrCreate(lastSelfKey).addDependency(lastSelfKey); EvaluationResult result = - eval(/*keepGoing=*/ true, ImmutableList.of(lastSelfKey, firstSelfKey, midSelfKey)); + eval(/* keepGoing= */ true, ImmutableList.of(lastSelfKey, firstSelfKey, midSelfKey)); assertWithMessage(result.toString()).that(result.keyNames()).isEmpty(); assertThat(result.errorMap().keySet()).containsExactly(lastSelfKey, firstSelfKey, midSelfKey); @@ -1712,10 +1723,10 @@ public void manyUnprocessedValuesInCycle() throws Exception { // Check firstSelfKey. It should not have discovered its own self-edge, because there were too // many other values before it in the queue. - assertManyCycles(result.getError(firstSelfKey), firstSelfKey, /*selfEdge=*/ false); + assertManyCycles(result.getError(firstSelfKey), firstSelfKey, /* selfEdge= */ false); // Check midSelfKey. It should have discovered its own self-edge. - assertManyCycles(result.getError(midSelfKey), midSelfKey, /*selfEdge=*/ true); + assertManyCycles(result.getError(midSelfKey), midSelfKey, /* selfEdge= */ true); } @Test @@ -1744,10 +1755,10 @@ public void continueWithErrorDep() throws Exception { .addErrorDependency(errorKey, new StringValue("recovered")) .setComputedValue(CONCATENATE) .addDependency("after"); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parentKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(parentKey)); assertThat(result.errorMap()).isEmpty(); assertThat(result.get(parentKey).getValue()).isEqualTo("recoveredafter"); - result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey)); + result = eval(/* keepGoing= */ false, ImmutableList.of(parentKey)); assertThatEvaluationResult(result).hasSingletonErrorThat(parentKey); } @@ -1780,7 +1791,7 @@ public void transformErrorDepOneLevelDownKeepGoing() throws Exception { .addDependency(parentErrorKey) .addDependency("after") .setComputedValue(CONCATENATE); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThat(ImmutableList.copyOf(result.keyNames())).containsExactly("top"); assertThat(result.get(topKey).getValue()).isEqualTo("parent valueafter"); assertThat(result.errorMap()).isEmpty(); @@ -1801,7 +1812,7 @@ public void transformErrorDepOneLevelDownNoKeepGoing() throws Exception { .addDependency(parentErrorKey) .addDependency("after") .setComputedValue(CONCATENATE); - EvaluationResult result = eval(/*keepGoing=*/ false, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ false, ImmutableList.of(topKey)); assertThatEvaluationResult(result).hasSingletonErrorThat(topKey); } @@ -1810,7 +1821,7 @@ public void errorDepDoesntStopOtherDep() throws Exception { graph = new InMemoryGraphImpl(); final SkyKey errorKey = GraphTester.toSkyKey("error"); tester.getOrCreate(errorKey).setHasError(true); - EvaluationResult result1 = eval(/*keepGoing=*/ true, ImmutableList.of(errorKey)); + EvaluationResult result1 = eval(/* keepGoing= */ true, ImmutableList.of(errorKey)); assertThatEvaluationResult(result1).hasError(); assertThatEvaluationResult(result1) .hasErrorEntryForKeyThat(errorKey) @@ -1845,7 +1856,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } }); - EvaluationResult result2 = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result2 = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThatEvaluationResult(result2).hasError(); assertThatEvaluationResult(result2) .hasErrorEntryForKeyThat(topKey) @@ -1875,7 +1886,7 @@ public void cycleWithMultipleUnfinishedChildren() throws Exception { tester.getOrCreate(cycleKey).addDependency(midKey); tester.getOrCreate(selfEdge1).addDependency(selfEdge1); tester.getOrCreate(selfEdge2).addDependency(selfEdge2); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableSet.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableSet.of(topKey)); assertThat(result.errorMap().keySet()).containsExactly(topKey); Iterable cycleInfos = result.getError(topKey).getCycleInfo(); CycleInfo cycleInfo = Iterables.getOnlyElement(cycleInfos); @@ -1913,7 +1924,7 @@ public void cycleAndErrorInBubbleUp(@TestParameter boolean keepGoing) throws Exc .getOrCreate(errorKey) .setBuilder( new ChainedFunction( - null, cycleFinish, null, /*waitForException=*/ false, null, ImmutableSet.of())); + null, cycleFinish, null, /* waitForException= */ false, null, ImmutableSet.of())); EvaluationResult result = eval(keepGoing, ImmutableSet.of(topKey)); assertThatEvaluationResult(result) @@ -1956,7 +1967,7 @@ public void cycleAndErrorAndOtherInBubbleUp() throws Exception { topStartAndCycleFinish, new CountDownLatch(0), null, - /*waitForException=*/ true, + /* waitForException= */ true, new StringValue("never returned"), ImmutableSet.of(GraphTester.toSkyKey("dep that never builds")))); @@ -1967,7 +1978,7 @@ public void cycleAndErrorAndOtherInBubbleUp() throws Exception { null, null, topStartAndCycleFinish, - /*waitForException=*/ false, + /* waitForException= */ false, new StringValue(""), ImmutableSet.of(midKey))); // error waits until otherTop starts and cycle finishes, to make sure otherTop will request @@ -1979,11 +1990,11 @@ public void cycleAndErrorAndOtherInBubbleUp() throws Exception { null, topStartAndCycleFinish, null, - /*waitForException=*/ false, + /* waitForException= */ false, null, ImmutableSet.of())); EvaluationResult result = - eval(/*keepGoing=*/ false, ImmutableSet.of(topKey, otherTop)); + eval(/* keepGoing= */ false, ImmutableSet.of(topKey, otherTop)); assertThat(result.errorMap().keySet()).containsExactly(topKey); Iterable cycleInfos = result.getError(topKey).getCycleInfo(); assertThat(cycleInfos).isNotEmpty(); @@ -2023,7 +2034,7 @@ public void cycleAndErrorAndError(@TestParameter boolean keepGoing) throws Excep topStartAndCycleFinish, new CountDownLatch(0), null, - /*waitForException=*/ !keepGoing, + /* waitForException= */ !keepGoing, null, ImmutableSet.of())); // error waits until otherTop starts and cycle finishes, to make sure otherTop will request @@ -2035,7 +2046,7 @@ public void cycleAndErrorAndError(@TestParameter boolean keepGoing) throws Excep null, topStartAndCycleFinish, null, - /*waitForException=*/ false, + /* waitForException= */ false, null, ImmutableSet.of())); tester @@ -2045,7 +2056,7 @@ public void cycleAndErrorAndError(@TestParameter boolean keepGoing) throws Excep null, null, topStartAndCycleFinish, - /*waitForException=*/ false, + /* waitForException= */ false, new StringValue(""), ImmutableSet.of(midKey))); EvaluationResult result = eval(keepGoing, ImmutableSet.of(topKey, otherTop)); @@ -2212,8 +2223,8 @@ public void sameDepInTwoGroups(@TestParameter boolean sameFirst, @TestParameter } return new StringValue("top"); }); - eval(/*keepGoing=*/ false, topKey); - assertThat(eval(/*keepGoing=*/ false, topKey)).isEqualTo(new StringValue("top")); + eval(/* keepGoing= */ false, topKey); + assertThat(eval(/* keepGoing= */ false, topKey)).isEqualTo(new StringValue("top")); } @Test @@ -2317,7 +2328,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(topKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(topKey)); assertThatEvaluationResult(result).hasError(); assertThatEvaluationResult(result) @@ -2356,7 +2367,7 @@ public void getValuesAndExceptionsWithErrors() throws Exception { assertThat(env.valuesMissing()).isTrue(); return null; }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parentKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(parentKey)); assertThatEvaluationResult(result).hasError(); assertThatEvaluationResult(result) .hasErrorEntryForKeyThat(parentKey) @@ -2388,13 +2399,13 @@ public void declareDependenciesAndCheckIfValuesMissing() throws Exception { env, ImmutableList.of(childKey), SomeOtherErrorException.class, - /*exceptionClass2=*/ null, + /* exceptionClass2= */ null, mockReporter); numComputes.incrementAndGet(); assertThat(valuesMissing).isTrue(); return null; }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parentKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(parentKey)); verify(mockReporter) .logUnexpected("Value for: '%s' was missing, this should never happen", childKey); verifyNoMoreInteractions(mockReporter); @@ -2438,7 +2449,7 @@ public void declareDependenciesAndCheckIfNotValuesMissing() throws Exception { } return null; }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parentKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(parentKey)); assertThatEvaluationResult(result).hasError(); assertThatEvaluationResult(result) .hasErrorEntryForKeyThat(parentKey) @@ -2481,7 +2492,7 @@ public void validateExceptionTypeInDifferentPosition( assertThat(env.valuesMissing()).isFalse(); throw new GenericFunctionException(parentExn, Transience.PERSISTENT); }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parentKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(parentKey)); assertThat(result.hasError()).isTrue(); assertThat(result.getError().getException()).isEqualTo(parentExn); } @@ -2508,7 +2519,7 @@ public void validateExceptionTypeWithDifferentException( assertThat(env.valuesMissing()).isFalse(); throw new GenericFunctionException(parentExn, Transience.PERSISTENT); }); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parentKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(parentKey)); assertThat(result.hasError()).isTrue(); assertThat(result.getError().getException()).isEqualTo(parentExn); } @@ -2639,7 +2650,7 @@ public void runDepOnErrorHaltsNoKeepGoingBuildEagerly( graph = new InMemoryGraphImpl(); SkyKey parentKey = GraphTester.toSkyKey("parent"); final SkyKey childKey = GraphTester.toSkyKey("child"); - tester.getOrCreate(childKey).setHasError(/*hasError=*/ true); + tester.getOrCreate(childKey).setHasError(/* hasError= */ true); // The parent should be built exactly twice: once during normal evaluation and once // during error bubbling. final AtomicInteger numParentInvocations = new AtomicInteger(0); @@ -2688,7 +2699,8 @@ public void runDepOnErrorHaltsNoKeepGoingBuildEagerly( // Ensure that the child is already in the graph. evalValueInError(childKey); } - EvaluationResult result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey)); + EvaluationResult result = + eval(/* keepGoing= */ false, ImmutableList.of(parentKey)); assertThat(numParentInvocations.get()).isEqualTo(2); assertThatEvaluationResult(result).hasErrorEntryForKeyThat(parentKey); } @@ -2778,7 +2790,7 @@ public void raceConditionWithNoKeepGoingErrors_FutureError() throws Exception { } }); EvaluationResult result = - eval(/*keepGoing=*/ false, ImmutableList.of(otherParentKey, errorParentKey)); + eval(/* keepGoing= */ false, ImmutableList.of(otherParentKey, errorParentKey)); assertThat(result.hasError()).isTrue(); assertThatEvaluationResult(result).hasErrorEntryForKeyThat(errorParentKey); } @@ -2799,9 +2811,10 @@ public void cachedErrorsFromKeepGoingUsedOnNoKeepGoing() throws Exception { .addDependency(errorKey) .setConstantValue(new StringValue("parent2")); tester.getOrCreate(errorKey).setHasError(true); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(parent1Key)); + EvaluationResult result = + eval(/* keepGoing= */ true, ImmutableList.of(parent1Key)); assertThatEvaluationResult(result).hasSingletonErrorThat(parent1Key); - result = eval(/*keepGoing=*/ false, ImmutableList.of(parent2Key)); + result = eval(/* keepGoing= */ false, ImmutableList.of(parent2Key)); assertThatEvaluationResult(result).hasSingletonErrorThat(parent2Key); } @@ -2811,7 +2824,7 @@ public void cachedTopLevelErrorsShouldHaltNoKeepGoingBuildEarly() throws Excepti tester = new GraphTester(); SkyKey errorKey = GraphTester.toSkyKey("error"); tester.getOrCreate(errorKey).setHasError(true); - EvaluationResult result = eval(/*keepGoing=*/ true, ImmutableList.of(errorKey)); + EvaluationResult result = eval(/* keepGoing= */ true, ImmutableList.of(errorKey)); assertThatEvaluationResult(result).hasSingletonErrorThat(errorKey); SkyKey rogueKey = GraphTester.toSkyKey("rogue"); tester @@ -2823,7 +2836,7 @@ public void cachedTopLevelErrorsShouldHaltNoKeepGoingBuildEarly() throws Excepti fail("eval call should have already terminated"); return null; }); - result = eval(/*keepGoing=*/ false, ImmutableList.of(errorKey, rogueKey)); + result = eval(/* keepGoing= */ false, ImmutableList.of(errorKey, rogueKey)); assertThatEvaluationResult(result).hasErrorMapThat().hasSize(1); assertThatEvaluationResult(result).hasErrorEntryForKeyThat(errorKey); assertThat(result.errorMap()).doesNotContainKey(rogueKey); @@ -2868,7 +2881,8 @@ public void declaresDifferentDepsAfterRestart() throws Exception { throw new IllegalStateException(); } }); - EvaluationResult result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey)); + EvaluationResult result = + eval(/* keepGoing= */ false, ImmutableList.of(parentKey)); assertThatEvaluationResult(result).hasNoError(); assertThatEvaluationResult(result) .hasEntryThat(parentKey) @@ -2910,7 +2924,7 @@ public void runUnhandledTransitiveErrors( return env.getValue(childKey); } }); - tester.getOrCreate(childKey).setHasError(/*hasError=*/ true); + tester.getOrCreate(childKey).setHasError(/* hasError= */ true); EvaluationResult result = eval(keepGoing, ImmutableList.of(grandparentKey)); assertThat(errorPropagated.get()).isTrue(); assertThatEvaluationResult(result).hasSingletonErrorThat(grandparentKey); @@ -3047,7 +3061,7 @@ class State implements SkyKeyComputeState { tester.putSkyFunction(SkyKeyForSkyKeyComputeStateTests.FUNCTION_NAME, skyFunctionForTest); graph = new InMemoryGraphImpl(); // Then, when we evaluate key1, - SkyValue resultValue = eval(/*keepGoing=*/ true, key1); + SkyValue resultValue = eval(/* keepGoing= */ true, key1); // It successfully produces the value we expect, confirming all our other expectations about // the compute states were correct. assertThat(resultValue).isEqualTo(new StringValue("value1")); @@ -3126,7 +3140,7 @@ class State implements SkyKeyComputeState {} tester.putSkyFunction(SkyKeyForSkyKeyComputeStateTests.FUNCTION_NAME, skyFunctionForTest); graph = new InMemoryGraphImpl(); // Then, when we do a nokeep_going evaluation of key1 and key3 in parallel, - assertThatEvaluationResult(eval(/*keepGoing=*/ false, key1, key3)) + assertThatEvaluationResult(eval(/* keepGoing= */ false, key1, key3)) // The evaluation fails (as expected), .hasErrorEntryForKeyThat(key1) .hasExceptionThat() @@ -3298,7 +3312,7 @@ class StateB implements SkyKeyComputeState { }; tester.putSkyFunction(SkyKeyForSkyKeyComputeStateTests.FUNCTION_NAME, skyFunctionForTest); graph = new InMemoryGraphImpl(); - SkyValue resultValue = eval(/*keepGoing=*/ true, key1); + SkyValue resultValue = eval(/* keepGoing= */ true, key1); assertThat(resultValue).isEqualTo(new StringValue("value")); } @@ -3408,31 +3422,44 @@ public void partialReevaluationOneButNotAllDeps( CountDownLatch key1ObservesTheValueOfKey2 = new CountDownLatch(1); SkyFunction f = (skyKey, env) -> { + PartialReevaluationMailbox mailbox = + PartialReevaluationMailbox.from( + env.getState(ClassToInstanceMapSkyKeyComputeState::new)); + Mail mail = mailbox.getMail(); + assertThat(mailbox.getMail().kind()).isEqualTo(Kind.EMPTY); + if (skyKey.equals(key1)) { int c = key1EvaluationCount.incrementAndGet(); SkyframeLookupResult result = DelegatingSkyframeLookupResult.fromRequestBatches( requestBatches, env, ImmutableList.of(key2, key3)); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isNull(); assertThat(result.get(key3)).isNull(); return null; } if (c == 2) { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key2); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isNull(); key1ObservesTheValueOfKey2.countDown(); return null; } assertThat(c).isEqualTo(3); + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key3); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isEqualTo(StringValue.of("val3")); return StringValue.of("val1"); } if (skyKey.equals(key2)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); return StringValue.of("val2"); } assertThat(skyKey).isEqualTo(key3); + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheValueOfKey2.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3500,6 +3527,10 @@ public void partialReevaluationOneDuringAReevaluation( CountDownLatch key3SignaledItsParents = new CountDownLatch(1); SkyFunction f = (skyKey, env) -> { + Mail mail = + PartialReevaluationMailbox.from( + env.getState(ClassToInstanceMapSkyKeyComputeState::new)) + .getMail(); if (skyKey.equals(key1)) { assertThat(key1EvaluationsInflight.incrementAndGet()).isEqualTo(1); try { @@ -3508,12 +3539,15 @@ public void partialReevaluationOneDuringAReevaluation( DelegatingSkyframeLookupResult.fromRequestBatches( requestBatches, env, ImmutableList.of(key2, key3, key4)); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isNull(); assertThat(result.get(key3)).isNull(); assertThat(result.get(key4)).isNull(); return null; } if (c == 2) { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key2); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isNull(); assertThat(result.get(key4)).isNull(); @@ -3525,6 +3559,8 @@ public void partialReevaluationOneDuringAReevaluation( return null; } if (c == 3) { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key3); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isEqualTo(StringValue.of("val3")); assertThat(result.get(key4)).isNull(); @@ -3532,6 +3568,8 @@ public void partialReevaluationOneDuringAReevaluation( return null; } assertThat(c).isEqualTo(4); + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key4); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isEqualTo(StringValue.of("val3")); assertThat(result.get(key4)).isEqualTo(StringValue.of("val4")); @@ -3542,10 +3580,12 @@ public void partialReevaluationOneDuringAReevaluation( } if (skyKey.equals(key2)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); return StringValue.of("val2"); } if (skyKey.equals(key3)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheValueOfKey2.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3558,6 +3598,7 @@ public void partialReevaluationOneDuringAReevaluation( } if (skyKey.equals(key4)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheValueOfKey3.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3568,12 +3609,15 @@ public void partialReevaluationOneDuringAReevaluation( assertThat(skyKey).isEqualTo(key5); int c = key5EvaluationCount.incrementAndGet(); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); SkyValue value = env.getValue(key3); assertThat(value).isNull(); key5ObservesTheAbsenceOfKey3.countDown(); return null; } assertThat(c).isEqualTo(2); + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key3); SkyValue value = env.getValue(key3); assertThat(value).isEqualTo(StringValue.of("val3")); key3SignaledItsParents.countDown(); @@ -3624,6 +3668,10 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke CountDownLatch key3SignaledItsParents = new CountDownLatch(1); SkyFunction f = (skyKey, env) -> { + Mail mail = + PartialReevaluationMailbox.from( + env.getState(ClassToInstanceMapSkyKeyComputeState::new)) + .getMail(); if (skyKey.equals(key1)) { assertThat(key1EvaluationsInflight.incrementAndGet()).isEqualTo(1); try { @@ -3631,12 +3679,15 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke SkyframeLookupResult result = env.getValuesAndExceptions(ImmutableList.of(key2, key3, key4)); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isNull(); assertThat(result.get(key3)).isNull(); assertThat(result.get(key4)).isNull(); return null; } if (c == 2) { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key2); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isNull(); assertThat(result.get(key4)).isNull(); @@ -3649,6 +3700,8 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke "Error thrown during partial reevaluation (1)"); } if (c == 3) { + // The Skyframe stateCache invalidates its entry for a node when it throws an error: + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isEqualTo(StringValue.of("val3")); assertThat(result.get(key4)).isNull(); @@ -3657,6 +3710,7 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke "Error thrown during partial reevaluation (2)"); } assertThat(c).isEqualTo(4); + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isEqualTo(StringValue.of("val3")); assertThat(result.get(key4)).isEqualTo(StringValue.of("val4")); @@ -3667,10 +3721,12 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke } if (skyKey.equals(key2)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); return StringValue.of("val2"); } if (skyKey.equals(key3)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheValueOfKey2.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3683,6 +3739,7 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke } if (skyKey.equals(key4)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheValueOfKey3.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3693,12 +3750,15 @@ public void partialReevaluationErrorDuringReevaluation(@TestParameter boolean ke assertThat(skyKey).isEqualTo(key5); int c = key5EvaluationCount.incrementAndGet(); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); SkyValue value = env.getValue(key3); assertThat(value).isNull(); key5ObservesTheAbsenceOfKey3.countDown(); return null; } assertThat(c).isEqualTo(2); + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key3); SkyValue value = env.getValue(key3); assertThat(value).isEqualTo(StringValue.of("val3")); key3SignaledItsParents.countDown(); @@ -3751,16 +3811,26 @@ public void partialReevaluationOneErrorButNotAllDeps( CountDownLatch key1ObservesTheErrorOfKey2 = new CountDownLatch(1); SkyFunction f = (skyKey, env) -> { + Mail mail = + PartialReevaluationMailbox.from( + env.getState(ClassToInstanceMapSkyKeyComputeState::new)) + .getMail(); if (skyKey.equals(key1)) { int c = key1EvaluationCount.incrementAndGet(); SkyframeLookupResult result = env.getValuesAndExceptions(ImmutableList.of(key2, key3)); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isNull(); assertThat(result.get(key3)).isNull(); return null; } if (c == 2) { - if (!keepGoing) { + if (keepGoing) { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key2); + } else { + // The Skyframe stateCache invalidates everything when starting error bubbling: + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(env.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors()) .isTrue(); } @@ -3777,6 +3847,13 @@ public void partialReevaluationOneErrorButNotAllDeps( } } assertThat(c).isEqualTo(3); + if (enrichError) { + // The Skyframe stateCache invalidates its entry for a node when it throws an error: + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); + } else { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key3); + } assertThat(keepGoing).isTrue(); assertThrows( "key2", @@ -3790,9 +3867,11 @@ public void partialReevaluationOneErrorButNotAllDeps( } } if (skyKey.equals(key2)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); throw new SkyFunctionExceptionForTest("key2"); } assertThat(skyKey).isEqualTo(key3); + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheErrorOfKey2.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3860,6 +3939,10 @@ public void partialReevaluationErrorObservedDuringReevaluation() throws Interrup CountDownLatch key4WaitsUntilInterruptedByNoKeepGoingEvaluationShutdown = new CountDownLatch(1); SkyFunction f = (skyKey, env) -> { + Mail mail = + PartialReevaluationMailbox.from( + env.getState(ClassToInstanceMapSkyKeyComputeState::new)) + .getMail(); if (skyKey.equals(key1)) { assertThat(key1EvaluationsInflight.incrementAndGet()).isEqualTo(1); try { @@ -3867,12 +3950,15 @@ public void partialReevaluationErrorObservedDuringReevaluation() throws Interrup SkyframeLookupResult result = env.getValuesAndExceptions(ImmutableList.of(key2, key3, key4)); if (c == 1) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(result.get(key2)).isNull(); assertThat(result.get(key3)).isNull(); assertThat(result.get(key4)).isNull(); return null; } if (c == 2) { + assertThat(mail.kind()).isEqualTo(Kind.CAUSES); + assertThat(mail.causes().signaledDeps()).containsExactly(key2); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); assertThat(result.get(key3)).isNull(); assertThat(result.get(key4)).isNull(); @@ -3880,6 +3966,8 @@ public void partialReevaluationErrorObservedDuringReevaluation() throws Interrup return null; } assertThat(c).isEqualTo(3); + // The Skyframe stateCache invalidates everything when starting error bubbling: + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(env.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors()) .isTrue(); assertThat(result.get(key2)).isEqualTo(StringValue.of("val2")); @@ -3895,10 +3983,12 @@ public void partialReevaluationErrorObservedDuringReevaluation() throws Interrup } if (skyKey.equals(key2)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); return StringValue.of("val2"); } if (skyKey.equals(key3)) { + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat( key1ObservesTheValueOfKey2.await( TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) @@ -3907,6 +3997,7 @@ public void partialReevaluationErrorObservedDuringReevaluation() throws Interrup } assertThat(skyKey).isEqualTo(key4); + assertThat(mail.kind()).isEqualTo(Kind.FRESHLY_INITIALIZED); assertThat(key4EvaluationCount.incrementAndGet()).isEqualTo(1); throw assertThrows( InterruptedException.class,