Re: [HACKERS] WIP: Faster Expression Processing v4

On 03/14/2017 08:53 AM, Andres Freund wrote:
> Besides that, this version has:
> - pgindented most of the affected pieces (i.e. all significant new code
>   has been reindent, not all touched one)

I think you'll need to add all the inner structs ExprEvalStep 
typedefs.list to indent them right.

> My current plan is to not do very much on this tomorrow, do another full
> pass on Wednesday, and push it, unless there's protest till then.

I looked at patch 0004. Some comments:

* EEO_DISPATCH_DIRECT(op) takes 'op' as an argument, but it really 
assumes that 'op' has already been set to point to the jump target. I 
find that a bit weird. I guess the idea is that you always pass the 
Program Counter variable as 'op' argument. For consistency, would be 
good if EEO_SWITCH() also took just 'op' as the argument, rather than 
op->opcode. But I think it would be more clear if they should both just 
assumed that there's a variable called 'op' that points to the current 
instruction.

* All the callers of EEO_DISPATCH_DIRECT(op) set 'op' just prior to 
calling EEO_DISPATCH_DIRECT(op). How about having a macro EEO_JUMP(<step 
number>), to encapsulate setting 'op' and jumping to it in one operation?

* ExecEvalStepOp() seems relatively expensive, with the linear scan over 
all the opcodes, if called on an ExprState that already has 
EEO_FLAG_JUMP_THREADED set. All the callers use it to check if the 
opcode is a particular one, so you could check if the opcode matches 
that particular one, instead of scanning the dispatch table to find what 
it is.

* But is ExecEvalStepOp() ever actually get called on an expression with 
EEO_FLAG_JUMP_THREADED already set? It's only used in 
ExecInstantiateInterpretedExpr(), and it's called only once on each 
ExprState. How about just adding an Assert that EEO_FLAG_JUMP_THREADED 
is not yet set? Or drop the EEO_FLAG_JUMP_THREADED flag altogether, and 
assert that evalfunc != ExecInterpExpr.

* How tight are we on space in the ExprEvalStep union? Currently, the 
jump-threading preparation replaces the opcodes with the goto labels, 
but it would be really nice to keep the original opcodes, for debugging 
purposes if nothing else.

* execInterpExpr.c is quite a mouthful. How about exprInterpreter.c?


Attached is a patch, on top of your 
0004-Faster-expression-evaluation-and-targetlist-projecti.patch, with 
some minor tweaks and comments here and there (search for HEIKKI). It's 
mostly the same stuff I listed above, implemented in a quick & dirty 
way. I hope it's helpful.

- Heikki


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers