Parallelize Package retrieval during Sky-query operation. To maintain type-safety, we now must pass in the exception type of the callback.

--
MOS_MIGRATED_REVID=113313312

8 files changed, 97 insertions, 38 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
index 5617c942e9..8b49a2ab0a 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
@@ -143,9 +143,9 @@ public abstract class TargetPattern implements Serializable {
    * Evaluates the current target pattern and returns the result.
    */
   public <T, E extends Exception> void eval(
-      TargetPatternResolver<T> resolver, BatchCallback<T, E> callback)
+      TargetPatternResolver<T> resolver, BatchCallback<T, E> callback, Class<E> exceptionClass)
       throws TargetParsingException, E, InterruptedException {
-    eval(resolver, ImmutableSet.<PathFragment>of(), callback);
+    eval(resolver, ImmutableSet.<PathFragment>of(), callback, exceptionClass);
   }
 
   /**
@@ -158,7 +158,7 @@ public abstract class TargetPattern implements Serializable {
   public abstract <T, E extends Exception> void eval(
       TargetPatternResolver<T> resolver,
       ImmutableSet<PathFragment> excludedSubdirectories,
-      BatchCallback<T, E> callback)
+      BatchCallback<T, E> callback, Class<E> exceptionClass)
       throws TargetParsingException, E, InterruptedException;
 
   /**
@@ -210,7 +210,7 @@ public abstract class TargetPattern implements Serializable {
     public <T, E extends Exception> void eval(
         TargetPatternResolver<T> resolver,
         ImmutableSet<PathFragment> excludedSubdirectories,
-        BatchCallback<T, E> callback)
+        BatchCallback<T, E> callback, Class<E> exceptionClass)
         throws TargetParsingException, E, InterruptedException {
       Preconditions.checkArgument(excludedSubdirectories.isEmpty(),
           "Target pattern \"%s\" of type %s cannot be evaluated with excluded subdirectories: %s.",
@@ -264,7 +264,7 @@ public abstract class TargetPattern implements Serializable {
     public <T, E extends Exception> void eval(
         TargetPatternResolver<T> resolver,
         ImmutableSet<PathFragment> excludedSubdirectories,
-        BatchCallback<T, E> callback)
+        BatchCallback<T, E> callback, Class<E> exceptionClass)
         throws TargetParsingException, E, InterruptedException {
       Preconditions.checkArgument(excludedSubdirectories.isEmpty(),
           "Target pattern \"%s\" of type %s cannot be evaluated with excluded subdirectories: %s.",
@@ -353,7 +353,7 @@ public abstract class TargetPattern implements Serializable {
     public <T, E extends Exception> void eval(
         TargetPatternResolver<T> resolver,
         ImmutableSet<PathFragment> excludedSubdirectories,
-        BatchCallback<T, E> callback)
+        BatchCallback<T, E> callback, Class<E> exceptionClass)
         throws TargetParsingException, E, InterruptedException {
       Preconditions.checkArgument(excludedSubdirectories.isEmpty(),
           "Target pattern \"%s\" of type %s cannot be evaluated with excluded subdirectories: %s.",
@@ -463,7 +463,7 @@ public abstract class TargetPattern implements Serializable {
     public <T, E extends Exception> void eval(
         TargetPatternResolver<T> resolver,
         ImmutableSet<PathFragment> excludedSubdirectories,
-        BatchCallback<T, E> callback)
+        BatchCallback<T, E> callback, Class<E> exceptionClass)
         throws TargetParsingException, E, InterruptedException {
       resolver.findTargetsBeneathDirectory(
           directory.getRepository(),
@@ -471,7 +471,8 @@ public abstract class TargetPattern implements Serializable {
           directory.getPackageFragment().getPathString(),
           rulesOnly,
           excludedSubdirectories,
-          callback);
+          callback,
+          exceptionClass);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
index 05c914ae49..8dc0f0a76a 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
@@ -79,6 +79,7 @@ public interface TargetPatternResolver<T> {
    * @param excludedSubdirectories a set of transitive subdirectories beneath {@code directory}
    *    to ignore
    * @param callback the callback to receive the result, possibly in multiple batches.
+   * @param exceptionClass The class type of the parameterized exception.
    * @throws TargetParsingException under implementation-specific failure conditions
    */
   <E extends Exception> void findTargetsBeneathDirectory(
@@ -87,7 +88,7 @@ public interface TargetPatternResolver<T> {
       String directory,
       boolean rulesOnly,
       ImmutableSet<PathFragment> excludedSubdirectories,
-      BatchCallback<T, E> callback)
+      BatchCallback<T, E> callback, Class<E> exceptionClass)
       throws TargetParsingException, E, InterruptedException;
 
   /**
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 363f5d565d..f5ffe544d9 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
@@ -25,6 +25,7 @@ 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.ThreadFactoryBuilder;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -83,6 +84,8 @@ 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.logging.Logger;
 
 import javax.annotation.Nullable;
@@ -418,13 +421,17 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
                     .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());
+                provider, eventHandler, targetPatternKey.getPolicy(), threadPool);
         TargetPattern parsedPattern = targetPatternKey.getParsedPattern();
         FilteringBatchingUniquifyingCallback wrapper =
             new FilteringBatchingUniquifyingCallback(callback);
-        parsedPattern.eval(resolver, wrapper);
+        parsedPattern.eval(resolver, wrapper, QueryException.class);
         wrapper.processLastPending();
       } catch (TargetParsingException e) {
         reportBuildFileError(owner, e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index f5c3ada49b..60221ad319 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.cmdline.TargetPattern;
 import com.google.devtools.build.lib.cmdline.TargetPattern.Type;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
@@ -55,6 +56,7 @@ import java.util.Set;
  * SkyQueryEnvironment} to look up the packages and targets matching the universe that's been
  * preloaded in {@code graph}.
  */
