Query: Avoid copying the package targets
Change TargetPatternResolver.getTargetsInPackage and the
TargetPatternPreloader to return Collections, rather than
ResolvedTargets instances.
No implementation currently sets the error bit, and no caller requires
it, and the set of filtered targets is always empty.
Make sure that getTargetsMatchingPatternImpl uses a Set to avoid
quadratic runtime. While this may cancel out the improvements from this
change, I have some follow-up changes which use the result and don't
require a Set implementation. I missed this regression because I was
testing it with the additional changes, not independently.
PiperOrigin-RevId: 249243165
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
index 933c061..2f6c4dc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
@@ -14,16 +14,16 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Preconditions;
-import com.google.common.base.Predicates;
-import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.ResolvedTargets;
+import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.skyframe.WalkableGraph;
+import java.util.Collection;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
@@ -33,24 +33,13 @@
* list of target patterns (eg, //foo:all -//bar/... //foo:test).
*/
class TargetPatternsResultBuilder {
- private final ResolvedTargets.Builder<Label> resolvedLabelsBuilder = ResolvedTargets.builder();
+ private final Set<Label> resolvedLabelsBuilder = CompactHashSet.create();
private Map<PackageIdentifier, Package> packages;
- private boolean hasError;
-
- /** Sets that there was an error, during evaluation. */
- public void setError() {
- hasError = true;
- }
/** Returns final set of targets and sets error flag if required. */
- public ResolvedTargets<Target> build(WalkableGraph walkableGraph) throws InterruptedException {
+ public Collection<Target> build(WalkableGraph walkableGraph) throws InterruptedException {
precomputePackages(walkableGraph);
- ResolvedTargets.Builder<Target> resolvedTargetsBuilder =
- transformLabelsIntoTargets(resolvedLabelsBuilder.build());
- if (hasError) {
- resolvedTargetsBuilder.setError();
- }
- return resolvedTargetsBuilder.build();
+ return transformLabelsIntoTargets(resolvedLabelsBuilder);
}
/**
@@ -58,18 +47,14 @@
* method is using information about packages, so {@link #precomputePackages} has to be called
* before this method.
*/
- private ResolvedTargets.Builder<Target> transformLabelsIntoTargets(
- ResolvedTargets<Label> resolvedLabels) {
+ private Collection<Target> transformLabelsIntoTargets(Set<Label> resolvedLabels) {
// precomputePackages has to be called before this method.
- ResolvedTargets.Builder<Target> resolvedTargetsBuilder = ResolvedTargets.builder();
+ Set<Target> targets = CompactHashSet.create();
Preconditions.checkNotNull(packages);
- for (Label label : resolvedLabels.getTargets()) {
- resolvedTargetsBuilder.add(getExistingTarget(label));
+ for (Label label : resolvedLabels) {
+ targets.add(getExistingTarget(label));
}
- for (Label label : resolvedLabels.getFilteredTargets()) {
- resolvedTargetsBuilder.remove(getExistingTarget(label));
- }
- return resolvedTargetsBuilder;
+ return targets;
}
private void precomputePackages(WalkableGraph walkableGraph) throws InterruptedException {
@@ -94,7 +79,7 @@
private Set<PackageIdentifier> getPackagesIdentifiers() {
Set<PackageIdentifier> packagesIdentifiers = new HashSet<>();
- for (Label label : getLabels()) {
+ for (Label label : resolvedLabelsBuilder) {
packagesIdentifiers.add(label.getPackageIdentifier());
}
return packagesIdentifiers;
@@ -107,19 +92,9 @@
.getPackage();
}
- /** Adds the result from expansion of positive target pattern (eg, "//foo:all"). */
- void addLabelsOfNegativePattern(ResolvedTargets<Label> labels) {
- resolvedLabelsBuilder.filter(Predicates.not(Predicates.in(labels.getTargets())));
- }
-
/** Adds the result from expansion of negative target pattern (eg, "-//foo:all"). */
void addLabelsOfPositivePattern(ResolvedTargets<Label> labels) {
- resolvedLabelsBuilder.merge(labels);
- }
-
- /** Returns target labels from all individual results. */
- private Iterable<Label> getLabels() {
- ResolvedTargets<Label> resolvedLabels = resolvedLabelsBuilder.build();
- return Iterables.concat(resolvedLabels.getTargets(), resolvedLabels.getFilteredTargets());
+ Preconditions.checkArgument(labels.getFilteredTargets().isEmpty());
+ resolvedLabelsBuilder.addAll(labels.getTargets());
}
}
