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
