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
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 62fd91b..6bc7217 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 @@
@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