+@ThreadSafe
 public final class GraphBackedRecursivePackageProvider implements RecursivePackageProvider {
 
   private final WalkableGraph graph;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index f98a01d45e..781c0015df 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -82,8 +82,8 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
       TargetPattern parsedPattern = patternKey.getParsedPattern();
       DepsOfPatternPreparer preparer = new DepsOfPatternPreparer(env, pkgPath.get());
       ImmutableSet<PathFragment> excludedSubdirectories = patternKey.getExcludedSubdirectories();
-      parsedPattern.<Void, RuntimeException>eval(
-          preparer, excludedSubdirectories, NullCallback.<Void>instance());
+      parsedPattern.eval(
+          preparer, excludedSubdirectories, NullCallback.<Void>instance(), RuntimeException.class);
     } catch (TargetParsingException e) {
       throw new PrepareDepsOfPatternFunctionException(e);
     } catch (MissingDepException e) {
@@ -216,7 +216,7 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
         String directory,
         boolean rulesOnly,
         ImmutableSet<PathFragment> excludedSubdirectories,
-        BatchCallback<Void, E> callback)
+        BatchCallback<Void, E> callback, Class<E> exceptionClass)
         throws TargetParsingException, E, InterruptedException {
       FilteringPolicy policy =
           rulesOnly ? FilteringPolicies.RULES_ONLY : FilteringPolicies.NO_FILTER;
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 b24ee7af58..65a737ec7f 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
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.skyframe;
 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.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -25,6 +26,8 @@ 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;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -41,27 +44,34 @@ 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.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * A {@link TargetPatternResolver} backed by a {@link RecursivePackageProvider}.
  */
+@ThreadCompatible
 public class RecursivePackageProviderBackedTargetPatternResolver
     implements TargetPatternResolver<Target> {
 
   // TODO(janakr): Move this to a more generic place and unify with SkyQueryEnvironment's value?
-  private static final int MAX_PACKAGES_BULK_GET = 10000;
+  private static final int MAX_PACKAGES_BULK_GET = 1000;
 
   private final RecursivePackageProvider recursivePackageProvider;
   private final EventHandler eventHandler;
   private final FilteringPolicy policy;
+  private final ExecutorService executor;
 
   public RecursivePackageProviderBackedTargetPatternResolver(
       RecursivePackageProvider recursivePackageProvider,
       EventHandler eventHandler,
-      FilteringPolicy policy) {
+      FilteringPolicy policy,
+      ExecutorService executor) {
     this.recursivePackageProvider = recursivePackageProvider;
     this.eventHandler = eventHandler;
     this.policy = policy;
+    this.executor = executor;
   }
 
   @Override
@@ -164,13 +174,13 @@ public class RecursivePackageProviderBackedTargetPatternResolver
   @Override
   public <E extends Exception> void findTargetsBeneathDirectory(
       final RepositoryName repository,
-      String originalPattern,
+      final String originalPattern,
       String directory,
       boolean rulesOnly,
       ImmutableSet<PathFragment> excludedSubdirectories,
-      BatchCallback<Target, E> callback)
+      final BatchCallback<Target, E> callback, Class<E> exceptionClass)
       throws TargetParsingException, E, InterruptedException {
-    FilteringPolicy actualPolicy = rulesOnly
+    final FilteringPolicy actualPolicy = rulesOnly
         ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy)
         : policy;
     PathFragment pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
@@ -185,29 +195,64 @@ public class RecursivePackageProviderBackedTargetPatternResolver
                 return PackageIdentifier.create(repository, path);
               }
             });
