Create one TargetPatternResolver per SkyQueryEnvironment initialization

Instead of creating one GraphBackedRecursivePackageProvider, thread
pool, and RecursivePackageProviderBackedTargetPatternResolver per call
to getTargetsMatchingPattern, create them once during field
initialization and in the init method.

Previously, Recursive[..]TargetPatternResolver expected to be given
ownership of the executor, so it could shut it down. With this change,
the resolver now employs a different technique for blocking on the
completion of its asynchronous calls. The change also fixes an issue
where the use of the resolver along with the
EnvironmentBackedRecursivePackageProvider worked only because the
provided executor was a direct executor.

--
MOS_MIGRATED_REVID=125499330

2 files changed, 77 insertions, 56 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index bc6dc9c9af..525bcb9e1e 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -27,6 +27,8 @@ import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -92,7 +94,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
-import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Logger;
@@ -112,7 +113,6 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
 
   protected WalkableGraph graph;
 
-  private ImmutableList<TargetPatternKey> universeTargetPatternKeys;
   private Supplier<ImmutableSet<PathFragment>> blacklistPatternsSupplier;
 
   private final BlazeTargetAccessor accessor = new BlazeTargetAccessor(this);
@@ -132,6 +132,12 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
       return target.getLabel();
     }
   };
+  private final ListeningExecutorService threadPool =
+      MoreExecutors.listeningDecorator(
+          Executors.newFixedThreadPool(
+              Runtime.getRuntime().availableProcessors(),
+              new ThreadFactoryBuilder().setNameFormat("GetPackages-%d").build()));
+  private RecursivePackageProviderBackedTargetPatternResolver resolver;
 
   private static class BlacklistSupplier implements Supplier<ImmutableSet<PathFragment>> {
     private final WalkableGraph graph;
@@ -179,9 +185,17 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
     blacklistPatternsSupplier = Suppliers.memoize(new BlacklistSupplier(graph));
 
     SkyKey universeKey = graphFactory.getUniverseKey(universeScope, parserPrefix);
-    universeTargetPatternKeys =
+    ImmutableList<TargetPatternKey> universeTargetPatternKeys =
         PrepareDepsOfPatternsFunction.getTargetPatternKeys(
             PrepareDepsOfPatternsFunction.getSkyKeys(universeKey, eventHandler));
+    GraphBackedRecursivePackageProvider graphBackedRecursivePackageProvider =
+        new GraphBackedRecursivePackageProvider(graph, universeTargetPatternKeys, pkgPath);
+    resolver =
+        new RecursivePackageProviderBackedTargetPatternResolver(
+            graphBackedRecursivePackageProvider,
+            eventHandler,
+            TargetPatternEvaluator.DEFAULT_FILTERING_POLICY,
+            threadPool);
 
     // The prepareAndGet call above evaluates a single PrepareDepsOfPatterns SkyKey.
     // We expect to see either a single successfully evaluated value or a cycle in the result.
@@ -487,14 +501,6 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
               TargetPatternValue.key(
                       pattern, TargetPatternEvaluator.DEFAULT_FILTERING_POLICY, parserPrefix)
                   .argument());
-      GraphBackedRecursivePackageProvider provider = new GraphBackedRecursivePackageProvider(
-          graph, universeTargetPatternKeys, pkgPath);
-      ExecutorService threadPool = Executors.newFixedThreadPool(
-          Runtime.getRuntime().availableProcessors(),
-          new ThreadFactoryBuilder().setNameFormat("GetPackages-%d").build());
-      RecursivePackageProviderBackedTargetPatternResolver resolver =
-          new RecursivePackageProviderBackedTargetPatternResolver(
-              provider, eventHandler, targetPatternKey.getPolicy(), threadPool);
       TargetPattern parsedPattern = targetPatternKey.getParsedPattern();
       ImmutableSet<PathFragment> subdirectoriesToExclude =
           targetPatternKey.getAllSubdirectoriesToExclude(blacklistPatternsSupplier);
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 46bd0c2693..642d8eae36 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
@@ -17,17 +17,20 @@ import static com.google.devtools.build.lib.pkgcache.FilteringPolicies.NO_FILTER
 
 import com.google.common.base.Function;
 import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.cmdline.TargetPatternResolver;
-import com.google.devtools.build.lib.concurrent.ExecutorUtil;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
@@ -45,7 +48,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -62,13 +65,13 @@ public class RecursivePackageProviderBackedTargetPatternResolver
   private final RecursivePackageProvider recursivePackageProvider;
   private final EventHandler eventHandler;
   private final FilteringPolicy policy;
