Restrict aspects visible to other aspects according to their advertised providers.
--
PiperOrigin-RevId: 147526961
MOS_MIGRATED_REVID=147526961
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 4f8835b..3d5aa88 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -34,6 +34,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.AspectDescriptor;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NativeAspectClass;
@@ -60,6 +61,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
@@ -218,16 +220,18 @@
ImmutableList.Builder<Aspect> aspectPathBuilder = ImmutableList.builder();
- if (key.getBaseKey() != null) {
- ImmutableList<SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKey());
+ if (!key.getBaseKeys().isEmpty()) {
+ // We transitively collect all required aspects to reduce the number of restarts.
+ // Semantically it is enough to just request key.getBaseKeys().
+ ImmutableMap<AspectDescriptor, SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKeys());
- Map<SkyKey, SkyValue> values = env.getValues(aspectKeys);
+ Map<SkyKey, SkyValue> values = env.getValues(aspectKeys.values());
if (env.valuesMissing()) {
return null;
}
try {
associatedTarget = getBaseTargetAndCollectPath(
- associatedTarget, aspectKeys, values, aspectPathBuilder);
+ associatedTarget, key.getBaseKeys(), values, aspectPathBuilder);
} catch (DuplicateException e) {
env.getListener().handle(
Event.error(associatedTarget.getTarget().getLocation(), e.getMessage()));
@@ -321,11 +325,13 @@
* @throws DuplicateException if there is a duplicate provider provided by aspects.
*/
private ConfiguredTarget getBaseTargetAndCollectPath(ConfiguredTarget target,
- ImmutableList<SkyKey> aspectKeys, Map<SkyKey, SkyValue> values,
+ ImmutableList<AspectKey> aspectKeys,
+ Map<SkyKey, SkyValue> values,
ImmutableList.Builder<Aspect> aspectPath)
throws DuplicateException {
ArrayList<ConfiguredAspect> aspectValues = new ArrayList<>();
- for (SkyKey skyAspectKey : aspectKeys) {
+ for (AspectKey aspectKey : aspectKeys) {
+ SkyKey skyAspectKey = aspectKey.getSkyKey();
AspectValue aspectValue = (AspectValue) values.get(skyAspectKey);
ConfiguredAspect configuredAspect = aspectValue.getConfiguredAspect();
aspectValues.add(configuredAspect);
@@ -335,19 +341,28 @@
}
/**
- * Returns a list of SkyKeys for all aspects the given aspect key depends on.
- * The order corresponds to the order the aspects should be merged into a configured target.
+ * Collect all SkyKeys that are needed for a given list of AspectKeys,
+ * including transitive dependencies.
*/
- private ImmutableList<SkyKey> getSkyKeysForAspects(AspectKey key) {
- ImmutableList.Builder<SkyKey> aspectKeysBuilder = ImmutableList.builder();
- AspectKey baseKey = key;
- while (baseKey != null) {
- aspectKeysBuilder.add(baseKey.getSkyKey());
- baseKey = baseKey.getBaseKey();
+ private ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspects(
+ ImmutableList<AspectKey> keys) {
+ HashMap<AspectDescriptor, SkyKey> result = new HashMap<>();
+ for (AspectKey key : keys) {
+ buildSkyKeys(key, result);
}
- return aspectKeysBuilder.build().reverse();
+ return ImmutableMap.copyOf(result);
}
+ private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> result) {
+ if (result.containsKey(key.getAspectDescriptor())) {
+ return;
+ }
+ ImmutableList<AspectKey> baseKeys = key.getBaseKeys();
+ result.put(key.getAspectDescriptor(), key.getSkyKey());
+ for (AspectKey baseKey : baseKeys) {
+ buildSkyKeys(baseKey, result);
+ }
+ }
private static SkyValue createAliasAspect(
Environment env,
Target originalTarget,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 7accbc1..df6048d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -47,36 +48,22 @@
*/
public static final class AspectKey extends AspectValueKey {
private final Label label;
- private final AspectKey baseKey;
+ private final ImmutableList<AspectKey> baseKeys;
private final BuildConfiguration aspectConfiguration;
private final BuildConfiguration baseConfiguration;
- private final AspectClass aspectClass;
- private final AspectParameters parameters;
+ private final AspectDescriptor aspectDescriptor;
private AspectKey(
Label label,
- BuildConfiguration aspectConfiguration,
BuildConfiguration baseConfiguration,
- AspectClass aspectClass,
- AspectParameters parameters) {
- this.baseKey = null;
- this.label = label;
- this.aspectConfiguration = aspectConfiguration;
- this.baseConfiguration = baseConfiguration;
- this.aspectClass = aspectClass;
- this.parameters = parameters;
- }
-
- private AspectKey(
- AspectKey baseKey,
- AspectClass aspectClass, AspectParameters aspectParameters,
+ ImmutableList<AspectKey> baseKeys,
+ AspectDescriptor aspectDescriptor,
BuildConfiguration aspectConfiguration) {
- this.baseKey = baseKey;
- this.label = baseKey.label;
- this.baseConfiguration = baseKey.getBaseConfiguration();
+ this.baseKeys = baseKeys;
+ this.label = label;
+ this.baseConfiguration = baseConfiguration;
this.aspectConfiguration = aspectConfiguration;
- this.aspectClass = aspectClass;
- this.parameters = aspectParameters;
+ this.aspectDescriptor = aspectDescriptor;
}
@Override
@@ -91,25 +78,31 @@
}
public AspectClass getAspectClass() {
- return aspectClass;
+ return aspectDescriptor.getAspectClass();
}
@Nullable
public AspectParameters getParameters() {
- return parameters;
+ return aspectDescriptor.getParameters();
+ }
+
+ public AspectDescriptor getAspectDescriptor() {
+ return aspectDescriptor;
}
@Nullable
- public AspectKey getBaseKey() {
- return baseKey;
+ public ImmutableList<AspectKey> getBaseKeys() {
+ return baseKeys;
}
@Override
public String getDescription() {
- if (baseKey == null) {
- return String.format("%s of %s", aspectClass.getName(), getLabel());
+ if (baseKeys.isEmpty()) {
+ return String.format("%s of %s",
+ aspectDescriptor.getAspectClass().getName(), getLabel());
} else {
- return String.format("%s on top of %s", aspectClass.getName(), baseKey.toString());
+ return String.format("%s on top of %s",
+ aspectDescriptor.getAspectClass().getName(), baseKeys.toString());
}
}
@@ -153,11 +146,10 @@
public int hashCode() {
return Objects.hashCode(
label,
- baseKey,
+ baseKeys,
aspectConfiguration,
baseConfiguration,
- aspectClass,
- parameters);
+ aspectDescriptor);
}
@Override
@@ -172,45 +164,52 @@
AspectKey that = (AspectKey) other;
return Objects.equal(label, that.label)
- && Objects.equal(baseKey, that.baseKey)
+ && Objects.equal(baseKeys, that.baseKeys)
&& Objects.equal(aspectConfiguration, that.aspectConfiguration)
&& Objects.equal(baseConfiguration, that.baseConfiguration)
- && Objects.equal(aspectClass, that.aspectClass)
- && Objects.equal(parameters, that.parameters);
+ && Objects.equal(aspectDescriptor, that.aspectDescriptor);
}
public String prettyPrint() {
if (label == null) {
return "null";
}
- return String.format("%s with aspect %s%s",
- baseKey == null ? label.toString() : baseKey.prettyPrint(),
- aspectClass.getName(),
+
+ String baseKeysString =
+ baseKeys.isEmpty()
+ ? ""
+ : String.format(" (over %s)", baseKeys.toString());
+ return String.format("%s with aspect %s%s%s",
+ label.toString(),
+ aspectDescriptor.getAspectClass().getName(),
(aspectConfiguration != null && aspectConfiguration.isHostConfiguration())
- ? "(host) " : "");
+ ? "(host) " : "",
+ baseKeysString
+ );
}
@Override
public String toString() {
- return (baseKey == null ? label : baseKey.toString())
+ return (baseKeys == null ? label : baseKeys.toString())
+ "#"
- + aspectClass.getName()
+ + aspectDescriptor.getAspectClass().getName()
+ " "
+ (aspectConfiguration == null ? "null" : aspectConfiguration.checksum())
+ " "
+ (baseConfiguration == null ? "null" : baseConfiguration.checksum())
+ " "
- + parameters;
+ + aspectDescriptor.getParameters();
}
public AspectKey withLabel(Label label) {
- if (baseKey == null) {
- return new AspectKey(
- label, aspectConfiguration, baseConfiguration, aspectClass, parameters);
- } else {
- return new AspectKey(
- baseKey.withLabel(label), aspectClass, parameters, aspectConfiguration);
+ ImmutableList.Builder<AspectKey> newBaseKeys = ImmutableList.builder();
+ for (AspectKey baseKey : baseKeys) {
+ newBaseKeys.add(baseKey.withLabel(label));
}
+
+ return new AspectKey(
+ label, baseConfiguration,
+ newBaseKeys.build(), aspectDescriptor, aspectConfiguration);
}
}
@@ -354,10 +353,14 @@
return transitivePackages;
}
- public static AspectKey createAspectKey(AspectKey baseKey, AspectDescriptor aspectDescriptor,
+ public static AspectKey createAspectKey(
+ Label label, BuildConfiguration baseConfiguration,
+ ImmutableList<AspectKey> baseKeys, AspectDescriptor aspectDescriptor,
BuildConfiguration aspectConfiguration) {
return new AspectKey(
- baseKey, aspectDescriptor.getAspectClass(), aspectDescriptor.getParameters(),
+ label, baseConfiguration,
+ baseKeys,
+ aspectDescriptor,
aspectConfiguration
);
}
@@ -368,8 +371,8 @@
BuildConfiguration baseConfiguration, AspectDescriptor aspectDescriptor,
BuildConfiguration aspectConfiguration) {
return new AspectKey(
- label, aspectConfiguration, baseConfiguration,
- aspectDescriptor.getAspectClass(), aspectDescriptor.getParameters());
+ label, baseConfiguration, ImmutableList.<AspectKey>of(),
+ aspectDescriptor, aspectConfiguration);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index e481dbd..673a18f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -31,6 +31,8 @@
import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
+import com.google.devtools.build.lib.analysis.AspectCollection;
+import com.google.devtools.build.lib.analysis.AspectCollection.AspectDeps;
import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -82,6 +84,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
@@ -849,18 +852,14 @@
NestedSetBuilder<Package> transitivePackages)
throws AspectCreationException, InterruptedException {
OrderedSetMultimap<SkyKey, ConfiguredAspect> result = OrderedSetMultimap.create();
- Set<SkyKey> aspectKeys = new HashSet<>();
+ Set<SkyKey> allAspectKeys = new HashSet<>();
for (Dependency dep : deps) {
- AspectKey key = null;
- for (Entry<AspectDescriptor, BuildConfiguration> depAspect
- : dep.getAspectConfigurations().entrySet()) {
- key = getNextAspectKey(key, dep, depAspect);
- aspectKeys.add(key.getSkyKey());
- }
+ allAspectKeys.addAll(getAspectKeys(dep).values());
}
Map<SkyKey, ValueOrException2<AspectCreationException, NoSuchThingException>> depAspects =
- env.getValuesOrThrow(aspectKeys, AspectCreationException.class, NoSuchThingException.class);
+ env.getValuesOrThrow(allAspectKeys,
+ AspectCreationException.class, NoSuchThingException.class);
for (Dependency dep : deps) {
SkyKey depKey = TO_KEYS.apply(dep);
@@ -869,12 +868,12 @@
if (result.containsKey(depKey)) {
continue;
}
- AspectKey key = null;
+ Map<AspectDescriptor, SkyKey> aspectToKeys = getAspectKeys(dep);
+
ConfiguredTarget depConfiguredTarget = configuredTargetMap.get(depKey);
- for (Entry<AspectDescriptor, BuildConfiguration> depAspect
- : dep.getAspectConfigurations().entrySet()) {
- key = getNextAspectKey(key, dep, depAspect);
- SkyKey aspectKey = key.getSkyKey();
+ for (AspectDeps depAspect : dep.getAspects().getVisibleAspects()) {
+ SkyKey aspectKey = aspectToKeys.get(depAspect.getAspect());
+
AspectValue aspectValue;
try {
// TODO(ulfjack): Catch all thrown AspectCreationException and NoSuchThingException
@@ -884,7 +883,7 @@
throw new AspectCreationException(
String.format(
"Evaluation of aspect %s on %s failed: %s",
- depAspect.getKey().getAspectClass().getName(),
+ depAspect.getAspect().getAspectClass().getName(),
dep.getLabel(),
e.toString()));
}
@@ -904,16 +903,32 @@
return result;
}
- private static AspectKey getNextAspectKey(AspectKey key, Dependency dep,
- Entry<AspectDescriptor, BuildConfiguration> depAspect) {
- if (key == null) {
- key = AspectValue.createAspectKey(dep.getLabel(),
- dep.getConfiguration(), depAspect.getKey(), depAspect.getValue()
- );
- } else {
- key = AspectValue.createAspectKey(key, depAspect.getKey(), depAspect.getValue());
+ private static Map<AspectDescriptor, SkyKey> getAspectKeys(Dependency dep) {
+ HashMap<AspectDescriptor, SkyKey> result = new HashMap<>();
+ AspectCollection aspects = dep.getAspects();
+ for (AspectDeps aspectDeps : aspects.getVisibleAspects()) {
+ buildAspectKey(aspectDeps, result, dep);
}
- return key;
+ return result;
+ }
+
+ private static AspectKey buildAspectKey(AspectDeps aspectDeps,
+ HashMap<AspectDescriptor, SkyKey> result, Dependency dep) {
+ if (result.containsKey(aspectDeps.getAspect())) {
+ return (AspectKey) result.get(aspectDeps.getAspect()).argument();
+ }
+
+ ImmutableList.Builder<AspectKey> dependentAspects = ImmutableList.builder();
+ for (AspectDeps path : aspectDeps.getDependentAspects()) {
+ dependentAspects.add(buildAspectKey(path, result, dep));
+ }
+ AspectKey aspectKey = AspectValue.createAspectKey(
+ dep.getLabel(), dep.getConfiguration(),
+ dependentAspects.build(),
+ aspectDeps.getAspect(),
+ dep.getAspectConfiguration(aspectDeps.getAspect()));
+ result.put(aspectKey.getAspectDescriptor(), aspectKey.getSkyKey());
+ return aspectKey;
}
private static boolean aspectMatchesConfiguredTarget(final ConfiguredTarget dep, Aspect aspect) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 10e5657..46b2b9d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -50,6 +50,7 @@
import com.google.devtools.build.lib.actions.PackageRootResolutionException;
import com.google.devtools.build.lib.actions.ResourceManager;
import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.analysis.AspectCollection;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.BuildView.Options;
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
@@ -1227,7 +1228,7 @@
}
for (BuildConfiguration depConfig : configs.get(key)) {
skyKeys.add(ConfiguredTargetValue.key(key.getLabel(), depConfig));
- for (AspectDescriptor aspectDescriptor : key.getAspects()) {
+ for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) {
skyKeys.add(ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(), depConfig,
aspectDescriptor, depConfig)));
}
@@ -1260,7 +1261,7 @@
((ConfiguredTargetValue) result.get(configuredTargetKey)).getConfiguredTarget();
List<ConfiguredAspect> configuredAspects = new ArrayList<>();
- for (AspectDescriptor aspectDescriptor : key.getAspects()) {
+ for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) {
SkyKey aspectKey = ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(),
depConfig, aspectDescriptor, depConfig));
if (result.get(aspectKey) == null) {
@@ -1450,7 +1451,7 @@
dep = Dependency.withNullConfiguration(label);
} else if (configuration.useDynamicConfigurations()) {
dep = Dependency.withTransitionAndAspects(label, Attribute.ConfigurationTransition.NONE,
- ImmutableSet.<AspectDescriptor>of());
+ AspectCollection.EMPTY);
} else {
dep = Dependency.withConfiguration(label, configuration);
}
