From: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: WIP: Faster Expression Processing v4 |
Date: | 2017-03-14 14:58:54 |
Message-ID: | 69b7bb0e-8dab-3d39-1a89-3c6ab6409f64@iki.fi |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
faster-eval-comments.patch | application/x-download | 23.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Steele | 2017-03-14 15:01:45 | Re: PATCH: Configurable file mode mask |
Previous Message | Ashutosh Sharma | 2017-03-14 14:51:07 | Re: PATCH: pageinspect / add page_checksum and bt_page_items(bytea) |