From a936f04c560e10ed1eae8c105311b4e2f85d54a2 Mon Sep 17 00:00:00 2001 From: Luca Di Grazia Date: Sun, 4 Sep 2022 15:58:26 +0200 Subject: [PATCH] Clean up `createMapBackedAttributeMap` a bit. Use `DelegatingAttributeMapper`, try to avoid double map lookups, and pre-size where possible. PiperOrigin-RevId: 382722014 --- .../packages/AggregatingAttributeMapper.java | 620 ++++++++++++++---- 1 file changed, 501 insertions(+), 119 deletions(-) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java index 2aef2242bb5..5c8fa74daaa 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java @@ -1,4 +1,4 @@ -// Copyright 2014 Google Inc. All rights reserved. +// Copyright 2014 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. @@ -13,116 +13,346 @@ // limitations under the License. package com.google.devtools.build.lib.packages; -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.syntax.Label; +import static com.google.common.base.Preconditions.checkArgument; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.collect.CollectionUtils; +import com.google.devtools.build.lib.packages.Attribute.ComputationLimiter; +import com.google.devtools.build.lib.packages.Attribute.ComputedDefault; +import com.google.devtools.build.lib.packages.BuildType.Selector; +import com.google.devtools.build.lib.packages.BuildType.SelectorList; +import com.google.devtools.build.lib.packages.Type.LabelClass; +import com.google.devtools.build.lib.packages.Type.ListType; +import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Collection; +import java.util.Deque; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; - +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.Nullable; /** - * {@link AttributeMap} implementation that provides the ability to retrieve *all possible* + * {@link AttributeMap} implementation that provides the ability to retrieve all possible * values an attribute might take. */ public class AggregatingAttributeMapper extends AbstractAttributeMapper { + private AggregatingAttributeMapper(Rule rule) { + super(rule); + } + + public static AggregatingAttributeMapper of(Rule rule) { + return new AggregatingAttributeMapper(rule); + } + /** - * Store for all of this rule's attributes that are non-configurable. These are - * unconditionally available to computed defaults no matter what dependencies - * they've declared. + * Returns all of this rule's attributes that are non-configurable. These are unconditionally + * available to computed defaults no matter what dependencies they've declared. */ - private final List nonconfigurableAttributes; + private List getNonConfigurableAttributes() { + return rule.getRuleClassObject().getNonConfigurableAttributes(); + } - private AggregatingAttributeMapper(Rule rule) { - super(rule.getPackage(), rule.getRuleClassObject(), rule.getLabel(), - rule.getAttributeContainer()); + /** + * Override that also visits the rule's configurable attribute keys (which are themselves labels). + * + *

This method directly parses each selector, vs. calling {@link #visitAttribute} to iterate + * over all possible values. The latter has dangerous efficiency consequences, as discussed in + * {@link #visitAttribute}'s documentation. So we want to avoid that code path when possible. + */ + @Override + void visitLabels(Attribute attribute, Type type, Type.LabelVisitor visitor) { + visitLabels(attribute, type, /*includeSelectKeys=*/ true, visitor); + } - ImmutableList.Builder nonconfigurableAttributesBuilder = ImmutableList.builder(); - for (Attribute attr : rule.getAttributes()) { - if (!attr.isConfigurable()) { - nonconfigurableAttributesBuilder.add(attr.getName()); + @SuppressWarnings("unchecked") + private void visitLabels( + Attribute attribute, Type type, boolean includeSelectKeys, Type.LabelVisitor visitor) { + String name = attribute.getName(); + + // The only way for LabelClass.NONE to contain labels is in select keys. + if (type.getLabelClass() == LabelClass.NONE) { + if (includeSelectKeys && attribute.isConfigurable()) { + SelectorList selectorList = getSelectorList(name, type); + if (selectorList != null) { + visitLabelsInSelect( + selectorList, + attribute, + type, + visitor, + /*includeKeys=*/ true, + /*includeValues=*/ false); + } + } + return; + } + + Object rawVal = rule.getAttr(name, type); + if (rawVal instanceof SelectorList) { + visitLabelsInSelect( + (SelectorList) rawVal, + attribute, + type, + visitor, + includeSelectKeys, + /*includeValues=*/ true); + } else if (rawVal instanceof ComputedDefault) { + // Computed defaults are a special pain: we have no choice but to iterate through their + // (computed) values and look for labels. + for (T value : ((ComputedDefault) rawVal).getPossibleValues(type, rule)) { + if (value != null) { + type.visitLabels(visitor, value, attribute); + } + } + } else { + T value = getFromRawAttributeValue(rawVal, name, type); + if (value != null) { + type.visitLabels(visitor, value, attribute); } } - nonconfigurableAttributes = nonconfigurableAttributesBuilder.build(); } - public static AggregatingAttributeMapper of(Rule rule) { - return new AggregatingAttributeMapper(rule); + private static void visitLabelsInSelect( + SelectorList selectorList, + Attribute attribute, + Type type, + Type.LabelVisitor visitor, + boolean includeKeys, + boolean includeValues) { + for (Selector selector : selectorList.getSelectors()) { + for (Map.Entry selectorEntry : selector.getEntries().entrySet()) { + if (includeKeys && !Selector.isReservedLabel(selectorEntry.getKey())) { + visitor.visit(selectorEntry.getKey(), attribute); + } + if (includeValues) { + T value = + selector.isValueSet(selectorEntry.getKey()) + ? selectorEntry.getValue() + : type.cast(attribute.getDefaultValue(null)); + type.visitLabels(visitor, value, attribute); + } + } + } } /** - * Override that also visits the rule's configurable attribute keys (which are - * themselves labels). + * Returns all labels reachable via the given attribute, with duplicate instances removed. + * + *

Use this interface over {@link #visitAttribute} whenever possible, since the latter has + * efficiency problems discussed in that method's documentation. + * + * @param includeSelectKeys whether to include config_setting keys for configurable attributes */ - @Override - public void visitLabels(AcceptsLabelAttribute observer) { - super.visitLabels(observer); - for (String attrName : getAttributeNames()) { - Attribute attribute = getAttributeDefinition(attrName); - Type.Selector selector = getSelector(attrName, attribute.getType()); - if (selector != null) { - for (Label configLabel : selector.getEntries().keySet()) { - if (!Type.Selector.isReservedLabel(configLabel)) { - observer.acceptLabelAttribute(configLabel, attribute); - } + public ImmutableSet

If the attribute's value is a simple value, then this returns a singleton list of that + * value. + * + *

If the attribute's value is an expression containing one or many {@code select(...)} + * expressions, then this returns a list of all values that expression may evaluate to. This is + * dangerous because it's easy to write attributes with an exponential number of possible values: + * + * 

+   *   foo = select({a: 1, b: 2} + select({c: 3, d: 4}) + select({e: 5, f: 6})
+   *
+ * + *

Possible values: [135, 136, 145, 146, 235, 236, 245, 246] (i.e. 2^3). + * + *

This is true not just for attributes with multiple selects, but also {@link + * Attribute.ComputedDefault}s depending on such attributes. + * + *

If the attribute does not have an explicit value for this rule, and the rule provides a + * computed default, the computed default function is evaluated given the rule's other attribute + * values as inputs and the output is returned in a singleton list. + * + *

If the attribute does not have an explicit value for this rule, and the rule provides a + * computed default, and the computed default function depends on other attributes whose values + * contain {@code select(...)} expressions, then the computed default function is evaluated for + * every possible combination of input values, and the list of outputs is returned. + * + *

EFFICIENCY WARNING: Do not use this method unless you really need every single value + * the attribute might take. + * + *

More often than not, calling code doesn't really need every value, but really just wants to + * know, e.g., which labels might appear in a dependency list. For such cases, merging methods + * like {@link #getReachableLabels} work just as well without the efficiency hit. Use those + * whenever possible. */ - @Override public Iterable visitAttribute(String attributeName, Type type) { + return visitAttribute(attributeName, type, /*mayTreatMultipleAsNone=*/ false); + } + + /** + * Specialization of {@link #visitAttribute(String, Type)} for query output formatters which need + * one attribute value or none at all. Should be used with the same care as its sibling method. + * + * @param mayTreatMultipleAsNone signals if attribute-value computation may be aborted if + * more than one possible value is encountered. This parameter is respected on a best-effort + * basis - multiple values may still be returned if an unoptimized code path is visited. + */ + @SuppressWarnings("unchecked") + public Iterable visitAttribute( + String attributeName, Type type, boolean mayTreatMultipleAsNone) { + Object rawVal = rule.getAttr(attributeName, type); + // If this attribute value is configurable, visit all possible values. - Type.Selector selector = getSelector(attributeName, type); - if (selector != null) { - ImmutableList.Builder builder = ImmutableList.builder(); - for (Map.Entry entry : selector.getEntries().entrySet()) { - builder.add(entry.getValue()); - } - return builder.build(); + if (rawVal instanceof SelectorList) { + return getAllValues(((SelectorList) rawVal).getSelectors(), type, mayTreatMultipleAsNone); } + return visitRawNonConfigurableAttributeValue(rawVal, attributeName, type); + } + + private List visitRawNonConfigurableAttributeValue( + Object rawVal, String attributeName, Type type) { // If this attribute is a computed default, feed it all possible value combinations of // its declared dependencies and return all computed results. For example, if this default // uses attributes x and y, x can configurably be x1 or x2, and y can configurably be y1 // or y1, then compute default values for the (x1,y1), (x1,y2), (x2,y1), and (x2,y2) cases. - Attribute.ComputedDefault computedDefault = getComputedDefault(attributeName, type); - if (computedDefault != null) { - // This will hold every (value1, value2, ..) combination of the declared dependencies. - List> depMaps = new LinkedList<>(); - // Collect those combinations. - mapDepsForComputedDefault(computedDefault.dependencies(), depMaps, - ImmutableMap.of()); - List possibleValues = new ArrayList<>(); // Not ImmutableList.Builder: values may be null. - // For each combination, call getDefault on a specialized AttributeMap providing those values. - for (Map depMap : depMaps) { - possibleValues.add(type.cast(computedDefault.getDefault(mapBackedAttributeMap(depMap)))); - } - return possibleValues; + if (rawVal instanceof Attribute.ComputedDefault) { + return ((Attribute.ComputedDefault) rawVal).getPossibleValues(type, rule); + } + + if ("visibility".equals(attributeName) && type.equals(BuildType.NODEP_LABEL_LIST)) { + // This special case for the visibility attribute is needed because its value is replaced + // with an empty list during package loading if it is public or private in order not to visit + // the package called 'visibility'. + return ImmutableList.of(type.cast(rule.getVisibility().getDeclaredLabels())); } // For any other attribute, just return its direct value. - T value = get(attributeName, type); - return value == null ? ImmutableList.of() : ImmutableList.of(value); + T value = getFromRawAttributeValue(rawVal, attributeName, type); + return value == null ? ImmutableList.of() : ImmutableList.of(value); } /** - * Given (possibly configurable) attributes that a computed default depends on, creates an - * {attrName -> attrValue} map for every possible combination of those attribute values and - * returns a list of all the maps. This defines the complete dependency space that can affect - * the computed default's values. + * Given a list of attributes, creates an {attrName -> attrValue} map for every possible + * combination of those attributes' values and returns a list of all the maps. * - *

For example, given dependencies x and y, which might respectively have values x1, x2 and + *

For example, given attributes x and y, which respectively have possible values x1, x2 and * y1, y2, this returns: + * * 

    *   [
    *    {x: x1, y: y1},
@@ -132,87 +362,239 @@ public  Iterable visitAttribute(String attributeName, Type type) {
    *   ]
    *
* - * @param depAttributes the names of the attributes this computed default depends on - * @param mappings the list of {attrName --> attrValue} maps defining the computed default's - * dependency space. This is where this method's results are written. - * @param currentMap a (possibly non-empty) map to add {attrName --> attrValue} - * entries to. Outside callers can just pass in an empty map. + *

The work done by this method may be limited by providing a {@link ComputationLimiter} that + * throws if too much work is attempted. + */ + List> visitAttributes( + List attributes, ComputationLimiter limiter) throws TException { + List> depMaps = new LinkedList<>(); + AtomicInteger combinationsSoFar = new AtomicInteger(0); + visitAttributesInner( + attributes, + depMaps, + Maps.newHashMapWithExpectedSize(attributes.size()), + combinationsSoFar, + limiter); + return depMaps; + } + + /** + * A recursive function used in the implementation of {@link #visitAttributes}. + * + * @param attributes a list of attributes that are yet to be visited. + * @param mappings a mutable list of {attrName --> attrValue} maps collected so far. This method + * will add newly discovered maps to the list. + * @param currentMap {attrName --> attrValue} assignments accumulated so far, not including those + * in {@code attributes}. This map may be mutated and as such must be copied if we wish to + * preserve its state, such as in the base case. + * @param combinationsSoFar a counter for all previously processed combinations of possible + * values. + * @param limiter a strategy to limit the work done by invocations of this method. */ - private void mapDepsForComputedDefault(List depAttributes, - List> mappings, Map currentMap) { - // Because this method uses exponential time/space on the number of inputs, keep the - // maximum number of inputs conservatively small. - Preconditions.checkState(depAttributes.size() <= 2); - - if (depAttributes.isEmpty()) { - // Recursive base case: store whatever's already been populated in currentMap. - mappings.add(currentMap); + private void visitAttributesInner( + List attributes, + List> mappings, + Map currentMap, + AtomicInteger combinationsSoFar, + ComputationLimiter limiter) + throws TException { + if (attributes.isEmpty()) { + // Because this method uses exponential time/space on the number of inputs, we may limit + // the total number of method calls. + limiter.onComputationCount(combinationsSoFar.incrementAndGet()); + // Recursive base case: snapshot and store whatever's already been populated in currentMap. + mappings.add(new HashMap<>(currentMap)); return; } // Take the first attribute in the dependency list and iterate over all its values. For each - // value x, copy currentMap with the additional entry { firstAttrName: x }, then feed + // value x, update currentMap with the additional entry { firstAttrName: x }, then feed // this recursively into a subcall over all remaining dependencies. This recursively // continues until we run out of values. - String firstAttribute = depAttributes.get(0); - for (Object value : visitAttribute(firstAttribute, getAttributeType(firstAttribute))) { - Map newMap = new HashMap<>(); - newMap.putAll(currentMap); - newMap.put(firstAttribute, value); - mapDepsForComputedDefault(depAttributes.subList(1, depAttributes.size()), mappings, newMap); + String currentAttribute = attributes.get(0); + Iterable firstAttributePossibleValues = + visitAttribute(currentAttribute, getAttributeType(currentAttribute)); + List restOfAttrs = attributes.subList(1, attributes.size()); + for (Object value : firstAttributePossibleValues) { + // Overwrite each time. + currentMap.put(currentAttribute, value); + visitAttributesInner(restOfAttrs, mappings, currentMap, combinationsSoFar, limiter); } } /** - * A custom {@link AttributeMap} that reads attribute values from the given Map. All - * non-configurable attributes are also readable. Any attempt to read an attribute - * that's not in one of these two cases triggers an IllegalArgumentException. + * Returns an {@link AttributeMap} that delegates to {@code AggregatingAttributeMapper.this} + * except for {@link #get} calls for attributes that are configurable. In that case, the {@link + * AttributeMap} looks up an attribute's value in {@code directMap}. Any attempt to {@link #get} a + * configurable attribute that's not in {@code directMap} causes an {@link + * IllegalArgumentException} to be thrown. */ - private AttributeMap mapBackedAttributeMap(final Map directMap) { - final AggregatingAttributeMapper owner = AggregatingAttributeMapper.this; - return new AttributeMap() { + AttributeMap createMapBackedAttributeMap(Map directMap) { + AggregatingAttributeMapper owner = this; + return new DelegatingAttributeMapper(owner) { @Override + @Nullable public T get(String attributeName, Type type) { owner.checkType(attributeName, type); - if (nonconfigurableAttributes.contains(attributeName)) { + if (getNonConfigurableAttributes().contains(attributeName)) { return owner.get(attributeName, type); } - if (!directMap.containsKey(attributeName)) { - throw new IllegalArgumentException("attribute \"" + attributeName - + "\" isn't available in this computed default context"); + + Object val = directMap.get(attributeName); + if (val == null) { + checkArgument( + directMap.containsKey(attributeName), + "attribute \"%s\" isn't available in this computed default context", + attributeName); + return null; } - return type.cast(directMap.get(attributeName)); + return type.cast(val); } - @Override public String getName() { return owner.getName(); } - @Override public Label getLabel() { return owner.getLabel(); } - @Override public Iterable getAttributeNames() { - return ImmutableList.builder() - .addAll(directMap.keySet()).addAll(nonconfigurableAttributes).build(); - } - @Override - public void visitLabels(AcceptsLabelAttribute observer) { owner.visitLabels(observer); } - @Override - public String getPackageDefaultHdrsCheck() { return owner.getPackageDefaultHdrsCheck(); } - @Override - public Boolean getPackageDefaultObsolete() { return owner.getPackageDefaultObsolete(); } @Override - public Boolean getPackageDefaultTestOnly() { return owner.getPackageDefaultTestOnly(); } - @Override - public String getPackageDefaultDeprecation() { return owner.getPackageDefaultDeprecation(); } - @Override - public ImmutableList getPackageDefaultCopts() { - return owner.getPackageDefaultCopts(); + public ImmutableList getAttributeNames() { + List nonConfigurableAttributes = getNonConfigurableAttributes(); + return ImmutableList.builderWithExpectedSize( + directMap.size() + nonConfigurableAttributes.size()) + .addAll(directMap.keySet()) + .addAll(nonConfigurableAttributes) + .build(); } - @Nullable @Override - public Type getAttributeType(String attrName) { return owner.getAttributeType(attrName); } - @Nullable @Override public Attribute getAttributeDefinition(String attrName) { - return owner.getAttributeDefinition(attrName); + }; + } + + /** + * Helper class for {@link #getAllValues}. Represents a node in the logical DAG of combinations of + * {@link Selector}s' values. + */ + private static class ConfigurableAttrVisitationNode { + /** Offset into the list of selectors being combined. */ + private final int offset; + /** Key of the selector taken. */ + private final Label boundKey; + /** Accumulated value through this node. */ + private final T valueSoFar; + + private ConfigurableAttrVisitationNode(int offset, Label boundKey, T valueSoFar) { + this.offset = offset; + this.boundKey = boundKey; + this.valueSoFar = valueSoFar; + } + } + + /** + * Represents a path previously taken through a previous selector. + * + *

Used to short-circuit visitation when encountering selectors with equivalent key + * sets. See uses for details. Note that this optimization is not safe for overlapping but + * different keysets due to specialization (see {@link ConfiguredAttributeMapper}). + */ + private static class BoundKeyAndOffset { + /** Key chosen from associated select. */ + private final Label key; + /** + * Offset into the list of selectors where this key was bound. Used to determine when {@link + * #key} is safe to follow through equivalent selects. + */ + private final int offset; + + private BoundKeyAndOffset(Label key, int offset) { + this.key = key; + this.offset = offset; + } + } + + /** + * Determines all possible values a configurable attribute can take. Do not call this method + * unless really necessary and avoid all new uses. + */ + // TODO(bazel-team): minimize or eliminate uses of this interface. It necessarily grows + // exponentially with the number of selects in the attribute. Is that always necessary? + // For example, dependency resolution just needs to know every possible label an attribute + // might reference, but it doesn't need to know the exact combination of labels that make + // up a value. This may be even less important for non-label values (e.g. strings), which + // have no impact on the dependency structure. + private static ImmutableList getAllValues( + List> selectors, Type type, boolean mayTreatMultipleAsNone) { + if (selectors.isEmpty()) { + return ImmutableList.of(); + } + + if (selectors.size() == 1) { + // Optimize for common case. + return selectors.get(0).getEntries().values().stream() + .filter(Objects::nonNull) + .collect(ImmutableList.toImmutableList()); + } + + Deque> nodes = new ArrayDeque<>(); + // Track per selector key set when we started visiting a specific key. + Map, BoundKeyAndOffset> boundKeysAndOffsets = new HashMap<>(); + ImmutableList.Builder result = ImmutableList.builder(); + + // Seed visitation. + for (Map.Entry root : selectors.get(0).getEntries().entrySet()) { + nodes.push(new ConfigurableAttrVisitationNode<>(0, root.getKey(), root.getValue())); + } + + boolean foundResults = false; + while (!nodes.isEmpty()) { + ConfigurableAttrVisitationNode node = nodes.pop(); + int nextOffset = node.offset + 1; + if (nextOffset >= selectors.size()) { + // Null values arise when a None is used as the value of a Selector for a type without a + // default value. + if (node.valueSoFar != null) { + if (foundResults && mayTreatMultipleAsNone) { + // Caller wanted one value or none at all, this is the second, so bail. + return ImmutableList.of(); + } + foundResults = true; + + // TODO(gregce): visitAttribute should probably convey that an unset attribute is + // possible. Therefore we need to actually handle null values here. + result.add(node.valueSoFar); + } + continue; } - @Override public boolean isAttributeValueExplicitlySpecified(String attributeName) { - return owner.isAttributeValueExplicitlySpecified(attributeName); + + Map nextSelectorEntries = selectors.get(nextOffset).getEntries(); + BoundKeyAndOffset boundKeyAndOffset = boundKeysAndOffsets.get(nextSelectorEntries.keySet()); + if (boundKeyAndOffset != null && boundKeyAndOffset.offset < node.offset) { + // We've seen this select key set before along this path and chosen this key. + nodes.push( + new ConfigurableAttrVisitationNode<>( + nextOffset, + boundKeyAndOffset.key, + concat(type, node.valueSoFar, nextSelectorEntries.get(boundKeyAndOffset.key)))); + continue; } - }; + + Set