-  private final ExecutorService executor;
+  private final ListeningExecutorService executor;
 
   public RecursivePackageProviderBackedTargetPatternResolver(
       RecursivePackageProvider recursivePackageProvider,
       EventHandler eventHandler,
       FilteringPolicy policy,
-      ExecutorService executor) {
+      ListeningExecutorService executor) {
     this.recursivePackageProvider = recursivePackageProvider;
     this.eventHandler = eventHandler;
     this.policy = policy;
@@ -210,49 +213,61 @@ public class RecursivePackageProviderBackedTargetPatternResolver
 
     // For very large sets of packages, we may not want to process all of them at once, so we split
     // into batches.
-    try {
-      for (final Iterable<PackageIdentifier> pkgIdBatch :
-          Iterables.partition(pkgIds, MAX_PACKAGES_BULK_GET)) {
-        executor.execute(new Runnable() {
-          @Override
-          public void run() {
-            Iterable<ResolvedTargets<Target>> resolvedTargets = null;
-            try {
-              resolvedTargets =
-                  bulkGetTargetsInPackage(originalPattern, pkgIdBatch, NO_FILTER).values();
-            } catch (InterruptedException e) {
-              interrupt.compareAndSet(null, e);
-              return;
-            } catch (TargetParsingException e) {
-              parsingException.compareAndSet(null, e);
-            }
+    List<List<PackageIdentifier>> partitions =
+        ImmutableList.copyOf(Iterables.partition(pkgIds, MAX_PACKAGES_BULK_GET));
+    ArrayList<ListenableFuture<?>> futures = new ArrayList<>(partitions.size());
+    for (final Iterable<PackageIdentifier> pkgIdBatch : partitions) {
+      futures.add(
+          executor.submit(
+              new Runnable() {
+                @Override
+                public void run() {
+                  Iterable<ResolvedTargets<Target>> resolvedTargets;
+                  try {
+                    resolvedTargets =
+                        bulkGetTargetsInPackage(originalPattern, pkgIdBatch, NO_FILTER).values();
+                  } catch (InterruptedException e) {
+                    interrupt.compareAndSet(null, e);
+                    return;
+                  } catch (TargetParsingException e) {
+                    parsingException.compareAndSet(null, e);
+                    return;
+                  } catch (RuntimeException e) {
+                    // In particular, we're interested in remembering any thrown
+                    // MissingDepExceptions.
+                    genericException.compareAndSet(null, e);
+                    return;
+                  }
 
-            List<Target> filteredTargets = new ArrayList<>(calculateSize(resolvedTargets));
-            for (ResolvedTargets<Target> targets : resolvedTargets) {
-              for (Target target : targets.getTargets()) {
-                // Perform the no-targets-found check before applying the filtering policy so we
-                // only return the error if the input directory's subtree really contains no
-                // targets.
-                foundTarget.set(true);
-                if (actualPolicy.shouldRetain(target, false)) {
-                  filteredTargets.add(target);
+                  List<Target> filteredTargets = new ArrayList<>(calculateSize(resolvedTargets));
+                  for (ResolvedTargets<Target> targets : resolvedTargets) {
+                    for (Target target : targets.getTargets()) {
+                      // Perform the no-targets-found check before applying the filtering policy
+                      // so we only return the error if the input directory's subtree really
+                      // contains no targets.
+                      foundTarget.set(true);
+                      if (actualPolicy.shouldRetain(target, false)) {
+                        filteredTargets.add(target);
+                      }
+                    }
+                  }
+                  try {
+                    synchronized (callbackLock) {
+                      callback.process(filteredTargets);
+                    }
+                  } catch (InterruptedException e) {
+                    interrupt.compareAndSet(null, e);
+                  } catch (Exception e) {
+                    genericException.compareAndSet(e, null);
+                  }
                 }
-              }
-            }
-            try {
-              synchronized (callbackLock) {
-                callback.process(filteredTargets);
-              }
-            } catch (InterruptedException e) {
-              interrupt.compareAndSet(null, e);
-            } catch (Exception e) {
-              genericException.compareAndSet(e, null);
-            }
-          }
-        });
-      }
-    } finally {
-      ExecutorUtil.interruptibleShutdown(executor);
+              }));
+    }
+
+    try {
+      Futures.allAsList(futures).get();
+    } catch (ExecutionException e) {
+      throw new IllegalStateException(e);
     }
 
     Throwables.propagateIfInstanceOf(interrupt.get(), InterruptedException.class);