Batch all DependencyResolver#getTarget calls. This leads to some duplicate iteration, but that should be cheap, while requesting packages sequentially can hurt...
PiperOrigin-RevId: 208126130
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
index fc587e0..384ce09 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
@@ -19,6 +19,9 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.BuildType.SelectorList;
import com.google.devtools.build.lib.syntax.Type;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
import javax.annotation.Nullable;
/**
@@ -162,16 +165,15 @@
}
@Override
- public void visitLabels(final AcceptsLabelAttribute observer) throws InterruptedException {
- Type.LabelVisitor<Attribute> visitor = new Type.LabelVisitor<Attribute>() {
- @Override
- public void visit(@Nullable Label label, Attribute attribute) throws InterruptedException {
- if (label != null) {
- Label absoluteLabel = ruleLabel.resolveRepositoryRelative(label);
- observer.acceptLabelAttribute(absoluteLabel, attribute);
- }
- }
- };
+ public Collection<DepEdge> visitLabels() throws InterruptedException {
+ List<DepEdge> edges = new ArrayList<>();
+ Type.LabelVisitor<Attribute> visitor =
+ (label, attribute) -> {
+ if (label != null) {
+ Label absoluteLabel = ruleLabel.resolveRepositoryRelative(label);
+ edges.add(AttributeMap.DepEdge.create(absoluteLabel, attribute));
+ }
+ };
for (Attribute attribute : ruleClass.getAttributes()) {
Type<?> type = attribute.getType();
// TODO(bazel-team): clean up the typing / visitation interface so we don't have to
@@ -181,6 +183,7 @@
visitLabels(attribute, visitor);
}
}
+ return edges;
}
/** Visits all labels reachable from the given attribute. */
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
index e932e60..75d3e0e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.syntax.Type.LabelVisitor;
import com.google.devtools.build.lib.syntax.Type.ListType;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.LinkedList;
@@ -521,8 +522,8 @@
}
@Override
- public void visitLabels(AcceptsLabelAttribute observer) throws InterruptedException {
- owner.visitLabels(observer);
+ public Collection<DepEdge> visitLabels() throws InterruptedException {
+ return owner.visitLabels();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java
index b8a28ce..faf3d9e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java
@@ -13,10 +13,12 @@
// limitations under the License.
package com.google.devtools.build.lib.packages;
+import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.Type;
+import java.util.Collection;
import javax.annotation.Nullable;
/**
@@ -104,24 +106,28 @@
/** Returns the {@link Location} at which the attribute was defined. */
Location getAttributeLocation(String attrName);
- /** An interface which accepts {@link Attribute}s, used by {@link #visitLabels}. */
- interface AcceptsLabelAttribute {
- /**
- * Accept a (Label, Attribute) pair describing a dependency edge.
- *
- * @param label the target node of the (Rule, Label) edge. The source node should already be
- * known.
- * @param attribute the attribute.
- */
- void acceptLabelAttribute(Label label, Attribute attribute) throws InterruptedException;
- }
+ /**
+ * Returns a {@link Collection} with a {@link DepEdge} for every attribute that contains labels in
+ * its value (either by *being* a label or being a collection that includes labels).
+ */
+ Collection<DepEdge> visitLabels() throws InterruptedException;
/**
- * For all attributes that contain labels in their values (either by *being* a label or being a
- * collection that includes labels), visits every label and notifies the specified observer at
- * each visit.
+ * {@code (Label, Attribute)} pair describing a dependency edge.
+ *
+ * <p>The {@link Label} is the target node of the {@code (Rule, Label)} edge. The source node
+ * should already be known. The {@link Attribute} is the attribute giving the edge.
*/
- void visitLabels(AcceptsLabelAttribute observer) throws InterruptedException;
+ @AutoValue
+ abstract class DepEdge {
+ public abstract Label getLabel();
+
+ public abstract Attribute getAttribute();
+
+ static DepEdge create(Label label, Attribute attribute) {
+ return new AutoValue_AttributeMap_DepEdge(label, attribute);
+ }
+ }
// TODO(bazel-team): These methods are here to support computed defaults that inherit
// package-level default values. Instead, we should auto-inherit and remove the computed
diff --git a/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java
index 28f29bc..c08f3d9 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/DelegatingAttributeMapper.java
@@ -18,6 +18,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.Type;
+import java.util.Collection;
import javax.annotation.Nullable;
/**
@@ -79,8 +80,8 @@
}
@Override
- public void visitLabels(AcceptsLabelAttribute observer) throws InterruptedException {
- delegate.visitLabels(observer);
+ public Collection<DepEdge> visitLabels() throws InterruptedException {
+ return delegate.visitLabels();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index daefc112..ad823a2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -32,7 +32,6 @@
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.AttributeMap.AcceptsLabelAttribute;
import com.google.devtools.build.lib.packages.License.DistributionType;
import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
@@ -1413,12 +1412,10 @@
// All labels mentioned in a rule that refer to an unknown target in the
// current package are assumed to be InputFiles, so let's create them:
for (final Rule rule : sortedRules) {
- AggregatingAttributeMapper.of(rule).visitLabels(new AcceptsLabelAttribute() {
- @Override
- public void acceptLabelAttribute(Label label, Attribute attribute) {
- createInputFileMaybe(label, rule.getAttributeLocation(attribute.getName()));
- }
- });
+ for (AttributeMap.DepEdge depEdge : AggregatingAttributeMapper.of(rule).visitLabels()) {
+ createInputFileMaybe(
+ depEdge.getLabel(), rule.getAttributeLocation(depEdge.getAttribute().getName()));
+ }
}
}
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 37c8767..94fbaaf 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
@@ -406,12 +406,11 @@
/** Returns a new List instance containing all direct dependencies (all types). */
public Collection<Label> getLabels() throws InterruptedException {
final List<Label> labels = Lists.newArrayList();
- AggregatingAttributeMapper.of(this).visitLabels(new AttributeMap.AcceptsLabelAttribute() {
- @Override
- public void acceptLabelAttribute(Label label, Attribute attribute) {
- labels.add(label);
- }
- });
+ AggregatingAttributeMapper.of(this)
+ .visitLabels()
+ .stream()
+ .map(AttributeMap.DepEdge::getLabel)
+ .forEach(labels::add);
return labels;
}
@@ -444,14 +443,11 @@
// TODO(bazel-team): move this to AttributeMap, too. Just like visitLabels, which labels should
// be visited may depend on the calling context. We shouldn't implicitly decide this for
// the caller.
- AggregatingAttributeMapper.of(this).visitLabels(new AttributeMap.AcceptsLabelAttribute() {
- @Override
- public void acceptLabelAttribute(Label label, Attribute attribute) {
- if (predicate.apply(Rule.this, attribute)) {
- transitions.put(attribute, label);
- }
- }
- });
+ AggregatingAttributeMapper.of(this)
+ .visitLabels()
+ .stream()
+ .filter(depEdge -> predicate.apply(Rule.this, depEdge.getAttribute()))
+ .forEach(depEdge -> transitions.put(depEdge.getAttribute(), depEdge.getLabel()));
return transitions;
}
@@ -706,16 +702,10 @@
// the introduction of this code is #2210848 (NullPointerException in
// Package.checkForConflicts() ).
void checkForNullLabels() throws InterruptedException {
- AggregatingAttributeMapper.of(this).visitLabels(
- new AttributeMap.AcceptsLabelAttribute() {
- @Override
- public void acceptLabelAttribute(Label labelToCheck, Attribute attribute) {
- checkForNullLabel(labelToCheck, attribute);
- }
- });
- for (OutputFile outputFile : getOutputFiles()) {
- checkForNullLabel(outputFile.getLabel(), "output file");
- }
+ AggregatingAttributeMapper.of(this)
+ .visitLabels()
+ .forEach(depEdge -> checkForNullLabel(depEdge.getLabel(), depEdge.getAttribute()));
+ getOutputFiles().forEach(outputFile -> checkForNullLabel(outputFile.getLabel(), "output file"));
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
index b1dd3da..b68f907 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
@@ -275,9 +275,13 @@
return true;
}
- ExplicitEdgeVisitor visitor = new ExplicitEdgeVisitor(rule, label);
- AggregatingAttributeMapper.of(rule).visitLabels(visitor);
- return visitor.isExplicit();
+ for (AttributeMap.DepEdge depEdge : AggregatingAttributeMapper.of(rule).visitLabels()) {
+ if (rule.isAttributeValueExplicitlySpecified(depEdge.getAttribute())
+ && label.equals(depEdge.getLabel())) {
+ return true;
+ }
+ }
+ return false;
}
/**
@@ -306,30 +310,6 @@
};
}
- private static class ExplicitEdgeVisitor implements AttributeMap.AcceptsLabelAttribute {
- private final Label expectedLabel;
- private final Rule rule;
- private boolean isExplicit = false;
-
- public ExplicitEdgeVisitor(Rule rule, Label expected) {
- this.rule = rule;
- this.expectedLabel = expected;
- }
-
- @Override
- public void acceptLabelAttribute(Label label, Attribute attr) {
- if (isExplicit || !rule.isAttributeValueExplicitlySpecified(attr)) {
- // Nothing to do here.
- } else if (expectedLabel.equals(label)) {
- isExplicit = true;
- }
- }
-
- public boolean isExplicit() {
- return isExplicit;
- }
- }
-
/**
* Return {@link Location} for {@link Target} target, if it should not be null.
*/