-    boolean foundTarget = false;
+    final AtomicBoolean foundTarget = new AtomicBoolean(false);
+    final AtomicReference<InterruptedException> interrupt = new AtomicReference<>();
+    final AtomicReference<TargetParsingException> parsingException = new AtomicReference<>();
+    final AtomicReference<Exception> genericException = new AtomicReference<>();
+
+    final Object callbackLock = new Object();
+
     // For very large sets of packages, we may not want to process all of them at once, so we split
     // into batches.
-    for (Iterable<PackageIdentifier> pkgIdBatch :
-        Iterables.partition(pkgIds, MAX_PACKAGES_BULK_GET)) {
-
-      Iterable<ResolvedTargets<Target>> resolvedTargets =
-          bulkGetTargetsInPackage(originalPattern, pkgIdBatch, NO_FILTER).values();
-      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 = true;
-          if (actualPolicy.shouldRetain(target, false)) {
-            filteredTargets.add(target);
+    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<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);
+            }
           }
-        }
+        });
       }
-      callback.process(filteredTargets);
+    } finally {
+      ExecutorUtil.interruptibleShutdown(executor);
     }
 
-    if (!foundTarget) {
+    Throwables.propagateIfInstanceOf(interrupt.get(), InterruptedException.class);
+    Throwables.propagateIfInstanceOf(parsingException.get(), TargetParsingException.class);
+    Throwables.propagateIfPossible(genericException.get(), exceptionClass);
+    if (!foundTarget.get()) {
       throw new TargetParsingException("no targets found beneath '" + pathFragment + "'");
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
index 58c32ceeac..32f64e82ca 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
@@ -54,7 +55,7 @@ public class TargetPatternFunction implements SkyFunction {
           new EnvironmentBackedRecursivePackageProvider(env);
       RecursivePackageProviderBackedTargetPatternResolver resolver =
           new RecursivePackageProviderBackedTargetPatternResolver(provider, env.getListener(),
-              patternKey.getPolicy());
+              patternKey.getPolicy(), MoreExecutors.newDirectExecutorService());
       TargetPattern parsedPattern = patternKey.getParsedPattern();
       ImmutableSet<PathFragment> excludedSubdirectories = patternKey.getExcludedSubdirectories();
       final Set<Target> results = CompactHashSet.create();
@@ -65,7 +66,7 @@ public class TargetPatternFunction implements SkyFunction {
               Iterables.addAll(results, partialResult);
             }
           };
-      parsedPattern.eval(resolver, excludedSubdirectories, callback);
+      parsedPattern.eval(resolver, excludedSubdirectories, callback, RuntimeException.class);
       resolvedTargets = ResolvedTargets.<Target>builder().addAll(results).build();
     } catch (TargetParsingException e) {
       throw new TargetPatternFunctionException(e);
diff --git a/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java b/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
index 75c347f8fe..fd43a07555 100644
--- a/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.EventHandler;
 
 import java.util.Collection;
@@ -24,6 +25,7 @@ import javax.annotation.Nullable;
  * Read-only graph that exposes the dependents, dependencies (reverse dependents), and value and
  * exception (if any) of a given node.
  */
+@ThreadSafe
 public interface WalkableGraph {
 
   /**