Re: Unimpressed with pg_attribute_always_inline

On 2018-01-08 19:12:08 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Unless this pg_attribute_always_inline gets a lot more widely
> > proliferated I don't see a need to change anything. Debuggability isn't
> > meaningfully impacted by seing more runtime attributed to
> > ExecHashJoin/ExecParallelHashJoin rather than ExecHashJoinImpl.
> 
> When I complained that always_inline inhibits debuggability, I did NOT
> mean what shows up in perf reports.  I'm talking about whether you can
> break at, or single-step through, a function reliably and whether gdb
> knows where all the variables are.  In my experience, inlining hurts
> both of those things, which is why I'm saying that forcing inlining
> even in non-optimized builds is a bad idea.

Yea, I got that, I just don't think it's a strong argument for the cases
here.


> If we needed this thing to cause inlining even in optimized builds,
> there might be a case for it; but that is not what I'm reading in
> the gcc manual.

That's what it's doing here however:

(gdb) disassemble ExecHashJoin
Dump of assembler code for function ExecHashJoin:
   0x00000000000012f0 <+0>:     xor    %esi,%esi
   0x00000000000012f2 <+2>:     jmpq   0x530 <ExecHashJoinImpl>
End of assembler dump.
(gdb) quit

$ git diff
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -161,7 +161,7 @@ static void ExecParallelHashJoinPartitionOuter(HashJoinState *node);
  *                       the other one is "outer".
  * ----------------------------------------------------------------
  */
-pg_attribute_always_inline
+//pg_attribute_always_inline
 static inline TupleTableSlot *
 ExecHashJoinImpl(PlanState *pstate, bool parallel)
 {


reverting that hunk:

make nodeHashjoin.o
gcc-7 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O3 -ggdb -g3
-Wmissing-declarations-Wmissing-prototypes -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare
-Wno-missing-field-initializers-Wempty-body -Wno-clobbered -march=native -mtune=native -Wno-implicit-fallthrough
-I../../../src/include-I/home/andres/src/postgresql-master/src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o
nodeHashjoin.o/home/andres/src/postgresql-master/src/backend/executor/nodeHashjoin.c -MMD -MP -MF
.deps/nodeHashjoin.Po

$ gdb nodeHashjoin.o
(gdb) disassemble ExecHashJoin
Dump of assembler code for function ExecHashJoin:
   0x0000000000000530 <+0>:     push   %r15
   0x0000000000000532 <+2>:     mov    %rdi,%r15
   0x0000000000000535 <+5>:     push   %r14
...[whole function]

If this were just to get it to force inlining in assertion builds, I'd
certainly not agree that it makes sense. But here it's really about
forcing the compilers hand in the optimized case.

Greetings,

Andres Freund