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
faster-eval-comments.patch
Description: application/download
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
