Skip to content

Commit

Permalink
Decouple StateMachine from ExtendedEventHandler.
Browse files Browse the repository at this point in the history
Instead, inject it where it is needed.

This is possible because StateMachines always emit events to a
StoredEventHandler associated with SkyKeyComputeState (driven by the
Skyframe requirement that SkyFunctions replay events if there is a restart).

This simplifies some error handling code by making the ExtendedEventHandler
immediately available to error handling callbacks, which would otherwise
need to propagate errors upwards.

PiperOrigin-RevId: 548175834
Change-Id: Ie384bb05561d6503513603971191d13b545bce18
  • Loading branch information
aoeui authored and copybara-github committed Jul 14, 2023
1 parent a522c13 commit 70dcf07
Show file tree
Hide file tree
Showing 27 changed files with 152 additions and 199 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper.ValidationException;
Expand Down Expand Up @@ -136,7 +135,7 @@ public IncompatibleTargetProducer(
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
Rule rule = targetAndConfiguration.getTarget().getAssociatedRule();
if (rule == null || !rule.useToolchainResolution() || platformInfo == null) {
sink.acceptIncompatibleTarget(Optional.empty());
Expand Down Expand Up @@ -178,7 +177,7 @@ public void accept(SkyValue value) {
invalidConstraintValuesBuilder.add(info);
}

private StateMachine processResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine processResult(Tasks tasks) {
var invalidConstraintValues = invalidConstraintValuesBuilder.build();
if (!invalidConstraintValues.isEmpty()) {
sink.acceptIncompatibleTarget(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
Expand Down Expand Up @@ -84,7 +83,7 @@ interface ResultSink {
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
if (configLabels == null) {
sink.acceptConfigConditions(ConfigConditions.EMPTY);
return runAfter;
Expand Down Expand Up @@ -133,7 +132,7 @@ public void acceptConfiguredTargetAndDataError(InconsistentNullConfigException e
"ConfigCondition dependency should never be evaluated with a null configuration.", error);
}

private StateMachine constructConfigConditions(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine constructConfigConditions(Tasks tasks) {
if (mostImportantExitCode != null) {
return runAfter; // There was a previous error.
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.google.devtools.build.lib.analysis.DuplicateException;
import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.AspectDescriptor;
import com.google.devtools.build.lib.skyframe.AspectCreationException;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
Expand Down Expand Up @@ -73,7 +72,7 @@ interface ResultSink {
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
var baseKey = ConfiguredTargetKey.fromConfiguredTarget(prerequisite.getConfiguredTarget());
var memoTable = new HashMap<AspectDescriptor, AspectKey>();
for (AspectDeps deps : aspects.getUsedAspects()) {
Expand All @@ -96,7 +95,7 @@ public void acceptValueOrException(
sink.acceptConfiguredAspectError(error);
}

private StateMachine processResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine processResult(Tasks tasks) {
ImmutableSet<AspectDeps> usedAspects = aspects.getUsedAspects();
if (aspectValues.size() < usedAspects.size()) {
return DONE; // There was an error.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.devtools.build.lib.analysis.InconsistentNullConfigException;
import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
Expand Down Expand Up @@ -87,7 +86,7 @@ public ConfiguredTargetAndDataProducer(
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
tasks.lookUp(
key,
ConfiguredValueCreationException.class,
Expand Down Expand Up @@ -133,7 +132,7 @@ public void acceptValueOrException3(
throw new IllegalArgumentException("both value and error were null");
}

private StateMachine fetchConfigurationAndPackage(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine fetchConfigurationAndPackage(Tasks tasks) {
if (configuredTarget == null) {
return DONE; // There was a previous error.
}
Expand Down Expand Up @@ -174,7 +173,7 @@ public void accept(SkyValue value) {
throw new IllegalArgumentException("unexpected value: " + value);
}

private StateMachine constructResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine constructResult(Tasks tasks) {
Target target;
try {
target = pkg.getTarget(configuredTarget.getLabel().getName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import com.google.devtools.build.lib.analysis.ToolchainCollection;
import com.google.devtools.build.lib.analysis.TransitiveDependencyState;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException;
import com.google.devtools.build.lib.skyframe.toolchains.ToolchainException;
import com.google.devtools.build.lib.skyframe.toolchains.UnloadedToolchainContext;
Expand Down Expand Up @@ -71,7 +70,7 @@ public DependencyContextProducer(
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
return new UnloadedToolchainContextsProducer(
unloadedToolchainContextsInputs,
(UnloadedToolchainContextsProducer.ResultSink) this,
Expand All @@ -90,7 +89,7 @@ public void acceptUnloadedToolchainContextsError(ToolchainException error) {
sink.acceptDependencyContextError(DependencyContextError.of(error));
}

private StateMachine computeConfigConditions(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine computeConfigConditions(Tasks tasks) {
if (hasError) {
return DONE;
}
Expand All @@ -114,7 +113,7 @@ public void acceptConfigConditionsError(ConfiguredValueCreationException error)
sink.acceptDependencyContextError(DependencyContextError.of(error));
}

private StateMachine constructResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine constructResult(Tasks tasks) {
if (hasError) {
return DONE;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.google.devtools.build.lib.analysis.constraints.IncompatibleTargetChecker.IncompatibleTargetException;
import com.google.devtools.build.lib.analysis.constraints.IncompatibleTargetChecker.IncompatibleTargetProducer;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper.ValidationException;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException;
Expand Down Expand Up @@ -77,7 +76,7 @@ public DependencyContextProducerWithCompatibilityCheck(
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
var defaultToolchainContextKey = unloadedToolchainContextsInputs.targetToolchainContextKey();
if (defaultToolchainContextKey == null) {
// If `defaultToolchainContextKey` is null, there's no platform info, incompatibility check
Expand Down Expand Up @@ -115,7 +114,7 @@ public void acceptPlatformInfoError(InvalidPlatformException error) {
sink.acceptDependencyContextError(DependencyContextError.of(error));
}

private StateMachine computeConfigConditions(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine computeConfigConditions(Tasks tasks) {
if (hasError) {
return DONE;
}
Expand All @@ -140,7 +139,7 @@ public void acceptConfigConditionsError(ConfiguredValueCreationException error)
sink.acceptDependencyContextError(DependencyContextError.of(error));
}

private StateMachine checkCompatibility(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine checkCompatibility(Tasks tasks) {
if (hasError) {
return DONE;
}
Expand Down Expand Up @@ -170,8 +169,7 @@ public void acceptValidationException(ValidationException e) {
sink.acceptDependencyContextError(DependencyContextError.of(e));
}

private StateMachine computeUnloadedToolchainContexts(
Tasks tasks, ExtendedEventHandler listener) {
private StateMachine computeUnloadedToolchainContexts(Tasks tasks) {
if (hasError) {
return DONE;
}
Expand All @@ -194,7 +192,7 @@ public void acceptUnloadedToolchainContextsError(ToolchainException error) {
sink.acceptDependencyContextError(DependencyContextError.of(error));
}

private StateMachine constructResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine constructResult(Tasks tasks) {
if (hasError) {
return DONE;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionCollector;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
Expand Down Expand Up @@ -86,7 +85,7 @@ public DependencyMapProducer(
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
int index = 0;
for (Map.Entry<DependencyKind, Collection<Label>> entry : dependencyLabels.asMap().entrySet()) {
var kind = entry.getKey();
Expand Down Expand Up @@ -122,7 +121,7 @@ public void acceptTransition(
sink.acceptTransition(kind, label, transition);
}

private StateMachine buildAndEmitResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine buildAndEmitResult(Tasks tasks) {
if (lastError != null || parameters.transitiveState().hasRootCause()) {
return DONE; // There was an error.
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import com.google.devtools.build.lib.causes.LoadingFailedCause;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.AttributeTransitionData;
Expand Down Expand Up @@ -115,7 +114,7 @@ interface ResultSink extends TransitionCollector {
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
@Nullable Attribute attribute = kind.getAttribute();

if (kind == VISIBILITY_DEPENDENCY
Expand Down Expand Up @@ -178,6 +177,7 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
attributeTransition,
parameters.transitionCache(),
(TransitionApplier.ResultSink) this,
parameters.eventHandler(),
/* runAfter= */ this::processTransitionResult);
}

Expand All @@ -197,12 +197,12 @@ public void acceptTransitionError(OptionsParsingException e) {
sink.acceptDependencyError(DependencyError.of(e));
}

private StateMachine processTransitionResult(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine processTransitionResult(Tasks tasks) {
if (transitionedConfigurations == null) {
return DONE; // There was a previously reported error.
}

if (isNonconfigurableTargetInSamePackage(listener)) {
if (isNonconfigurableTargetInSamePackage()) {
// The target is in the same package as the parent and non-configurable. In the general case
// loading a child target would defeat Package-based sharding. However, when the target is in
// the same Package, that concern no longer applies. This optimization means that delegation,
Expand All @@ -224,7 +224,9 @@ private StateMachine processTransitionResult(Tasks tasks, ExtendedEventHandler l
for (BuildConfigurationKey configuration : transitionedConfigurations.values()) {
String childChecksum = configuration.getOptionsChecksum();
if (!parentChecksum.equals(childChecksum)) {
listener.post(ConfigurationTransitionEvent.create(parentChecksum, childChecksum));
parameters
.eventHandler()
.post(ConfigurationTransitionEvent.create(parentChecksum, childChecksum));
}
}

Expand Down Expand Up @@ -299,7 +301,7 @@ public void acceptPrerequisitesAspectError(AspectCreationException error) {
sink.acceptDependencyError(DependencyError.of(error));
}

private boolean isNonconfigurableTargetInSamePackage(ExtendedEventHandler listener) {
private boolean isNonconfigurableTargetInSamePackage() {
Target parentTarget = parameters.target();
if (parentTarget.getLabel().getPackageIdentifier().equals(toLabel.getPackageIdentifier())) {
try {
Expand All @@ -311,10 +313,12 @@ private boolean isNonconfigurableTargetInSamePackage(ExtendedEventHandler listen
parameters
.transitiveState()
.addTransitiveCause(new LoadingFailedCause(toLabel, e.getDetailedExitCode()));
listener.handle(
Event.error(
TargetUtils.getLocationMaybe(parentTarget),
TargetUtils.formatMissingEdge(parentTarget, toLabel, e, kind.getAttribute())));
parameters
.eventHandler()
.handle(
Event.error(
TargetUtils.getLocationMaybe(parentTarget),
TargetUtils.formatMissingEdge(parentTarget, toLabel, e, kind.getAttribute())));
}
}
return false;
Expand All @@ -338,7 +342,7 @@ private ExecGroupErrorEmitter(String message) {
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
// The configuration value should already exist as a dependency so this lookup is safe enough
// for error handling.
tasks.lookUp(parameters.configurationKey(), (Consumer<SkyValue>) this);
Expand All @@ -350,8 +354,10 @@ public void accept(SkyValue value) {
this.configuration = (BuildConfigurationValue) value;
}

private StateMachine postEvent(Tasks tasks, ExtendedEventHandler listener) {
listener.post(AnalysisRootCauseEvent.withConfigurationValue(configuration, toLabel, message));
private StateMachine postEvent(Tasks tasks) {
parameters
.eventHandler()
.post(AnalysisRootCauseEvent.withConfigurationValue(configuration, toLabel, message));
sink.acceptDependencyError(
DependencyError.of(
new DependencyEvaluationException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
Expand Down Expand Up @@ -64,7 +63,7 @@ interface ResultSink {
}

@Override
public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
public StateMachine step(Tasks tasks) {
// Loads the Package first to verify the Target. The ConfiguredTarget should not be loaded
// until after verification. See https://github.com/bazelbuild/bazel/pull/10307.
//
Expand Down Expand Up @@ -102,7 +101,7 @@ public void acceptValueOrException(
throw new IllegalArgumentException("both value and error were null");
}

private StateMachine lookupPlatform(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine lookupPlatform(Tasks tasks) {
if (!passedValidation) {
return runAfter;
}
Expand All @@ -126,7 +125,7 @@ private void acceptPlatformValueOrError(
throw new IllegalArgumentException("both value and error were null");
}

private StateMachine retrievePlatformInfo(Tasks tasks, ExtendedEventHandler listener) {
private StateMachine retrievePlatformInfo(Tasks tasks) {
if (platform == null) {
return runAfter; // An error occurred and was reported.
}
Expand Down
Loading

0 comments on commit 70dcf07

Please sign in to comment.