Fix inadvertent performance regression introduced by the recent rewrite of 'blaze query'.

The "streaming" callbacks used by some query functions, e.g. 'deps', make calls to QueryEnvironment#buildTransitiveClosure.

For a cold blaze server, these calls do package loading via LabelVisitor (which calls into Skyframe via a top-level #evaluate call). So we'd prefer a single massive call which can make full use of blaze's loading-phase parallelism via Skyframe over a bunch of sequential small calls.

For a hot blaze server, there are two problems:
(1) LabelVisitor's meager up-to-date check isn't useful (as in we cannot reuse old visitations) when we do a whole bunch of small visitations instead of one massive one.
(2) The actual work of the LabelVisitor (building up a portion of a temporary graph) isn't being effectively parallelized when we do it sequentially in small chunks.

This issue is yet another subtle reason why the old BlazeQueryEnvironment#eval made sense (and why it was unfortunately not compatible with the streaming query evaluation model from the beginning).

--
PiperOrigin-RevId: 150081619
MOS_MIGRATED_REVID=150081619

1 files changed, 18 insertions, 2 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/AbstractQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/engine/AbstractQueryEnvironment.java
index 62fd91b56f..6bc7217b16 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/engine/AbstractQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/engine/AbstractQueryEnvironment.java
@@ -121,8 +121,24 @@ public abstract class AbstractQueryEnvironment<T> implements QueryEnvironment<T>
 
   @Override
   public QueryTaskFuture<Void> eval(
-      QueryExpression expr, VariableContext<T> context, Callback<T> callback) {
-    return expr.eval(this, context, callback);
+      QueryExpression expr, VariableContext<T> context, final Callback<T> callback) {
+    // Not all QueryEnvironment implementations embrace the async+streaming evaluation framework. In
+    // particular, the streaming callbacks employed by functions like 'deps' use
+    // QueryEnvironment#buildTransitiveClosure. So if the implementation of that method does some
+    // heavyweight blocking work, then it's best to do this blocking work in a single batch.
+    // Importantly, the callback we pass in needs to maintain order.
+    final QueryUtil.AggregateAllCallback<T> aggregateAllCallback =
+        QueryUtil.newOrderedAggregateAllOutputFormatterCallback();
+    QueryTaskFuture<Void> evalAllFuture = expr.eval(this, context, aggregateAllCallback);
+    return whenSucceedsCall(
+        evalAllFuture,
+        new QueryTaskCallable<Void>() {
+          @Override
+          public Void call() throws QueryException, InterruptedException {
+            callback.process(aggregateAllCallback.getResult());
+            return null;
+          }
+        });
   }
 
   @Override