Re: Query is over 2x slower with jit=on

On 2018-09-26 23:45:35 +0530, Amit Khandekar wrote:
> On Tue, 25 Sep 2018 at 14:17, Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2018-09-18 10:03:02 +0530, Amit Khandekar wrote:
> > > Attached v3 patch that does the above change.
> >
> > Attached is a revised version of that patch.  I've changed quite a few
> > things:
> > - I've reverted the split of "base" and "provider specific" contexts - I
> >   don't think it really buys us anything here.
> 
> The idea was to have a single estate field that accumulates all the
> JIT counters of leader as well as workers. I see that you want to
> delay the merging of workers and backend counters until end of query
> execution. More points on this in the bottom section.

No, I never want to merge things into the leader's stats.

> > - I've reverted the context creation changes - instead of creating a
> >   context in the leader just to store instrumentation in the worker,
> >   there's now a new EState->es_jit_combined_instr.
> 
> The context created in the leader was a light context, involving only
> the resource owner stuff, and not the provider initialization.

I know, but it added a fair bit of infrastructure and complications just
for that - and I see very very little reason that that'd ever be a
necessary separation.

> >
> > - That also means worker instrumentation doesn't get folded into the
> >   leader's instrumentation.
> 
> You mean the worker instrumentation doesn't get folded into the leader
> until the query execution end, right ? In the committed code, I see
> that now we merge the leader instrumentation into the combined worker
> instrumentation in standard_ExecutorEnd().

No, I mean it *never* gets folded into the leader's
instrumentation. There's a *separate* instrumentation field, where they
do get combined. But that still allows code to print out the leader's
stats alone.


> > This seems good for the future and for
> >   extensions - it's not actually "linear" time that's spent doing
> >   JIT in workers (& leader), as all of that work happens in
> >   parallel. Being able to disentangle that seems important.
> 
> Ok. Your point is: we should have the backend and workers info stored
> in two separate fields, and combine them only when we need it; so that
> we will be in a position to show combined workers-only info separately
> in the future. From the code, it looks like the es_jit_combined_instr
> stores combined workers info not just from a single Gather node, but
> all the Gather nodes in the plan. If we want to have separate workers
> info, I am not sure if it makes sense in combining workers from two
> separate Gather nodes; because these two sets of workers are
> unrelated, aren't they ?

Well, we now have all the individual stats around in an unmodified
manner. We could print both the aggregate and individualized stats.

Greetings,

Andres Freund