Query performance: Skip aspect handling if none
Don't collect or try to convert aspect information if the rule class
doesn't have any aspects. To that end, keep track of which rule classes
have aspects and which don't.
It makes only a small difference in performance, but I think it's worth
it. I measure an improvement of a 2-3% on a query with a large proto
output.
PiperOrigin-RevId: 291144350
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index d52eae2..d91b05a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -2177,10 +2177,17 @@
return allowedValues;
}
+ public boolean hasAspects() {
+ return !aspects.isEmpty();
+ }
+
/**
* Returns the list of aspects required for dependencies through this attribute.
*/
public ImmutableList<Aspect> getAspects(Rule rule) {
+ if (aspects.isEmpty()) {
+ return ImmutableList.of();
+ }
ImmutableList.Builder<Aspect> builder = null;
for (RuleAspect<?> aspect : aspects) {
Aspect a = aspect.getAspect(rule);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 33f47ff..28f0df3 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -203,6 +203,10 @@
return containsErrors;
}
+ public boolean hasAspects() {
+ return ruleClass.hasAspects();
+ }
+
/**
* Returns an (unmodifiable, unordered) collection containing all the
* Attribute definitions for this kind of rule. (Note, this doesn't include
@@ -683,6 +687,9 @@
* can require from its direct dependencies.
*/
public Collection<? extends Label> getAspectLabelsSuperset(DependencyFilter predicate) {
+ if (!hasAspects()) {
+ return ImmutableList.of();
+ }
SetMultimap<Attribute, Label> labels = LinkedHashMultimap.create();
for (Attribute attribute : this.getAttributes()) {
for (Aspect candidateClass : attribute.getAspects(this)) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 29a0e35..1531c8a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -31,6 +31,7 @@
import com.google.common.collect.Interner;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
@@ -1392,6 +1393,7 @@
private final boolean hasAnalysisTestTransition;
private final boolean hasFunctionTransitionWhitelist;
private final boolean ignoreLicenses;
+ private final boolean hasAspects;
/**
* A (unordered) mapping from attribute names to small integers indexing into
@@ -1581,14 +1583,17 @@
// Create the index and collect non-configurable attributes.
int index = 0;
- attributeIndex = new HashMap<>(attributes.size());
+ attributeIndex = Maps.newHashMapWithExpectedSize(attributes.size());
+ boolean computedHasAspects = false;
ImmutableList.Builder<String> nonConfigurableAttributesBuilder = ImmutableList.builder();
for (Attribute attribute : attributes) {
+ computedHasAspects |= attribute.hasAspects();
attributeIndex.put(attribute.getName(), index++);
if (!attribute.isConfigurable()) {
nonConfigurableAttributesBuilder.add(attribute.getName());
}
}
+ this.hasAspects = computedHasAspects;
this.nonConfigurableAttributes = nonConfigurableAttributesBuilder.build();
}
@@ -1773,6 +1778,10 @@
return supportsConstraintChecking;
}
+ public boolean hasAspects() {
+ return hasAspects;
+ }
+
/**
* Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
* associated with {@code pkgBuilder}.
@@ -2308,32 +2317,34 @@
private static void checkAspectAllowedValues(
Rule rule, EventHandler eventHandler) {
- for (Attribute attrOfRule : rule.getAttributes()) {
- for (Aspect aspect : attrOfRule.getAspects(rule)) {
- for (Attribute attrOfAspect : aspect.getDefinition().getAttributes().values()) {
- // By this point the AspectDefinition has been created and values assigned.
- if (attrOfAspect.checkAllowedValues()) {
- PredicateWithMessage<Object> allowedValues = attrOfAspect.getAllowedValues();
- Object value = attrOfAspect.getDefaultValue(rule);
- if (!allowedValues.apply(value)) {
- if (RawAttributeMapper.of(rule).isConfigurable(attrOfAspect.getName())) {
- rule.reportError(
- String.format(
- "%s: attribute '%s' has a select() and aspect %s also declares "
- + "'%s'. Aspect attributes don't currently support select().",
- rule.getLabel(),
- attrOfAspect.getName(),
- aspect.getDefinition().getName(),
- rule.getLabel()),
- eventHandler);
- } else {
- rule.reportError(
- String.format(
- "%s: invalid value in '%s' attribute: %s",
- rule.getLabel(),
- attrOfAspect.getName(),
- allowedValues.getErrorReason(value)),
- eventHandler);
+ if (rule.hasAspects()) {
+ for (Attribute attrOfRule : rule.getAttributes()) {
+ for (Aspect aspect : attrOfRule.getAspects(rule)) {
+ for (Attribute attrOfAspect : aspect.getDefinition().getAttributes().values()) {
+ // By this point the AspectDefinition has been created and values assigned.
+ if (attrOfAspect.checkAllowedValues()) {
+ PredicateWithMessage<Object> allowedValues = attrOfAspect.getAllowedValues();
+ Object value = attrOfAspect.getDefaultValue(rule);
+ if (!allowedValues.apply(value)) {
+ if (RawAttributeMapper.of(rule).isConfigurable(attrOfAspect.getName())) {
+ rule.reportError(
+ String.format(
+ "%s: attribute '%s' has a select() and aspect %s also declares "
+ + "'%s'. Aspect attributes don't currently support select().",
+ rule.getLabel(),
+ attrOfAspect.getName(),
+ aspect.getDefinition().getName(),
+ rule.getLabel()),
+ eventHandler);
+ } else {
+ rule.reportError(
+ String.format(
+ "%s: invalid value in '%s' attribute: %s",
+ rule.getLabel(),
+ attrOfAspect.getName(),
+ allowedValues.getErrorReason(value)),
+ eventHandler);
+ }
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/ConservativeAspectResolver.java b/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/ConservativeAspectResolver.java
index e62ebd2..2137041 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/ConservativeAspectResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/ConservativeAspectResolver.java
@@ -40,6 +40,9 @@
return ImmutableMultimap.of();
}
Rule rule = (Rule) target;
+ if (!rule.hasAspects()) {
+ return ImmutableMultimap.of();
+ }
Multimap<Attribute, Label> result = LinkedHashMultimap.create();
for (Attribute attribute : rule.getAttributes()) {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/PreciseAspectResolver.java b/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/PreciseAspectResolver.java
index a7437e6..407df98 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/PreciseAspectResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers/PreciseAspectResolver.java
@@ -51,17 +51,21 @@
@Override
public ImmutableMultimap<Attribute, Label> computeAspectDependencies(
Target target, DependencyFilter dependencyFilter) throws InterruptedException {
+ if (!(target instanceof Rule)) {
+ return ImmutableMultimap.of();
+ }
+ Rule rule = (Rule) target;
+ if (!rule.hasAspects()) {
+ return ImmutableMultimap.of();
+ }
Multimap<Attribute, Label> result = LinkedListMultimap.create();
- if (target instanceof Rule) {
- Rule rule = (Rule) target;
- Multimap<Attribute, Label> transitions =
- rule.getTransitions(DependencyFilter.NO_NODEP_ATTRIBUTES);
- for (Attribute attribute : transitions.keySet()) {
- for (Aspect aspect : attribute.getAspects(rule)) {
- if (hasDepThatSatisfies(aspect, transitions.get(attribute))) {
- AspectDefinition.forEachLabelDepFromAllAttributesOfAspect(
- rule, aspect, dependencyFilter, result::put);
- }
+ Multimap<Attribute, Label> transitions =
+ rule.getTransitions(DependencyFilter.NO_NODEP_ATTRIBUTES);
+ for (Attribute attribute : transitions.keySet()) {
+ for (Aspect aspect : attribute.getAspects(rule)) {
+ if (hasDepThatSatisfies(aspect, transitions.get(attribute))) {
+ AspectDefinition.forEachLabelDepFromAllAttributesOfAspect(
+ rule, aspect, dependencyFilter, result::put);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
index b4822ba..23db2a6 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
@@ -188,38 +188,39 @@
ImmutableMultimap<Attribute, Label> aspectsDependencies =
aspectResolver.computeAspectDependencies(target, dependencyFilter);
- // Add information about additional attributes from aspects.
- List<Build.Attribute> attributes = new ArrayList<>(aspectsDependencies.asMap().size());
- for (Map.Entry<Attribute, Collection<Label>> entry : aspectsDependencies.asMap().entrySet()) {
- Attribute attribute = entry.getKey();
- Collection<Label> labels = entry.getValue();
- if (!includeAspectAttribute(attribute, labels)) {
- continue;
+ if (!aspectsDependencies.isEmpty()) {
+ // Add information about additional attributes from aspects.
+ List<Build.Attribute> attributes = new ArrayList<>(aspectsDependencies.asMap().size());
+ for (Map.Entry<Attribute, Collection<Label>> entry :
+ aspectsDependencies.asMap().entrySet()) {
+ Attribute attribute = entry.getKey();
+ Collection<Label> labels = entry.getValue();
+ if (!includeAspectAttribute(attribute, labels)) {
+ continue;
+ }
+ Object attributeValue = getAspectAttributeValue(attribute, labels);
+ Build.Attribute serializedAttribute =
+ AttributeFormatter.getAttributeProto(
+ attribute,
+ attributeValue,
+ /*explicitlySpecified=*/ false,
+ /*encodeBooleanAndTriStateAsIntegerAndString=*/ true);
+ attributes.add(serializedAttribute);
}
- Object attributeValue = getAspectAttributeValue(attribute, labels);
- Build.Attribute serializedAttribute =
- AttributeFormatter.getAttributeProto(
- attribute,
- attributeValue,
- /*explicitlySpecified=*/ false,
- /*encodeBooleanAndTriStateAsIntegerAndString=*/ true);
- attributes.add(serializedAttribute);
+ rulePb.addAllAttribute(
+ attributes.stream().distinct().sorted(ATTRIBUTE_NAME).collect(Collectors.toList()));
}
- rulePb.addAllAttribute(
- attributes.stream().distinct().sorted(ATTRIBUTE_NAME).collect(Collectors.toList()));
if (includeRuleInputsAndOutputs) {
// Add all deps from aspects as rule inputs of current target.
- aspectsDependencies
- .values()
- .stream()
- .distinct()
- .forEach(dep -> rulePb.addRuleInput(dep.toString()));
- // Include explicit elements for all direct inputs and outputs of a rule;
- // this goes beyond what is available from the attributes above, since it
- // may also (depending on options) include implicit outputs,
- // host-configuration outputs, and default values.
- rule.getLabels(dependencyFilter)
- .stream()
+ if (!aspectsDependencies.isEmpty()) {
+ aspectsDependencies.values().stream()
+ .distinct()
+ .forEach(dep -> rulePb.addRuleInput(dep.toString()));
+ }
+ // Include explicit elements for all direct inputs and outputs of a rule; this goes beyond
+ // what is available from the attributes above, since it may also (depending on options)
+ // include implicit outputs, host-configuration outputs, and default values.
+ rule.getLabels(dependencyFilter).stream()
.distinct()
.forEach(input -> rulePb.addRuleInput(input.toString()));
rule.getOutputFiles()
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 e06d4f3..9649e57 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
@@ -181,6 +181,9 @@
}
Rule rule = (Rule) target;
+ if (!rule.hasAspects()) {
+ return ImmutableList.of();
+ }
List<SkyKey> depKeys = Lists.newArrayList();
Multimap<Attribute, Label> transitions =
