diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index b29db6f994c71b..51e4e1720c9c36 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -19,6 +19,7 @@ import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; +import com.google.common.base.Predicates; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; @@ -113,6 +114,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.IntFunction; +import java.util.function.Predicate; import java.util.function.Supplier; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -268,11 +270,8 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) env.getValues(state.allInputs.packageLookupsRequested); Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state); } - CheckInputResults checkedInputs = null; - @Nullable - ImmutableSet mandatoryInputs = - action.discoversInputs() ? action.getMandatoryInputs().toSet() : null; + CheckInputResults checkedInputs = null; NestedSet allInputs = state.allInputs.getAllInputs(); Iterable depKeys = getInputDepKeys(allInputs, state); @@ -290,9 +289,7 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) try { if (previousExecution == null && !state.hasArtifactData()) { // Do we actually need to find our metadata? - checkedInputs = - checkInputs( - env, action, inputDeps, allInputs, mandatoryInputs, depKeys, actionLookupData); + checkedInputs = checkInputs(env, action, inputDeps, allInputs, depKeys, actionLookupData); } } catch (ActionExecutionException e) { // Remove action from state map in case it's there (won't be unless it discovers inputs). @@ -572,7 +569,6 @@ private ActionInputDepOwners createAugmentedInputDepOwners( action, inputDeps, allInputs, - action.discoversInputs() ? action.getMandatoryInputs().toSet() : null, requestedSkyKeys, lostInputsAndOwnersSoFar, actionLookupDataForError); @@ -1094,7 +1090,6 @@ private CheckInputResults checkInputs( ArtifactNestedSetEvalException>> inputDeps, NestedSet allInputs, - ImmutableSet mandatoryInputs, Iterable requestedSkyKeys, ActionLookupData actionLookupDataForError) throws ActionExecutionException, InterruptedException { @@ -1103,7 +1098,6 @@ private CheckInputResults checkInputs( action, inputDeps, allInputs, - mandatoryInputs, requestedSkyKeys, ActionInputMap::new, CheckInputResults::new, @@ -1124,7 +1118,6 @@ private ActionInputDepOwnerMap getInputDepOwners( ArtifactNestedSetEvalException>> inputDeps, NestedSet allInputs, - ImmutableSet mandatoryInputs, Iterable requestedSkyKeys, Collection lostInputs, ActionLookupData actionLookupDataForError) @@ -1138,7 +1131,6 @@ private ActionInputDepOwnerMap getInputDepOwners( action, inputDeps, allInputs, - mandatoryInputs, requestedSkyKeys, ignoredInputDepsSize -> new ActionInputDepOwnerMap(lostInputs), (actionInputMapSink, @@ -1150,6 +1142,29 @@ private ActionInputDepOwnerMap getInputDepOwners( actionLookupDataForError); } + private static Predicate makeMandatoryInputPredicate(Action action) { + if (!action.discoversInputs()) { + return Predicates.alwaysTrue(); + } + + return new Predicate() { + // Lazily flatten the NestedSet in case the predicate is never needed. It's only used in the + // exceptional case of a missing artifact. + private ImmutableSet mandatoryInputs = null; + + @Override + public boolean test(Artifact input) { + if (!input.isSourceArtifact()) { + return true; + } + if (mandatoryInputs == null) { + mandatoryInputs = action.getMandatoryInputs().toSet(); + } + return mandatoryInputs.contains(input); + } + }; + } + /** * May return {@code null} if {@code allowValuesMissingEarlyReturn} and {@code * env.valuesMissing()} are true and no inputs result in {@link ActionExecutionException}s. @@ -1165,14 +1180,13 @@ private R accumulateInputs( ArtifactNestedSetEvalException>> inputDeps, NestedSet allInputs, - ImmutableSet mandatoryInputs, Iterable requestedSkyKeys, IntFunction actionInputMapSinkFactory, AccumulateInputResultsFactory accumulateInputResultsFactory, boolean allowValuesMissingEarlyReturn, ActionLookupData actionLookupDataForError) throws ActionExecutionException, InterruptedException { - + Predicate isMandatoryInput = makeMandatoryInputPredicate(action); ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = new ActionExecutionFunctionExceptionHandler( Suppliers.memoize( @@ -1191,7 +1205,7 @@ private R accumulateInputs( }), inputDeps, action, - mandatoryInputs, + isMandatoryInput, requestedSkyKeys, env.valuesMissing()); @@ -1218,7 +1232,7 @@ private R accumulateInputs( for (Artifact input : allInputsList) { SkyValue value = ArtifactNestedSetFunction.getInstance().getValueForKey(Artifact.key(input)); if (value == null) { - if (mandatoryInputs == null || mandatoryInputs.contains(input)) { + if (isMandatoryInput.test(input)) { BugReport.sendBugReport( new IllegalStateException( String.format( @@ -1228,7 +1242,7 @@ private R accumulateInputs( continue; } if (value instanceof MissingArtifactValue) { - if (actionExecutionFunctionExceptionHandler.isMandatory(input)) { + if (isMandatoryInput.test(input)) { actionExecutionFunctionExceptionHandler.accumulateMissingFileArtifactValue( input, (MissingArtifactValue) value); continue; @@ -1438,7 +1452,7 @@ private final class ActionExecutionFunctionExceptionHandler { SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>> inputDeps; private final Action action; - private final Set mandatoryInputs; + private final Predicate isMandatoryInput; private final Iterable requestedSkyKeys; private final boolean valuesMissing; List missingArtifactCauses = Lists.newArrayListWithCapacity(0); @@ -1454,13 +1468,13 @@ private final class ActionExecutionFunctionExceptionHandler { ArtifactNestedSetEvalException>> inputDeps, Action action, - Set mandatoryInputs, + Predicate isMandatoryInput, Iterable requestedSkyKeys, boolean valuesMissing) { this.skyKeyToDerivedArtifactSetForExceptions = skyKeyToDerivedArtifactSetForExceptions; this.inputDeps = inputDeps; this.action = action; - this.mandatoryInputs = mandatoryInputs; + this.isMandatoryInput = isMandatoryInput; this.requestedSkyKeys = requestedSkyKeys; this.valuesMissing = valuesMissing; } @@ -1540,7 +1554,7 @@ private void handleSourceArtifactExceptionFromSkykey(SkyKey key, SourceArtifactE return; } - if (isMandatory((Artifact) key)) { + if (isMandatoryInput.test((Artifact) key)) { missingArtifactCauses.add( createLabelCauseNullOwnerOk( (Artifact) key, @@ -1586,15 +1600,9 @@ void maybeThrowException() throws ActionExecutionException { } } - boolean isMandatory(Artifact input) { - return !input.isSourceArtifact() - || mandatoryInputs == null - || mandatoryInputs.contains(input); - } - private void handleActionExecutionExceptionPerArtifact( Artifact input, ActionExecutionException e) { - if (isMandatory(input)) { + if (isMandatoryInput.test(input)) { // Prefer a catastrophic exception as the one we propagate. if (firstActionExecutionException == null || (!firstActionExecutionException.isCatastrophe() && e.isCatastrophe())) {