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

On 14 September 2018 at 16:48, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> On 11 September 2018 at 14:50, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>> On 10 September 2018 at 21:39, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>> /*
>>>> + * Add up the workers' JIT instrumentation from dynamic shared memory.
>>>> + */
>>>> +static void
>>>> +ExecParallelRetrieveJitInstrumentation(PlanState *planstate,
>>>> + SharedJitInstrumentation *shared_jit)
>>>> +{
>>>> + int n;
>>>> + JitContext *jit = planstate->state->es_jit;
>>>> +
>>>> + /* If the leader hasn't yet created a jit context, allocate one now. */
>>>> + if (!jit)
>>>> + {
>>>> + planstate->state->es_jit = jit =
>>>> + jit_create_context(planstate->state->es_jit_flags);
>>>> + }
>>>
>>> Wouldn't it be better to move the jit instrumentation to outside of the
>>> context, to avoid having to do this? Or just cope with not having
>>> instrumentation for the leader in this case? We'd kinda need to deal
>>> with failure to create one anyway?
>>
>> Yeah, I think taking out the instrumentation out of the context looks
>> better. Will work on that.
>
> Not yet done this. Will do now.

Attached v3 patch that does the above change.

The jit instrumentation counters are used by the provider code
directly. So I think even if we keep the instrumentation outside of
the context, we should let the resource manager handle deallocation of
the instrumentation, similar to how it deallocates the whole jit
context. So we should allocate the instrumentation in
TopMemoryContext. Otherwise, if the instrumentation is allocated in
per-query context, and if the provider functions accesses it after the
Portal resource is cleaned up then the instrumentation would have been
already de-allocated.

So for this, I thought we better have two separate contexts: base
context (i.e. the usual JitContext) and provider-specific context.
JitContext has new field provider_context pointing to LLVMJitContext.
And LLVMJitContext has a 'base' field pointing to the base context
(i.e. JitContext)

So in ExecParallelRetrieveJitInstrumentation(), where the jit context
is created if it's NULL, I now create just the base context. Later,
on-demand the JitContext->provider_context will be allocated in
llvm_compile().

This way, we can release both the base context and provider context
during ResourceOwnerRelease(), and at the same time continue to be
able to access the jit instrumentation from the provider.

-----------

BTW There is this code in llvm_compile_expr() on HEAD that does not
handle the !parent case correctly :

/* get or create JIT context */
if (parent && parent->state->es_jit)
{
context = (LLVMJitContext *) parent->state->es_jit;
}
else
{
context = llvm_create_context(parent->state->es_jit_flags);

if (parent)
{
parent->state->es_jit = &context->base;
}
}

Here, llvm_create_context() will crash if parent is NULL. Is it that
parent can never be NULL ? I think so, because in jit_compile_expr()
we skip everything if there is no plan state. So probably there should
be an Assert(parent != NULL) rather than handling parent check. Or
else, we need to decide what JIT flags to supply to
llvm_create_context() if we let the provider create the context
without a plan state. For now, in the changes that did to the above
snippet, I have kept a TODO.

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company