Skip to content

Commit

Permalink
Minor AspectFunction / PrerequisiteProducer cleanups.
Browse files Browse the repository at this point in the history 
* Deduplicates TargetAndConfiguration state. It's already stored in
  PrerequisiteProducer.State, which AspectFunction.State has access
  to. Storing a second copy in AspectFunction.InitialValues could cause
  future problems.

* Deletes PrerequisiteProducer.State.resolveAspectDependenciesResult.
  There are no restarts between this step and merging Aspects so storing it
  is not useful.

* Deletes the aspectConfiguration parameter from
  AspectFunction.createAliasAspect. It's always the same as the one
  belonging to the primary AspectKey.

* Replaces an expensive String.format call with cheaper concatenation.

PiperOrigin-RevId: 523174761
Change-Id: I829ff523724f744343e3b74fff96f3d27b57e9ca
  • Loading branch information
@aoeui @copybara-github
aoeui authored and copybara-github committed Apr 10, 2023
1 parent dee8cf1 commit 2a4acf9
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 67 deletions.
50 changes: 18 additions & 32 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,21 +152,15 @@ static class State implements SkyKeyComputeState {
private static class InitialValues {
@Nullable private final Aspect aspect;
@Nullable private final ConfiguredAspectFactory aspectFactory;
@Nullable private final BuildConfigurationValue configuration;
private final ConfiguredTarget associatedTarget;
private final Target target;

private InitialValues(
@Nullable Aspect aspect,
@Nullable ConfiguredAspectFactory aspectFactory,
@Nullable BuildConfigurationValue configuration,
ConfiguredTarget associatedTarget,
Target target) {
ConfiguredTarget associatedTarget) {
this.aspect = aspect;
this.aspectFactory = aspectFactory;
this.configuration = configuration;
this.associatedTarget = associatedTarget;
this.target = target;
}
}

Expand All @@ -177,18 +171,20 @@ public SkyValue compute(SkyKey skyKey, Environment env)
AspectKey key = (AspectKey) skyKey.argument();
State state = env.getState(() -> new State(storeTransitivePackages));

PrerequisiteProducer.State computeDependenciesState = state.computeDependenciesState;
if (state.initialValues == null) {
InitialValues initialValues = getInitialValues(key, env);
InitialValues initialValues = getInitialValues(computeDependenciesState, key, env);
if (initialValues == null) {
return null;
}
state.initialValues = initialValues;
}
Aspect aspect = state.initialValues.aspect;
ConfiguredAspectFactory aspectFactory = state.initialValues.aspectFactory;
BuildConfigurationValue configuration = state.initialValues.configuration;
ConfiguredTarget associatedTarget = state.initialValues.associatedTarget;
Target target = state.initialValues.target;
TargetAndConfiguration targetAndConfiguration = computeDependenciesState.targetAndConfiguration;
Target target = targetAndConfiguration.getTarget();
BuildConfigurationValue configuration = targetAndConfiguration.getConfiguration();

// If the target is incompatible, then there's not much to do. The intent here is to create an
// AspectValue that doesn't trigger any of the associated target's dependencies to be evaluated
Expand All @@ -208,13 +204,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

if (AliasProvider.isAlias(associatedTarget)) {
return createAliasAspect(
env,
new TargetAndConfiguration(target, configuration),
aspect,
key,
configuration,
associatedTarget);
return createAliasAspect(env, targetAndConfiguration, aspect, key, associatedTarget);
}
// If we get here, label should match original label, and therefore the target we looked up
// above indeed corresponds to associatedTarget.getLabel().
Expand Down Expand Up @@ -322,12 +312,11 @@ public SkyValue compute(SkyKey skyKey, Environment env)
try {
depValueMap =
PrerequisiteProducer.computeDependencies(
state.computeDependenciesState,
computeDependenciesState,
state.transitivePackages,
state.transitiveRootCauses,
env,
resolver,
originalTargetAndConfiguration,
topologicalAspectPath,
configConditions.asProviders(),
unloadedToolchainContexts == null
Expand Down Expand Up @@ -355,8 +344,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
ToolchainCollection<ResolvedToolchainContext> toolchainContexts = null;
if (unloadedToolchainContexts != null) {
String targetDescription =
String.format(
"aspect %s applied to %s", aspect.getDescriptor().getDescription(), target);
"aspect " + aspect.getDescriptor().getDescription() + " applied to " + target;
ToolchainCollection.Builder<ResolvedToolchainContext> contextsBuilder =
ToolchainCollection.builder();
for (Map.Entry<String, UnloadedToolchainContext> unloadedContext :
Expand Down Expand Up @@ -434,7 +422,8 @@ static BzlLoadValue.Key bzlLoadKeyForStarlarkAspect(StarlarkAspectClass starlark
}

@Nullable
private static InitialValues getInitialValues(AspectKey key, Environment env)
private static InitialValues getInitialValues(
PrerequisiteProducer.State state, AspectKey key, Environment env)
throws AspectFunctionException, InterruptedException {
StarlarkAspectClass starlarkAspectClass;
ConfiguredAspectFactory aspectFactory = null;
Expand Down Expand Up @@ -545,7 +534,9 @@ private static InitialValues getInitialValues(AspectKey key, Environment env)
throw new IllegalStateException("Name already verified", e);
}

return new InitialValues(aspect, aspectFactory, configuration, associatedTarget, target);
state.targetAndConfiguration = new TargetAndConfiguration(target, configuration);

return new InitialValues(aspect, aspectFactory, associatedTarget);
}

/**
Expand Down Expand Up @@ -640,7 +631,6 @@ private AspectValue createAliasAspect(
TargetAndConfiguration originalTarget,
Aspect aspect,
AspectKey originalKey,
BuildConfigurationValue aspectConfiguration,
ConfiguredTarget configuredTarget)
throws AspectFunctionException, InterruptedException {
ImmutableList<Label> aliasChain =
Expand All @@ -653,7 +643,7 @@ private AspectValue createAliasAspect(
storeTransitivePackages ? NestedSetBuilder.stableOrder() : null;
NestedSetBuilder<Cause> transitiveRootCauses = NestedSetBuilder.stableOrder();

// Compute the Dependency from originalTarget to aliasedLabel
// Compute the Dependency from the original target to aliasedLabel.
Dependency dep;
try {
ComputedToolchainContexts computedToolchainContexts =
Expand All @@ -665,15 +655,11 @@ private AspectValue createAliasAspect(
ToolchainCollection<UnloadedToolchainContext> unloadedToolchainContexts =
computedToolchainContexts.toolchainCollection;

// See comment in compute() above for why we pair target with aspectConfiguration here
TargetAndConfiguration originalTargetAndAspectConfiguration =
new TargetAndConfiguration(originalTarget.getTarget(), aspectConfiguration);

// Get the configuration targets that trigger this rule's configurable attributes.
ConfigConditions configConditions =
PrerequisiteProducer.computeConfigConditions(
env,
originalTargetAndAspectConfiguration,
originalTarget,
transitivePackages,
unloadedToolchainContexts == null
? null
Expand All @@ -690,7 +676,7 @@ private AspectValue createAliasAspect(
}
ConfigurationTransition transition =
TransitionResolver.evaluateTransition(
aspectConfiguration,
originalTarget.getConfiguration(),
NoTransition.INSTANCE,
aliasedTarget,
((ConfiguredRuleClassProvider) ruleClassProvider).getTrimmingTransitionFactory());
Expand All @@ -706,7 +692,7 @@ private AspectValue createAliasAspect(
ConfigurationResolver resolver =
new ConfigurationResolver(
env,
originalTargetAndAspectConfiguration,
originalTarget,
configConditions.asProviders(),
buildViewProvider.getSkyframeBuildView().getStarlarkTransitionCache());
ImmutableList<Dependency> deps =
Expand Down
35 changes: 11 additions & 24 deletions src/main/java/com/google/devtools/build/lib/skyframe/PrerequisiteProducer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,6 @@ static class State implements SkyKeyComputeState, IncompatibleTargetProducer.Res
@Nullable
private Map<SkyKey, ConfiguredTargetAndData> resolveConfiguredTargetDependenciesResult;

/** Null if not yet computed or if {@link #computeDependenciesResult} is non-null. */
@Nullable
private OrderedSetMultimap<Dependency, ConfiguredAspect> resolveAspectDependenciesResult;

/**
* Non-null if all the work in {@link #computeDependencies} is already done. This field contains
Expand Down Expand Up @@ -412,7 +409,6 @@ public boolean evaluate(
transitiveRootCauses,
env,
resolver,
targetAndConfiguration,
ImmutableList.of(),
configConditions.asProviders(),
unloadedToolchainContexts == null
Expand Down Expand Up @@ -886,7 +882,6 @@ public static ImmutableSet<Label> getExecutionPlatformConstraints(
* @param state the compute state
* @param env the Skyframe environment
* @param resolver the dependency resolver
* @param ctgValue the label and the configuration of the node
* @param configConditions the configuration conditions for evaluating the attributes of the node
* @param toolchainContexts the toolchain context for this target
* @param ruleClassProvider rule class provider for determining the right configuration fragments
Expand All @@ -902,7 +897,6 @@ static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDepend
NestedSetBuilder<Cause> transitiveRootCauses,
Environment env,
SkyframeDependencyResolver resolver,
TargetAndConfiguration ctgValue,
Iterable<Aspect> aspects,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
Expand All @@ -912,12 +906,12 @@ static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDepend
ConfiguredValueCreationException,
AspectCreationException,
InterruptedException {
if (state.computeDependenciesResult != null) {
state.storedEventHandlerFromResolveConfigurations.replayOn(env.getListener());
return state.computeDependenciesResult;
}
try {
if (state.computeDependenciesResult != null) {
state.storedEventHandlerFromResolveConfigurations.replayOn(env.getListener());
return state.computeDependenciesResult;
}

TargetAndConfiguration ctgValue = state.targetAndConfiguration;
OrderedSetMultimap<DependencyKind, Dependency> depValueNames;
if (state.resolveConfigurationsResult != null) {
depValueNames = state.resolveConfigurationsResult;
Expand Down Expand Up @@ -1004,17 +998,11 @@ static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDepend
}

// Resolve required aspects.
OrderedSetMultimap<Dependency, ConfiguredAspect> depAspects;
if (state.resolveAspectDependenciesResult != null) {
depAspects = state.resolveAspectDependenciesResult;
} else {
depAspects =
AspectResolver.resolveAspectDependencies(
env, depValues, depValueNames.values(), transitivePackages);
if (env.valuesMissing()) {
return null;
}
state.resolveAspectDependenciesResult = depAspects;
OrderedSetMultimap<Dependency, ConfiguredAspect> depAspects =
AspectResolver.resolveAspectDependencies(
env, depValues, depValueNames.values(), transitivePackages);
if (env.valuesMissing()) {
return null;
}

// Merge the dependent configured targets and aspects into a single map.
Expand All @@ -1032,7 +1020,6 @@ static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDepend
// We won't need these anymore.
state.resolveConfigurationsResult = null;
state.resolveConfiguredTargetDependenciesResult = null;
state.resolveAspectDependenciesResult = null;

return mergeAspectsResult;
} catch (InterruptedException e) {
Expand All @@ -1050,7 +1037,7 @@ static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDepend
/**
* Returns the targets that key the configurable attributes used by this rule.
*
* <p>>If the configured targets supplying those providers aren't yet resolved by the dependency
* <p>If the configured targets supplying those providers aren't yet resolved by the dependency
* resolver, returns null.
*/
@Nullable
Expand Down
22 changes: 11 additions & 11 deletions src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,18 @@
* Tests {@link ConfiguredTargetFunction}'s logic for determining each target's {@link
* BuildConfigurationValue}.
*
* <p>This is essentially an integration test for {@link
* ConfiguredTargetFunction#computeDependencies} and {@link DependencyResolver}. These methods form
* the core logic that figures out what a target's deps are, how their configurations should differ
* from their parent, and how to instantiate those configurations as tangible {@link
* BuildConfigurationValue} objects.
* <p>This is essentially an integration test for {@link PrerequisiteProducer#computeDependencies}
* and {@link DependencyResolver}. These methods form the core logic that figures out what a
* target's deps are, how their configurations should differ from their parent, and how to
* instantiate those configurations as tangible {@link BuildConfigurationValue} objects.
*
* <p>{@link ConfiguredTargetFunction} is a complicated class that does a lot of things. This test
* focuses purely on the task of determining configurations for deps. So instead of evaluating full
* {@link ConfiguredTargetFunction} instances, it evaluates a mock {@link SkyFunction} that just
* wraps the {@link ConfiguredTargetFunction#computeDependencies} part. This keeps focus tight and
* wraps the {@link PrerequisiteProducer#computeDependencies} part. This keeps focus tight and
* integration dependencies narrow.
*
* <p>We can't just call {@link ConfiguredTargetFunction#computeDependencies} directly because that
* <p>We can't just call {@link PrerequisiteProducer#computeDependencies} directly because that
* method needs a {@link SkyFunction.Environment} and Blaze's test infrastructure doesn't support
* direct access to environments.
*/
Expand All @@ -87,8 +86,8 @@ public final class ConfigurationsForTargetsTest extends AnalysisTestCase {
private static final Label EXEC_PLATFORM_LABEL = Label.parseCanonicalUnchecked("//platform:exec");

/**
* A mock {@link SkyFunction} that just calls {@link ConfiguredTargetFunction#computeDependencies}
* and returns its results.
* A mock {@link SkyFunction} that just calls {@link PrerequisiteProducer#computeDependencies} and
* returns its results.
*/
private static class ComputeDependenciesFunction implements SkyFunction {
static final SkyFunctionName SKYFUNCTION_NAME =
Expand Down Expand Up @@ -146,14 +145,15 @@ public SkyValue compute(SkyKey skyKey, Environment env)
PlatformInfo.builder().setLabel(EXEC_PLATFORM_LABEL).build())
.build())
.build();
var state = new PrerequisiteProducer.State();
state.targetAndConfiguration = targetAndConfiguration;
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> depMap =
PrerequisiteProducer.computeDependencies(
new PrerequisiteProducer.State(),
state,
/* transitivePackages= */ null,
/* transitiveRootCauses= */ NestedSetBuilder.stableOrder(),
env,
new SkyframeDependencyResolver(env),
targetAndConfiguration,
ImmutableList.of(),
ImmutableMap.of(),
toolchainContexts,
Expand Down

0 comments on commit 2a4acf9

Please sign in to comment.