Skip to content

Commit

Permalink
Narrow inputs for computation of unloaded toolchain contexts.
Browse files Browse the repository at this point in the history 
Instead of requiring a long list of arguments, the tuple composed of
ExecGroupCollection.Builder and the default ToolchainContextKey
suffices.

Part of the logic for auto exec groups has been moved into
ExecGroupCollection where it is more logically cohesive and reduces the
number of map-to-map-to-map transformations by one. Some logic remains in
createDefaultToolchainContextKey.

PiperOrigin-RevId: 524962389
Change-Id: Ic892d17a5433d37252c9bf7e0df1d094a8c6eccc
  • Loading branch information
@aoeui @fweikert
aoeui authored and fweikert committed May 25, 2023
1 parent bf5c94c commit 0d600cf
Show file tree
Hide file tree
Showing 6 changed files with 291 additions and 274 deletions.
35 changes: 24 additions & 11 deletions src/main/java/com/google/devtools/build/lib/analysis/ExecGroupCollection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.Maps;
import com.google.common.collect.Table;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
Expand All @@ -44,17 +45,22 @@
*/
@AutoValue
public abstract class ExecGroupCollection {

public static Builder emptyBuilder() {
return new AutoValue_ExecGroupCollection_Builder(ImmutableMap.of());
}

public static Builder builder(
/**
* Prepares the input exec groups to serve as {@link Builder#execGroups}.
*
* <p>Applies any inheritance specified via {@link ExecGroup#copyFrom} and adds auto exec groups
* when {@code useAutoExecGroups} is true.
*/
public static ImmutableMap<String, ExecGroup> process(
ImmutableMap<String, ExecGroup> execGroups,
ImmutableSet<Label> defaultExecWith,
ImmutableSet<ToolchainTypeRequirement> defaultToolchainTypes) {
// Post-process the groups to handle inheritance.
Map<String, ExecGroup> processedGroups = new LinkedHashMap<>();
ImmutableSet<ToolchainTypeRequirement> defaultToolchainTypes,
boolean useAutoExecGroups) {
var processedGroups =
Maps.<String, ExecGroup>newHashMapWithExpectedSize(
useAutoExecGroups
? (execGroups.size() + defaultToolchainTypes.size())
: execGroups.size());
for (Map.Entry<String, ExecGroup> entry : execGroups.entrySet()) {
String name = entry.getKey();
ExecGroup execGroup = entry.getValue();
Expand All @@ -74,13 +80,20 @@ public static Builder builder(
processedGroups.put(name, execGroup);
}

return new AutoValue_ExecGroupCollection_Builder(ImmutableMap.copyOf(processedGroups));
if (useAutoExecGroups) {
// Creates one exec group for each toolchain (automatic exec groups).
for (ToolchainTypeRequirement toolchainType : defaultToolchainTypes) {
processedGroups.put(
toolchainType.toolchainType().toString(),
ExecGroup.builder().addToolchainType(toolchainType).copyFrom(null).build());
}
}
return ImmutableMap.copyOf(processedGroups);
}

/** Builder class for correctly constructing ExecGroupCollection instances. */
// Note that this is _not_ an actual @AutoValue.Builder: it provides more logic and has different
// fields.
@AutoValue
public abstract static class Builder {
public abstract ImmutableMap<String, ExecGroup> execGroups();

Expand Down
15 changes: 14 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/ToolchainCollection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.google.devtools.build.lib.analysis;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.Maps.newHashMapWithExpectedSize;

import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -78,10 +79,22 @@ public static <T extends ToolchainContext> Builder<T> builder() {
return new Builder<T>();
}

public static <T extends ToolchainContext> Builder<T> builderWithExpectedSize(int expectedSize) {
return new Builder<T>(expectedSize);
}

/** Builder for ToolchainCollection. */
public static final class Builder<T extends ToolchainContext> {
// This is not immutable so that we can check for duplicate keys easily.
private final Map<String, T> toolchainContexts = new HashMap<>();
private final Map<String, T> toolchainContexts;

private Builder() {
this.toolchainContexts = new HashMap<>();
}

private Builder(int expectedSize) {
this.toolchainContexts = newHashMapWithExpectedSize(expectedSize);
}

public ToolchainCollection<T> build() {
Preconditions.checkArgument(toolchainContexts.containsKey(ExecGroup.DEFAULT_EXEC_GROUP_NAME));
Expand Down
118 changes: 61 additions & 57 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 @@ -15,9 +15,9 @@
package com.google.devtools.build.lib.skyframe;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.devtools.build.lib.skyframe.PrerequisiteProducer.createDefaultToolchainContextKey;

import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -64,6 +64,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.AspectDefinition;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NativeAspectClass;
Expand All @@ -81,7 +82,7 @@
import com.google.devtools.build.lib.profiler.memory.CurrentRuleTracker;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.PrerequisiteProducer.ComputedToolchainContexts;
import com.google.devtools.build.lib.skyframe.PrerequisiteProducer.UnloadedToolchainContextsInputs;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.build.skyframe.SkyFunction;
Expand All @@ -94,6 +95,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import javax.annotation.Nullable;
import net.starlark.java.eval.StarlarkSemantics;

Expand Down Expand Up @@ -275,27 +277,25 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}
}

TargetAndConfiguration originalTargetAndConfiguration =
new TargetAndConfiguration(target, configuration);
try {
ComputedToolchainContexts computedToolchainContexts =
getUnloadedToolchainContexts(env, key, aspect, configuration);
if (env.valuesMissing()) {
var unloadedToolchainContextsInputs =
getUnloadedToolchainContextsInputs(
aspect.getDefinition(), key.getConfigurationKey(), configuration);
Optional<ToolchainCollection<UnloadedToolchainContext>> computedToolchainContexts =
PrerequisiteProducer.computeUnloadedToolchainContexts(
env, unloadedToolchainContextsInputs);
if (computedToolchainContexts == null) {
return null;
}

ToolchainCollection<UnloadedToolchainContext> unloadedToolchainContexts = null;
ExecGroupCollection.Builder execGroupCollectionBuilder = null;
if (computedToolchainContexts != null) {
unloadedToolchainContexts = computedToolchainContexts.toolchainCollection;
execGroupCollectionBuilder = computedToolchainContexts.execGroupCollectionBuilder;
}
ToolchainCollection<UnloadedToolchainContext> unloadedToolchainContexts =
computedToolchainContexts.orElse(null);

// Get the configuration targets that trigger this rule's configurable attributes.
ConfigConditions configConditions =
PrerequisiteProducer.computeConfigConditions(
env,
originalTargetAndConfiguration,
targetAndConfiguration,
state.transitivePackages,
unloadedToolchainContexts == null
? null
Expand Down Expand Up @@ -368,7 +368,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
configuration,
configConditions,
toolchainContexts,
execGroupCollectionBuilder,
unloadedToolchainContextsInputs,
depValueMap,
state.transitivePackages);
} catch (DependencyEvaluationException e) {
Expand Down Expand Up @@ -559,46 +559,42 @@ static StarlarkDefinedAspect loadAspectFromBzl(
}

@Nullable
private static ComputedToolchainContexts getUnloadedToolchainContexts(
Environment env,
AspectKey key,
Aspect aspect,
@Nullable BuildConfigurationValue configuration)
throws InterruptedException, AspectCreationException {
private static UnloadedToolchainContextsInputs getUnloadedToolchainContextsInputs(
AspectDefinition aspectDefinition,
@Nullable BuildConfigurationKey configurationKey,
@Nullable BuildConfigurationValue configuration) {
if (configuration == null) {
// Configuration can be null in the case of aspects applied to input files. In this case,
// there are no chances of toolchains being used, so skip it.
return null;
}

ImmutableMap<String, Attribute> aspectAttributes = aspect.getDefinition().getAttributes();
// there are no toolchains being used.
return UnloadedToolchainContextsInputs.empty();
}

boolean useAutoExecGroups = shouldUseAutoExecGroups(aspectDefinition, configuration);
var processedExecGroups =
ExecGroupCollection.process(
aspectDefinition.execGroups(),
aspectDefinition.execCompatibleWith(),
aspectDefinition.getToolchainTypes(),
useAutoExecGroups);
// Note: `configuration.getOptions().hasNoConfig()` is handled early in #compute.
return UnloadedToolchainContextsInputs.create(
processedExecGroups,
createDefaultToolchainContextKey(
configurationKey,
aspectDefinition.execCompatibleWith(),
/* debugTarget= */ false,
/* useAutoExecGroups= */ useAutoExecGroups,
aspectDefinition.getToolchainTypes(),
/* parentExecutionPlatformLabel= */ null));
}

boolean useAutoExecGroups;
private static boolean shouldUseAutoExecGroups(
AspectDefinition aspectDefinition, BuildConfigurationValue configuration) {
ImmutableMap<String, Attribute> aspectAttributes = aspectDefinition.getAttributes();
if (aspectAttributes.containsKey("$use_auto_exec_groups")) {
useAutoExecGroups =
(boolean) aspectAttributes.get("$use_auto_exec_groups").getDefaultValueUnchecked();
} else {
useAutoExecGroups = configuration.useAutoExecGroups();
}

// Determine what toolchains are needed by this target.
try {
return PrerequisiteProducer.computeUnloadedToolchainContexts(
env,
key.getLabel(),
!configuration.getOptions().hasNoConfig(),
Predicates.alwaysFalse(),
useAutoExecGroups,
configuration.getKey(),
aspect.getDefinition().getToolchainTypes(),
aspect.getDefinition().execCompatibleWith(),
aspect.getDefinition().execGroups(),
null);
} catch (ToolchainException e) {
// TODO(katre): better error handling
throw new AspectCreationException(
e.getMessage(), new LabelCause(key.getLabel(), e.getDetailedExitCode()));
return (boolean) aspectAttributes.get("$use_auto_exec_groups").getDefaultValueUnchecked();
}
return configuration.useAutoExecGroups();
}

/**
Expand Down Expand Up @@ -643,14 +639,20 @@ private AspectValue createAliasAspect(
// Compute the Dependency from the original target to aliasedLabel.
Dependency dep;
try {
ComputedToolchainContexts computedToolchainContexts =
getUnloadedToolchainContexts(env, originalKey, aspect, originalTarget.getConfiguration());
if (env.valuesMissing()) {
var configuration = originalTarget.getConfiguration();
// Determine what toolchains are needed by this target.
var unloadedToolchainContextsInputs =
getUnloadedToolchainContextsInputs(
aspect.getDefinition(), originalKey.getConfigurationKey(), configuration);
Optional<ToolchainCollection<UnloadedToolchainContext>> computedToolchainContexts =
PrerequisiteProducer.computeUnloadedToolchainContexts(
env, unloadedToolchainContextsInputs);
if (computedToolchainContexts == null) {
return null;
}

ToolchainCollection<UnloadedToolchainContext> unloadedToolchainContexts =
computedToolchainContexts.toolchainCollection;
computedToolchainContexts.orElse(null);

// Get the configuration targets that trigger this rule's configurable attributes.
ConfigConditions configConditions =
Expand All @@ -673,7 +675,7 @@ private AspectValue createAliasAspect(
}
ConfigurationTransition transition =
TransitionResolver.evaluateTransition(
originalTarget.getConfiguration(),
configuration,
NoTransition.INSTANCE,
aliasedTarget,
((ConfiguredRuleClassProvider) ruleClassProvider).getTrimmingTransitionFactory());
Expand Down Expand Up @@ -705,8 +707,10 @@ private AspectValue createAliasAspect(
throw new AspectFunctionException(e);
} catch (ConfiguredValueCreationException e) {
throw new AspectFunctionException(e);
} catch (AspectCreationException e) {
throw new AspectFunctionException(e);
} catch (ToolchainException e) {
throw new AspectFunctionException(
new AspectCreationException(
e.getMessage(), new LabelCause(originalKey.getLabel(), e.getDetailedExitCode())));
}

if (!transitiveRootCauses.isEmpty()) {
Expand Down
Loading

0 comments on commit 0d600cf

Please sign in to comment.