> All that code is hotspot stuff, and turning it into a pile of nested
> procedures doesn't seem like it improves either performance or
> readability.
Your concern regarding performance is understandable. But I should note
that any standard compiler supports inlining these days (probably
this statement is true for at least a decade now). Here is assembly code
of patched version of heap_open produced by GCC 4.8.4 with -O2 flag:
(lldb) disassemble
postgres`heap_open:
0x497db0 <+0>: pushq %rbx
0x497db1 <+1>: callq 0x497af0 ; relation_open(...)
0x497db6 <+6>: movq %rax, %rbx
-> 0x497db9 <+9>: movq 0x30(%rax), %rax
0x497dbd <+13>: movzbl 0x6f(%rax), %eax
0x497dc1 <+17>: cmpb $0x69, %al ; 'i', RELKIND_INDEX
0x497dc3 <+19>: je 0x497dce
0x497dc5 <+21>: cmpb $0x63, %al ; 'c', COMPOSITE_TYPE
0x497dc7 <+23>: je 0x497dd7
0x497dc9 <+25>: movq %rbx, %rax
0x497dcc <+28>: popq %rbx
0x497dcd <+29>: retq
As you see heap_open_check_relation procedure was successfully inlined.
Just to be sure that less smart compilers will more likely apply this
optimization I updated patch with 'inline' hints (see attachment).
And even if compiler decide not to apply inlining in this case there is
much more to consider than presence or absence of one 'call' assembly
instruction. For instance compiler may believe that on this concrete
architecture it will be more beneficial to make code shorter so it
would fit to CPU cache better.
Anyway I don't believe that imaginary benchmarks are worth trusting. I
personally don't have much faith in non-imaginary benchmarks either but
it's a different story.
In the same time I'm deeply convinced that this patch will make code
more readable at least because it makes code much shorter:
src/backend/access/heap/heapam.c | 109 +++---
1 file changed, 39 insertions(+), 70 deletions(-)
Thus we can see more code on the screen. Besides since there is no code
duplication there is less change that somebody someday will change say
heap_openrv without updating heap_openrv_extended accordingly.
--
Best regards,
Aleksander Alekseev
http://eax.me/
