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/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);
+ }
}
}
}
