Try to clean up some of the excess baggage Aspects are carting around, mainly from a code cleanliness perspective, but with some minor memory benefits:
1. Use AspectKey instead of AspectValue wherever possible at the top-level, since keys are what callers should be using, not values.
2. Remove the freestanding label field from AspectKey. It is always equal to its ConfiguredTargetKey field's label. Having two fields with redundant data is confusing.
3. Remove the freestanding label field from AspectValue. Same reason: it was always the AspectKey's label.
4. Remove AspectDescriptor from ConfiguredAspect: it comes from the AspectKey, so just use the key.
This is also good because I think maximizing correspondence between ConfiguredAspect and ConfiguredTarget is a goal. Having an AspectValue muddies the waters. Note that, for historical reasons, ConfiguredTarget has much more data in it than we'd want a ConfiguredAspect to have (ideally, both would just have actions and providers).
In a follow-up, I may wrap AspectValue together with an AspectKey in TopLevelSkylarkAspectFunction#compute's return value. I think that, plus some tolerable casting, would remove the only remaining real need for AspectValue to have an AspectKey inside it.
PiperOrigin-RevId: 307877866
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
index 60edd8f..ae03826 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
@@ -15,12 +15,14 @@
package com.google.devtools.build.lib.analysis;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionGraph;
import com.google.devtools.build.lib.actions.PackageRoots;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import java.util.Collection;
import javax.annotation.Nullable;
@@ -38,7 +40,7 @@
private final ImmutableSet<ConfiguredTarget> parallelTests;
private final ImmutableSet<ConfiguredTarget> exclusiveTests;
@Nullable private final TopLevelArtifactContext topLevelContext;
- private final ImmutableSet<AspectValue> aspects;
+ private final ImmutableMap<AspectKey, ConfiguredAspect> aspects;
private final PackageRoots packageRoots;
private final String workspaceName;
private final Collection<TargetAndConfiguration> topLevelTargetsWithConfigs;
@@ -47,7 +49,7 @@
AnalysisResult(
BuildConfigurationCollection configurations,
ImmutableSet<ConfiguredTarget> targetsToBuild,
- ImmutableSet<AspectValue> aspects,
+ ImmutableMap<AspectKey, ConfiguredAspect> aspects,
@Nullable ImmutableList<ConfiguredTarget> targetsToTest,
ImmutableSet<ConfiguredTarget> targetsToSkip,
@Nullable String error,
@@ -93,13 +95,8 @@
return packageRoots;
}
- /**
- * Returns aspects of configured targets to build.
- *
- * <p>If this list is empty, build the targets returned by {@code getTargetsToBuild()}.
- * Otherwise, only build these aspects of the targets returned by {@code getTargetsToBuild()}.
- */
- public ImmutableSet<AspectValue> getAspects() {
+ /** Returns aspects to build. */
+ public ImmutableMap<AspectKey, ConfiguredAspect> getAspectsMap() {
return aspects;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java
index 650a841..3d42896 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java
@@ -109,7 +109,7 @@
@Override
public BuildEventId getEventId() {
return BuildEventIdUtil.aspectCompleted(
- aspectValue.getLabel(),
+ aspectValue.getKey().getLabel(),
configurationEventId,
aspectValue.getAspect().getDescriptor().getDescription());
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java
index 57a80c3..6735b6c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java
@@ -16,23 +16,22 @@
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.actions.BasicActionLookupValue;
-import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
-import com.google.devtools.build.lib.skyframe.ConfiguredObjectValue;
import com.google.devtools.build.lib.syntax.Location;
import javax.annotation.Nullable;
/** An aspect in the context of the Skyframe graph. */
public final class AspectValue extends BasicActionLookupValue implements ConfiguredObjectValue {
-
// These variables are only non-final because they may be clear()ed to save memory. They are null
// only after they are cleared except for transitivePackagesForPackageRootResolution.
- @Nullable private Label label;
@Nullable private Aspect aspect;
@Nullable private Location location;
+ // Normally the key used to evaluate this value in AspectFunction#compute. But in the case of a
+ // top-level SkylarkAspectKey, the AspectValue will be this value but the key will be the
+ // associated aspect key from SkylarkAspectKey#toAspectkey.
@Nullable private AspectKey key;
@Nullable private ConfiguredAspect configuredAspect;
// May be null either after clearing or because transitive packages are not tracked.
@@ -41,16 +40,14 @@
public AspectValue(
AspectKey key,
Aspect aspect,
- Label label,
Location location,
ConfiguredAspect configuredAspect,
NestedSet<Package> transitivePackagesForPackageRootResolution) {
super(configuredAspect.getActions());
- this.label = Preconditions.checkNotNull(label, actions);
- this.aspect = Preconditions.checkNotNull(aspect, label);
- this.location = Preconditions.checkNotNull(location, label);
- this.key = Preconditions.checkNotNull(key, label);
- this.configuredAspect = Preconditions.checkNotNull(configuredAspect, label);
+ this.key = key;
+ this.aspect = Preconditions.checkNotNull(aspect, location);
+ this.location = Preconditions.checkNotNull(location, aspect);
+ this.configuredAspect = Preconditions.checkNotNull(configuredAspect, location);
this.transitivePackagesForPackageRootResolution = transitivePackagesForPackageRootResolution;
}
@@ -58,10 +55,6 @@
return Preconditions.checkNotNull(configuredAspect);
}
- public Label getLabel() {
- return Preconditions.checkNotNull(label);
- }
-
public Location getLocation() {
return Preconditions.checkNotNull(location);
}
@@ -76,13 +69,11 @@
@Override
public void clear(boolean clearEverything) {
- Preconditions.checkNotNull(label, this);
Preconditions.checkNotNull(aspect, this);
Preconditions.checkNotNull(location, this);
Preconditions.checkNotNull(key, this);
Preconditions.checkNotNull(configuredAspect, this);
if (clearEverything) {
- label = null;
aspect = null;
location = null;
key = null;
@@ -99,7 +90,6 @@
@Override
public String toString() {
return getStringHelper()
- .add("label", label)
.add("key", key)
.add("location", location)
.add("aspect", aspect)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 90bdded..2e4a23b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -460,7 +460,7 @@
Set<Label> testsToRun = loadingResult.getTestsToRunLabels();
Set<ConfiguredTarget> configuredTargets =
Sets.newLinkedHashSet(skyframeAnalysisResult.getConfiguredTargets());
- ImmutableSet<AspectValue> aspects = ImmutableSet.copyOf(skyframeAnalysisResult.getAspects());
+ ImmutableMap<AspectKey, ConfiguredAspect> aspects = skyframeAnalysisResult.getAspects();
Set<ConfiguredTarget> allTargetsToTest = null;
if (testsToRun != null) {
@@ -617,7 +617,7 @@
private void addExtraActionsIfRequested(
AnalysisOptions viewOptions,
Collection<ConfiguredTarget> configuredTargets,
- Collection<AspectValue> aspects,
+ ImmutableMap<AspectKey, ConfiguredAspect> aspects,
ArtifactsToOwnerLabels.Builder artifactsToTopLevelLabelsMap,
ExtendedEventHandler eventHandler) {
RegexFilter filter = viewOptions.extraActionFilter;
@@ -659,21 +659,21 @@
}
}
}
- for (AspectValue aspect : aspects) {
+ for (Map.Entry<AspectKey, ConfiguredAspect> aspectEntry : aspects.entrySet()) {
ExtraActionArtifactsProvider provider =
- aspect.getConfiguredAspect().getProvider(ExtraActionArtifactsProvider.class);
+ aspectEntry.getValue().getProvider(ExtraActionArtifactsProvider.class);
if (provider != null) {
if (viewOptions.extraActionTopLevelOnly) {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getExtraActionArtifacts(),
filter,
- aspect.getLabel(),
+ aspectEntry.getKey().getLabel(),
artifactsToTopLevelLabelsMap);
} else {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getTransitiveExtraActionArtifacts(),
filter,
- aspect.getLabel(),
+ aspectEntry.getKey().getLabel(),
artifactsToTopLevelLabelsMap);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
index 29fc277..d7b6386 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
@@ -31,14 +31,9 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.packages.AspectClass;
-import com.google.devtools.build.lib.packages.AspectDescriptor;
-import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.Provider;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.Location;
import java.util.Arrays;
import java.util.Map;
import java.util.TreeMap;
@@ -60,18 +55,12 @@
* @see com.google.devtools.build.lib.packages.AspectClass
*/
@Immutable
-@AutoCodec
public final class ConfiguredAspect implements ProviderCollection {
- private final AspectDescriptor descriptor;
private final ImmutableList<ActionAnalysisMetadata> actions;
private final TransitiveInfoProviderMap providers;
- @AutoCodec.VisibleForSerialization
- ConfiguredAspect(
- AspectDescriptor descriptor,
- ImmutableList<ActionAnalysisMetadata> actions,
- TransitiveInfoProviderMap providers) {
- this.descriptor = descriptor;
+ private ConfiguredAspect(
+ ImmutableList<ActionAnalysisMetadata> actions, TransitiveInfoProviderMap providers) {
this.actions = actions;
this.providers = providers;
@@ -84,20 +73,6 @@
}
}
- /**
- * Returns the aspect name.
- */
- public String getName() {
- return descriptor.getAspectClass().getName();
- }
-
- /**
- * The aspect descriptor originating this ConfiguredAspect.
- */
- public AspectDescriptor getDescriptor() {
- return descriptor;
- }
-
public ImmutableList<ActionAnalysisMetadata> getActions() {
return actions;
}
@@ -128,19 +103,17 @@
}
public static ConfiguredAspect forAlias(ConfiguredAspect real) {
- return new ConfiguredAspect(real.descriptor, real.getActions(), real.getProviders());
+ return new ConfiguredAspect(real.getActions(), real.getProviders());
}
- public static ConfiguredAspect forNonapplicableTarget(AspectDescriptor descriptor) {
+ public static ConfiguredAspect forNonapplicableTarget() {
return new ConfiguredAspect(
- descriptor,
ImmutableList.of(),
new TransitiveInfoProviderMapBuilder().add().build());
}
- public static Builder builder(
- AspectClass aspectClass, AspectParameters parameters, RuleContext ruleContext) {
- return new Builder(aspectClass, parameters, ruleContext);
+ public static Builder builder(RuleContext ruleContext) {
+ return new Builder(ruleContext);
}
/**
@@ -151,17 +124,8 @@
new TransitiveInfoProviderMapBuilder();
private final Map<String, NestedSetBuilder<Artifact>> outputGroupBuilders = new TreeMap<>();
private final RuleContext ruleContext;
- private final AspectDescriptor descriptor;
- public Builder(
- AspectClass aspectClass,
- AspectParameters parameters,
- RuleContext context) {
- this(new AspectDescriptor(aspectClass, parameters), context);
- }
-
- public Builder(AspectDescriptor descriptor, RuleContext ruleContext) {
- this.descriptor = descriptor;
+ public Builder(RuleContext ruleContext) {
this.ruleContext = ruleContext;
}
@@ -221,12 +185,6 @@
return this;
}
- public Builder addSkylarkTransitiveInfo(String name, Object value, Location loc)
- throws EvalException {
- providers.put(name, value);
- return this;
- }
-
public Builder addSkylarkDeclaredProvider(Info declaredProvider) throws EvalException {
Provider constructor = declaredProvider.getProvider();
if (!constructor.isExported()) {
@@ -288,7 +246,6 @@
}
return new ConfiguredAspect(
- descriptor,
generatingActions.getActions(),
providers.build());
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java
new file mode 100644
index 0000000..6b15159
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java
@@ -0,0 +1,49 @@
+// Copyright 2020 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.lib.analysis;
+
+import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.skyframe.AspectFunction;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue;
+import com.google.devtools.build.skyframe.NotComparableSkyValue;
+
+/**
+ * Super-interface for {@link ConfiguredTargetValue} and {@link
+ * com.google.devtools.build.lib.analysis.AspectValue}.
+ */
+public interface ConfiguredObjectValue extends ActionLookupValue, NotComparableSkyValue {
+ /** Returns the configured target/aspect for this value. */
+ ProviderCollection getConfiguredObject();
+
+ /**
+ * Returns the set of packages transitively loaded by this value. Must only be used for
+ * constructing the package -> source root map needed for some builds. If the caller has not
+ * specified that this map needs to be constructed (via the constructor argument in {@link
+ * ConfiguredTargetFunction#ConfiguredTargetFunction} or {@link AspectFunction#AspectFunction}),
+ * calling this will crash.
+ */
+ NestedSet<Package> getTransitivePackagesForPackageRootResolution();
+
+ /**
+ * Clears provider data from this value, leaving only the artifact->generating action map.
+ *
+ * <p>Should only be used when user specifies --discard_analysis_cache. Must be called at most
+ * once per value, after which {@link #getConfiguredObject} and {@link #getActions} cannot be
+ * called.
+ */
+ void clear(boolean clearEverything);
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index efa2699..59c5526 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -20,7 +20,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.ArtifactFactory;
@@ -65,6 +64,7 @@
import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.profiler.memory.CurrentRuleTracker;
+import com.google.devtools.build.lib.skyframe.AspectValueKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.util.ClassName;
@@ -599,7 +599,7 @@
@Nullable ResolvedToolchainContext toolchainContext,
BuildConfiguration aspectConfiguration,
BuildConfiguration hostConfiguration,
- ActionLookupValue.ActionLookupKey aspectKey)
+ AspectValueKey.AspectKey aspectKey)
throws InterruptedException, ActionConflictException {
RuleContext.Builder builder =
@@ -649,10 +649,11 @@
ruleClassProvider.getToolsRepository());
if (configuredAspect != null) {
validateAdvertisedProviders(
- configuredAspect, aspect.getDefinition().getAdvertisedProviders(),
+ configuredAspect,
+ aspectKey,
+ aspect.getDefinition().getAdvertisedProviders(),
associatedTarget.getTarget(),
- env.getEventHandler()
- );
+ env.getEventHandler());
}
return configuredAspect;
}
@@ -680,34 +681,34 @@
private void validateAdvertisedProviders(
ConfiguredAspect configuredAspect,
- AdvertisedProviderSet advertisedProviders, Target target,
+ AspectValueKey.AspectKey aspectKey,
+ AdvertisedProviderSet advertisedProviders,
+ Target target,
EventHandler eventHandler) {
if (advertisedProviders.canHaveAnyProvider()) {
return;
}
for (Class<?> aClass : advertisedProviders.getNativeProviders()) {
if (configuredAspect.getProvider(aClass.asSubclass(TransitiveInfoProvider.class)) == null) {
- eventHandler.handle(Event.error(
- target.getLocation(),
- String.format(
- "Aspect '%s', applied to '%s', does not provide advertised provider '%s'",
- configuredAspect.getName(),
- target.getLabel(),
- aClass.getSimpleName()
- )));
+ eventHandler.handle(
+ Event.error(
+ target.getLocation(),
+ String.format(
+ "Aspect '%s', applied to '%s', does not provide advertised provider '%s'",
+ aspectKey.getAspectClass().getName(),
+ target.getLabel(),
+ aClass.getSimpleName())));
}
}
for (StarlarkProviderIdentifier providerId : advertisedProviders.getSkylarkProviders()) {
if (configuredAspect.get(providerId) == null) {
- eventHandler.handle(Event.error(
- target.getLocation(),
- String.format(
- "Aspect '%s', applied to '%s', does not provide advertised provider '%s'",
- configuredAspect.getName(),
- target.getLabel(),
- providerId
- )));
+ eventHandler.handle(
+ Event.error(
+ target.getLocation(),
+ String.format(
+ "Aspect '%s', applied to '%s', does not provide advertised provider '%s'",
+ aspectKey.getAspectClass().getName(), target.getLabel(), providerId)));
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
index 981166a..6cd33a3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
@@ -25,9 +25,11 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.profiler.AutoProfiler;
import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.util.RegexFilter;
import java.time.Duration;
import java.util.Collection;
+import java.util.Map;
import javax.annotation.Nullable;
/**
@@ -122,7 +124,7 @@
@VisibleForTesting
public static ArtifactsToOwnerLabels makeTopLevelArtifactsToOwnerLabels(
- AnalysisResult analysisResult, Iterable<AspectValue> aspects) {
+ AnalysisResult analysisResult) {
try (AutoProfiler ignored =
GoogleAutoProfilerUtils.logged("assigning owner labels", MIN_LOGGING)) {
ArtifactsToOwnerLabels.Builder artifactsToOwnerLabelsBuilder =
@@ -135,11 +137,12 @@
target.getLabel(),
artifactsToOwnerLabelsBuilder);
}
- for (AspectValue aspect : aspects) {
+ for (Map.Entry<AspectKey, ConfiguredAspect> aspectEntry :
+ analysisResult.getAspectsMap().entrySet()) {
addArtifactsWithOwnerLabel(
- getAllArtifactsToBuild(aspect, artifactContext).getAllArtifacts(),
+ getAllArtifactsToBuild(aspectEntry.getValue(), artifactContext).getAllArtifacts(),
null,
- aspect.getLabel(),
+ aspectEntry.getKey().getLabel(),
artifactsToOwnerLabelsBuilder);
}
if (analysisResult.getTargetsToTest() != null) {
@@ -195,11 +198,6 @@
);
}
- public static ArtifactsToBuild getAllArtifactsToBuild(
- AspectValue aspectValue, TopLevelArtifactContext context) {
- return getAllArtifactsToBuild(aspectValue.getConfiguredAspect(), context);
- }
-
static ArtifactsToBuild getAllArtifactsToBuild(
@Nullable OutputGroupInfo outputGroupInfo,
@Nullable FileProvider fileProvider,