commit 53a0ff1641d8efaa34c995dbd54d849c4b8a50bc
author ulfjack <ulfjack@google.com> Tue May 21 07:08:40 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Tue May 21 07:09:38 2019 -0700
tree 3a7430455a204857f02da245b4e6b7869e231b8a
parent 7b3804a33cbaec4b653599680749fe3e5b51ec36

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

src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index 0ae0337..d63b8eb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.util.ThreadSafeBatchCallback;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ExecutionException;
@@ -122,7 +123,7 @@
   }
 
   @Override
-  public ResolvedTargets<Target> getTargetsInPackage(
+  public Collection<Target> getTargetsInPackage(
       String originalPattern, PackageIdentifier packageIdentifier, boolean rulesOnly)
       throws TargetParsingException, InterruptedException {
     FilteringPolicy actualPolicy = rulesOnly
@@ -138,18 +139,16 @@
     }
   }
 
-  private Map<PackageIdentifier, ResolvedTargets<Target>> bulkGetTargetsInPackage(
-          String originalPattern,
-          Iterable<PackageIdentifier> pkgIds, FilteringPolicy policy)
-          throws InterruptedException {
+  private Map<PackageIdentifier, Collection<Target>> bulkGetTargetsInPackage(
+      String originalPattern, Iterable<PackageIdentifier> pkgIds, FilteringPolicy policy)
+      throws InterruptedException {
     try {
       Map<PackageIdentifier, Package> pkgs = bulkGetPackages(pkgIds);
       if (pkgs.size() != Iterables.size(pkgIds)) {
         throw new IllegalStateException("Bulk package retrieval missing results: "
             + Sets.difference(ImmutableSet.copyOf(pkgIds), pkgs.keySet()));
       }
-      ImmutableMap.Builder<PackageIdentifier, ResolvedTargets<Target>> result =
-              ImmutableMap.builder();
+      ImmutableMap.Builder<PackageIdentifier, Collection<Target>> result = ImmutableMap.builder();
       for (PackageIdentifier pkgId : pkgIds) {
         Package pkg = pkgs.get(pkgId);
         result.put(pkgId,  TargetPatternResolverUtil.resolvePackageTargets(pkg, policy));
@@ -291,11 +290,11 @@
                 ImmutableSet<PackageIdentifier> pkgIdBatchSet = ImmutableSet.copyOf(pkgIdBatch);
                 packageSemaphore.acquireAll(pkgIdBatchSet);
                 try {
-                  Iterable<ResolvedTargets<Target>> resolvedTargets =
+                  Iterable<Collection<Target>> resolvedTargets =
                       bulkGetTargetsInPackage(originalPattern, pkgIdBatch, actualPolicy).values();
                   List<Target> filteredTargets = new ArrayList<>(calculateSize(resolvedTargets));
-                  for (ResolvedTargets<Target> targets : resolvedTargets) {
-                    filteredTargets.addAll(targets.getTargets());
+                  for (Collection<Target> targets : resolvedTargets) {
+                    filteredTargets.addAll(targets);
                   }
                   // TODO(bazel-core): Invoking the callback while holding onto the package
                   // semaphore can lead to deadlocks. Also, if the semaphore has a small count,
@@ -312,10 +311,10 @@
     return Futures.whenAllSucceed(futures).call(() -> null, directExecutor());
   }
 
-  private static <T> int calculateSize(Iterable<ResolvedTargets<T>> resolvedTargets) {
+  private static <T> int calculateSize(Iterable<Collection<T>> resolvedTargets) {
     int size = 0;
-    for (ResolvedTargets<T> targets : resolvedTargets) {
-      size += targets.getTargets().size();
+    for (Collection<T> targets : resolvedTargets) {
+      size += targets.size();
     }
     return size;
   }