Slight refactor so as to micro-optimize usages of AspectDefinition#visitAspectsIfRequired.
Previously, for each attribute, Attribute#getAspects was called for every label-dep entailed by the attribute's value. This is wasteful -- for list-label-valued attribute's (e.g. 'deps'), there's a lot of duplicate work here. Now, we call Attribute#getAspects exactly once per attribute.
RELNOTES: None
PiperOrigin-RevId: 235552013
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index da5161f..9cf3a1a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -172,11 +172,11 @@
return applyToFiles;
}
- /**
- * Returns the attribute -> set of labels that are provided by aspects of attribute.
- */
+ /** Returns the attribute -> set of labels that are provided by aspects of attribute. */
public static ImmutableMultimap<Attribute, Label> visitAspectsIfRequired(
- Target from, Attribute attribute, Target to,
+ Target from,
+ ImmutableList<Aspect> aspectsOfAttribute,
+ Target to,
DependencyFilter dependencyFilter) {
// Aspect can be declared only for Rules.
if (!(from instanceof Rule) || !(to instanceof Rule)) {
@@ -184,19 +184,17 @@
}
RuleClass ruleClass = ((Rule) to).getRuleClassObject();
AdvertisedProviderSet providers = ruleClass.getAdvertisedProviders();
- return visitAspectsIfRequired((Rule) from, attribute,
- providers, dependencyFilter);
+ return visitAspectsIfRequired((Rule) from, aspectsOfAttribute, providers, dependencyFilter);
}
- /**
- * Returns the attribute -> set of labels that are provided by aspects of attribute.
- */
+ /** Returns the attribute -> set of labels that are provided by aspects of attribute. */
public static ImmutableMultimap<Attribute, Label> visitAspectsIfRequired(
- Rule from, Attribute attribute,
+ Rule from,
+ ImmutableList<Aspect> aspectsOfAttribute,
AdvertisedProviderSet advertisedProviders,
DependencyFilter dependencyFilter) {
SetMultimap<Attribute, Label> result = LinkedHashMultimap.create();
- for (Aspect candidateClass : attribute.getAspects(from)) {
+ for (Aspect candidateClass : aspectsOfAttribute) {
// Check if target satisfies condition for this aspect (has to provide all required
// TransitiveInfoProviders)
RequiredProviders requiredProviders =
diff --git a/src/main/java/com/google/devtools/build/lib/query2/LabelVisitor.java b/src/main/java/com/google/devtools/build/lib/query2/LabelVisitor.java
index b65538c..4cadef9 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/LabelVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/LabelVisitor.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
+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.BuildType;
@@ -394,8 +395,13 @@
private void visitAspectsIfRequired(
Target from, Attribute attribute, final Target to, int depth, int count) {
+ // TODO(bazel-team): The getAspects call below is duplicate work for each direct dep entailed
+ // by an attribute's value. Consider doing a slight refactor where we make this method call
+ // exactly once per Attribute.
+ ImmutableList<Aspect> aspectsOfAttribute =
+ (from instanceof Rule) ? attribute.getAspects((Rule) from) : ImmutableList.of();
ImmutableMultimap<Attribute, Label> labelsFromAspects =
- AspectDefinition.visitAspectsIfRequired(from, attribute, to, edgeFilter);
+ AspectDefinition.visitAspectsIfRequired(from, aspectsOfAttribute, to, edgeFilter);
// Create an edge from target to the attribute value.
for (Map.Entry<Attribute, Label> entry : labelsFromAspects.entries()) {
enqueueTarget(from, entry.getKey(), entry.getValue(), depth, count);
diff --git a/src/main/java/com/google/devtools/build/lib/query2/output/PreciseAspectResolver.java b/src/main/java/com/google/devtools/build/lib/query2/output/PreciseAspectResolver.java
index d208ec7..8e0e8fb 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/output/PreciseAspectResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/output/PreciseAspectResolver.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.query2.output;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap;
@@ -31,7 +32,6 @@
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import java.util.LinkedHashSet;
-import java.util.Map;
import java.util.Set;
/**
@@ -54,21 +54,22 @@
DependencyFilter dependencyFilter) throws InterruptedException {
Multimap<Attribute, Label> result = LinkedListMultimap.create();
if (target instanceof Rule) {
+ Rule rule = (Rule) target;
Multimap<Attribute, Label> transitions =
- ((Rule) target).getTransitions(DependencyFilter.NO_NODEP_ATTRIBUTES);
- for (Map.Entry<Attribute, Label> entry : transitions.entries()) {
- Target toTarget;
- try {
- toTarget = packageProvider.getTarget(eventHandler, entry.getValue());
- result.putAll(
- AspectDefinition.visitAspectsIfRequired(
- target,
- entry.getKey(),
- toTarget,
- dependencyFilter));
- } catch (NoSuchThingException e) {
- // Do nothing. One of target direct deps has an error. The dependency on the BUILD file
- // (or one of the files included in it) will be reported in the query result of :BUILD.
+ rule.getTransitions(DependencyFilter.NO_NODEP_ATTRIBUTES);
+ for (Attribute attribute : transitions.keySet()) {
+ ImmutableList<Aspect> aspects = attribute.getAspects(rule);
+ for (Label toLabel : transitions.get(attribute)) {
+ Target toTarget;
+ try {
+ toTarget = packageProvider.getTarget(eventHandler, toLabel);
+ result.putAll(
+ AspectDefinition.visitAspectsIfRequired(
+ target, aspects, toTarget, dependencyFilter));
+ } catch (NoSuchThingException e) {
+ // Do nothing. One of target direct deps has an error. The dependency on the BUILD file
+ // (or one of the files included in it) will be reported in the query result of :BUILD.
+ }
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
index ce5ed0c..8e10829 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
@@ -16,12 +16,15 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.DependencyFilter;
@@ -183,24 +186,34 @@
Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
Environment env)
throws InterruptedException {
- List<SkyKey> depKeys = Lists.newArrayList();
- if (target instanceof Rule) {
- Map<Label, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> labelDepMap =
- new HashMap<>(depMap.size());
- for (Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>
- entry : depMap.entrySet()) {
- labelDepMap.put(argumentFromKey(entry.getKey()), entry.getValue());
- }
+ if (!(target instanceof Rule)) {
+ return ImmutableList.of();
+ }
- Multimap<Attribute, Label> transitions =
- ((Rule) target).getTransitions(DependencyFilter.NO_NODEP_ATTRIBUTES);
- for (Map.Entry<Attribute, Label> entry : transitions.entries()) {
+ Rule rule = (Rule) target;
+ Map<Label, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> labelDepMap =
+ new HashMap<>(depMap.size());
+ for (Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> entry :
+ depMap.entrySet()) {
+ labelDepMap.put(argumentFromKey(entry.getKey()), entry.getValue());
+ }
+
+ Map<String, ImmutableList<Aspect>> aspectsByAttribute =
+ Maps.newHashMapWithExpectedSize(rule.getAttributes().size());
+ for (Attribute attribute : rule.getAttributes()) {
+ aspectsByAttribute.put(attribute.getName(), attribute.getAspects(rule));
+ }
+
+ List<SkyKey> depKeys = Lists.newArrayList();
+ Multimap<Attribute, Label> transitions =
+ rule.getTransitions(DependencyFilter.NO_NODEP_ATTRIBUTES);
+ for (Attribute attribute : transitions.keySet()) {
+ ImmutableList<Aspect> aspects = attribute.getAspects(rule);
+ for (Label toLabel : transitions.get(attribute)) {
ValueOrException2<NoSuchPackageException, NoSuchTargetException> value =
- labelDepMap.get(entry.getValue());
- for (Label label :
- getAspectLabels((Rule) target, entry.getKey(), entry.getValue(), value, env)) {
- depKeys.add(getKey(label));
- }
+ labelDepMap.get(toLabel);
+ getAspectLabels(rule, aspects, toLabel, value, env)
+ .forEach(aspectLabel -> depKeys.add(getKey(aspectLabel)));
}
}
return depKeys;
@@ -209,7 +222,7 @@
/** Get the Aspect-related Label deps for the given edge. */
protected abstract Collection<Label> getAspectLabels(
Rule fromRule,
- Attribute attr,
+ ImmutableList<Aspect> aspectsOfAttribute,
Label toLabel,
ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
Environment env)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index dc415df..7d67c21 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -25,6 +25,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
+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.ConfigurationFragmentPolicy;
@@ -265,7 +266,7 @@
@Override
protected Collection<Label> getAspectLabels(
Rule fromRule,
- Attribute attr,
+ ImmutableList<Aspect> aspectsOfAttribute,
Label toLabel,
ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
final Environment env)
@@ -284,8 +285,9 @@
return ImmutableList.of();
}
Target dependedTarget = pkgValue.getPackage().getTarget(toLabel.getName());
- return AspectDefinition.visitAspectsIfRequired(fromRule, attr, dependedTarget,
- DependencyFilter.ALL_DEPS).values();
+ return AspectDefinition.visitAspectsIfRequired(
+ fromRule, aspectsOfAttribute, dependedTarget, DependencyFilter.ALL_DEPS)
+ .values();
} catch (NoSuchThingException e) {
// Do nothing. This error was handled when we computed the corresponding
// TransitiveTargetValue.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
index fcda217..96cee0f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -18,8 +18,8 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.EventHandler;
+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.DependencyFilter;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -92,7 +92,7 @@
@Override
protected Collection<Label> getAspectLabels(
Rule fromRule,
- Attribute attr,
+ ImmutableList<Aspect> aspectsOfAttribute,
Label toLabel,
ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
Environment env) {
@@ -106,9 +106,9 @@
}
// Retrieve the providers of the dep from the TransitiveTraversalValue, so we can avoid
// issuing a dep on its defining Package.
- return AspectDefinition.visitAspectsIfRequired(fromRule, attr,
- traversalVal.getProviders(),
- DependencyFilter.ALL_DEPS).values();
+ return AspectDefinition.visitAspectsIfRequired(
+ fromRule, aspectsOfAttribute, traversalVal.getProviders(), DependencyFilter.ALL_DEPS)
+ .values();
} catch (NoSuchThingException e) {
// Do nothing. This error was handled when we computed the corresponding
// TransitiveTargetValue.
