Обсуждение: Don't codegen deform code for virtual tuples in expr eval for scan fetch

Hi,

Thanks for working on this!

On 2019-09-17 23:54:51 -0700, Soumyadeep Chakraborty wrote:
> This is to address a TODO I found in the JIT expression evaluation
> code (opcode =
> EEOP_INNER_FETCHSOME/EEOP_OUTER_FETCHSOME/EEOP_SCAN_FETCHSOME):
> 
> * TODO: skip nvalid check if slot is fixed and known to
> * be a virtual slot.

I now think this isn't actually the right approach. Instead of doing
this optimization just for JIT compilation, we should do it while
generating the ExprState itself. That way we don't just accelerate JITed
programs, but also normal interpreted execution.

IOW, wherever ExecComputeSlotInfo() is called, we should only actually
push the expression step, when ExecComputeSlotInfo does not determine
that a) the slot is virtual, b) and fixed (i.e. guaranteed to always be
the same type of slot).

Does that make sense?

Greetings,

Andres Freund

Hi,

On 2019-09-20 22:19:46 -0700, Soumyadeep Chakraborty wrote:
> In my previous patch 0001, the resulting opblock consisted of a single
> br instruction to it's successor opblock. Such a block represents
> unnecessary overhead. Even though such a block would be optimized
> away, what if optimization is not performed (perhaps due to
> jit_optimize_above_cost)? Perhaps we could be more aggressive. We
> could maybe remove the opblock altogether. However, such a solution
> is not without complexity.

I'm doubtful this is worth the complexity - and not that we already have
plenty other places with zero length blocks.

WRT jit_optimize_above_cost not triggering, I think we need to replace
the "basic, unoptimized" codegen path with one that does cheap
optimizations only. The reason we don't do the full optimizations all
the time is that they're expensive, but there's enough optimizations
that are pretty cheap.  At some point we'll probably need our own
optimization pipeline, but I don't want to maintain that right now
(i.e. if some other people want to help with this aspect, cool)...

See also: https://www.postgresql.org/message-id/20190904152438.pv4vdk3ctuvvnxh3%40alap3.anarazel.de

Greetings,

Andres Freund

Hi,

On 2019-09-25 22:11:51 -0700, Soumyadeep Chakraborty wrote:
> Thank you very much for reviewing my patch!
> 
> On Wed, Sep 25, 2019 at 1:02 PM Andres Freund <andres@anarazel.de> wrote:
> > IOW, wherever ExecComputeSlotInfo() is called, we should only actually
> > push the expression step, when ExecComputeSlotInfo does not determine
> > that a) the slot is virtual, b) and fixed (i.e. guaranteed to always be
> > the same type of slot).
> >
> > Does that make sense?
> 
> That is a great suggestion and I totally agree. I have attached a patch
> that achieves this.

I think as done, this has the slight disadvantage of removing the
fast-path for small interpreted expressions, because that explicitly
matches for seeing the EEOP_*_FETCHSOME ops. Look at execExprInterp.c,
around:
    /*
     * Select fast-path evalfuncs for very simple expressions.  "Starting up"
     * the full interpreter is a measurable overhead for these, and these
     * patterns occur often enough to be worth optimizing.
     */
    if (state->steps_len == 3)
    {

So I think we'd have to add a separate fastpath for virtual slots for
that.

What do you think about the attached?

Greetings,

Andres Freund

Hi,

On 2019-09-27 23:01:05 -0700, Soumyadeep Chakraborty wrote:
> I completely agree, that was an important consideration.
> 
> I had some purely cosmetic suggestions:
> 1. Rename ExecComputeSlotInfo to eliminate the need for the asserts.

How does renaming it do so? I feel like the asserts are a good idea
independent of anything else?


> 2. Extract return value to a bool variable for slightly better
> readability.

To me that seems clearly worse. The variable doesn't add anything, but
needing to track more state.


> 3. Taking the opportunity to use TTS_IS_VIRTUAL.

Good point.

- Andres

Hi,

On 2019-09-30 09:14:45 -0700, Soumyadeep Chakraborty wrote:
> I don't feel very strongly about the changes I proposed.
> 
> > > I completely agree, that was an important consideration.
> > >
> > > I had some purely cosmetic suggestions:
> > > 1. Rename ExecComputeSlotInfo to eliminate the need for the asserts.
> >
> > How does renaming it do so? I feel like the asserts are a good idea
> > independent of anything else?
> 
> I felt that encoding the restriction that the function is meant to be called
> only in the context of fetch operations in the function name itself
> ensured that we don't call it from a non-fetch operation - something the
> asserts within ExecComputeSlotInfo() are guarding against.
> 
> >
> > > 2. Extract return value to a bool variable for slightly better
> > > readability.
> >
> > To me that seems clearly worse. The variable doesn't add anything, but
> > needing to track more state.>
> 
> Agreed.

I pushed this to master. Thanks for your contribution!

Regards,

Andres