From a6ee96f95a9608046b773b2ed8493bdcb4282463 Mon Sep 17 00:00:00 2001 From: Nathan Harmata Date: Tue, 14 Mar 2017 17:22:12 +0000 Subject: 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 --- .../lib/query2/engine/AbstractQueryEnvironment.java | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) (limited to 'src') 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 implements QueryEnvironment @Override public QueryTaskFuture eval( - QueryExpression expr, VariableContext context, Callback callback) { - return expr.eval(this, context, callback); + QueryExpression expr, VariableContext context, final Callback 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 aggregateAllCallback = + QueryUtil.newOrderedAggregateAllOutputFormatterCallback(); + QueryTaskFuture evalAllFuture = expr.eval(this, context, aggregateAllCallback); + return whenSucceedsCall( + evalAllFuture, + new QueryTaskCallable() { + @Override + public Void call() throws QueryException, InterruptedException { + callback.process(aggregateAllCallback.getResult()); + return null; + } + }); } @Override -- cgit v1.2.3