Re: Query is over 2x slower with jit=on - Mailing list pgsql-hackers
From | Amit Khandekar |
---|---|
Subject | Re: Query is over 2x slower with jit=on |
Date | |
Msg-id | CAJ3gD9eLrz51RK_gTkod+71iDcjpB_N8eC6vU2AW-VicsAERpQ@mail.gmail.com Whole thread Raw |
In response to | Re: Query is over 2x slower with jit=on (Amit Khandekar <amitdkhan.pg@gmail.com>) |
Responses |
Re: Query is over 2x slower with jit=on
|
List | pgsql-hackers |
On 14 September 2018 at 16:48, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > On 11 September 2018 at 14:50, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >> On 10 September 2018 at 21:39, Andres Freund <andres@anarazel.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
Attachment
pgsql-hackers by date: