Horton

Thread

Commits

  1. Improve implementation of pg_attribute_always_inline.

  2. Add pg_attribute_always_inline.

  1. Unimpressed with pg_attribute_always_inline

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-02T03:17:48Z

    I find myself entirely unimpressed with the results of commit dbb3d6f01.

In the first place, even among the compilers that claim to understand
that directive at all, there is a noticeable tendency to issue warnings.
I find the following in recent buildfarm "make" logs:

 baiji     | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
 bowerbird | src/backend/executor/nodeHashjoin.c(165): warning C4141: 'inline' : used more than once [c:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]
 currawong | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
 gaur      | nodeHashjoin.c:167: warning: `always_inline' attribute directive ignored
 mastodon  | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
 thrips    | src/backend/executor/nodeHashjoin.c(165): warning C4141: 'inline' : used more than once [C:\buildfarm\buildenv\HEAD\pgsql.build\postgres.vcxproj]
 woodlouse | src/backend/executor/nodeHashjoin.c(165): warning C4141: 'inline' : used more than once [C:\buildfarm\buildenv\HEAD\pgsql.build\postgres.vcxproj]

In the second place, what I read in gcc's manual about the meaning of
the always_inline directive is

`always_inline'
     Generally, functions are not inlined unless optimization is
     specified.  For functions declared inline, this attribute inlines
     the function even if no optimization level was specified.

I entirely reject the notion that we should be worried about optimizing
performance in -O0 builds.  In fact, if someone is building with -O0,
it's likely the case that they are hoping for exact correspondence
of source lines to object code, and thus forcing inline is defeating
their purpose.  I've certainly found plenty of times that inlining
makes it harder to follow things in a debugger.

Therefore, I think that pg_attribute_always_inline is not merely
useless but actively bad, and should be removed.

			regards, tom lane

  2. Re: Unimpressed with pg_attribute_always_inline

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-01-02T03:58:51Z

    On Tue, Jan 2, 2018 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>  baiji     | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
>  bowerbird | src/backend/executor/nodeHashjoin.c(165): warning C4141: 'inline' : used more than once [c:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]
>  currawong | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
>  gaur      | nodeHashjoin.c:167: warning: `always_inline' attribute directive ignored
>  mastodon  | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
>  thrips    | src/backend/executor/nodeHashjoin.c(165): warning C4141: 'inline' : used more than once [C:\buildfarm\buildenv\HEAD\pgsql.build\postgres.vcxproj]
>  woodlouse | src/backend/executor/nodeHashjoin.c(165): warning C4141: 'inline' : used more than once [C:\buildfarm\buildenv\HEAD\pgsql.build\postgres.vcxproj]

So that's two compilers:

1. MSVC doesn't like you to say both "__forceinline" and "inline".

2.  GCC 2.95.3 doesn't understand always_inline.  From a quick look at
archived manuals, it seems that that attribute arrived in 3.1.

It may be that "inline" can be removed (that seems to work OK for me
on clang, but I didn't check GCC).  Not sure off-hand how best to
tackle the ancient GCC problem; maybe a configure test or maybe a GCC
version test.  I will look into those problems.

> In the second place, what I read in gcc's manual about the meaning of
> the always_inline directive is
>
> `always_inline'
>      Generally, functions are not inlined unless optimization is
>      specified.  For functions declared inline, this attribute inlines
>      the function even if no optimization level was specified.
>
> I entirely reject the notion that we should be worried about optimizing
> performance in -O0 builds.  In fact, if someone is building with -O0,
> it's likely the case that they are hoping for exact correspondence
> of source lines to object code, and thus forcing inline is defeating
> their purpose.  I've certainly found plenty of times that inlining
> makes it harder to follow things in a debugger.
>
> Therefore, I think that pg_attribute_always_inline is not merely
> useless but actively bad, and should be removed.

My intention was to make sure it really did get inlined at higher
optimisation levels even though the compiler wouldn't otherwise choose
to do that in a couple of special cases, not to force inlining even at
-O0.  Not sure how to achieve the first of those things without the
second.  I wonder if there is a better way.

-- 
Thomas Munro
http://www.enterprisedb.com

  3. Re: Unimpressed with pg_attribute_always_inline

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-01-04T11:11:19Z

    On Tue, Jan 2, 2018 at 4:58 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Tue, Jan 2, 2018 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>  gaur      | nodeHashjoin.c:167: warning: `always_inline' attribute directive ignored
>>  mastodon  | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
>
> 1. MSVC doesn't like you to say both "__forceinline" and "inline".
>
> 2.  GCC 2.95.3 doesn't understand always_inline.  From a quick look at
> archived manuals, it seems that that attribute arrived in 3.1.

Here is one way to fix those warnings.  Thoughts?

>> Therefore, I think that pg_attribute_always_inline is not merely
>> useless but actively bad, and should be removed.

How about a macro PG_NOINLINE, which, if defined, inhibits this?  Or I
could give up this strategy and maintain two separate very similar
functions, ExecHashJoin and ExecParallelHashJoin.

-- 
Thomas Munro
http://www.enterprisedb.com

  4. Re: Unimpressed with pg_attribute_always_inline

    Andres Freund <andres@anarazel.de> — 2018-01-08T23:36:04Z

    On 2018-01-05 00:11:19 +1300, Thomas Munro wrote:
> On Tue, Jan 2, 2018 at 4:58 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
> > On Tue, Jan 2, 2018 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>  gaur      | nodeHashjoin.c:167: warning: `always_inline' attribute directive ignored
> >>  mastodon  | .\src\backend\executor\nodeHashjoin.c(165): warning C4141: 'inline' : used more than once
> >
> > 1. MSVC doesn't like you to say both "__forceinline" and "inline".
> >
> > 2.  GCC 2.95.3 doesn't understand always_inline.  From a quick look at
> > archived manuals, it seems that that attribute arrived in 3.1.
> 
> Here is one way to fix those warnings.  Thoughts?

That looks good to me.


> >> Therefore, I think that pg_attribute_always_inline is not merely
> >> useless but actively bad, and should be removed.
> 
> How about a macro PG_NOINLINE, which, if defined, inhibits this?  Or I
> could give up this strategy and maintain two separate very similar
> functions, ExecHashJoin and ExecParallelHashJoin.

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. If this
were used on functions that are called from a lot of places that
argument would hold some weight, but for now I'm really not seing it.

- Andres

  5. Re: Unimpressed with pg_attribute_always_inline

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-09T00:12:08Z

    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.

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.

			regards, tom lane

  6. Re: Unimpressed with pg_attribute_always_inline

    Andres Freund <andres@anarazel.de> — 2018-01-09T00:19:35Z

    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

  7. Re: Unimpressed with pg_attribute_always_inline

    Peter Geoghegan <pg@bowt.ie> — 2018-01-09T00:20:26Z

    On Mon, Jan 8, 2018 at 4:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

Isn't that an argument against inlining in general, rather than
forcing inlining in particular?

-- 
Peter Geoghegan

  8. Re: Unimpressed with pg_attribute_always_inline

    Andres Freund <andres@anarazel.de> — 2018-01-09T00:22:57Z

    On 2018-01-08 16:20:26 -0800, Peter Geoghegan wrote:
> On Mon, Jan 8, 2018 at 4:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 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.
> 
> Isn't that an argument against inlining in general, rather than
> forcing inlining in particular?

No. Normal 'inline' annotation doesn't do anything on -O0 / debug
builds. But always_inline does, even though the goal of the usage is
just to override the compiler's inlining heuristics.

Greetings,

Andres Freund

  9. Re: Unimpressed with pg_attribute_always_inline

    Peter Geoghegan <pg@bowt.ie> — 2018-01-09T00:39:50Z

    On Mon, Jan 8, 2018 at 4:22 PM, Andres Freund <andres@anarazel.de> wrote:
>> Isn't that an argument against inlining in general, rather than
>> forcing inlining in particular?
>
> No. Normal 'inline' annotation doesn't do anything on -O0 / debug
> builds. But always_inline does, even though the goal of the usage is
> just to override the compiler's inlining heuristics.

Sorry, I found the way this was discussed confusing.

Anyway, ISTM that it should be possible to make
pg_attribute_always_inline have no effect in typical debug builds.
Wouldn't that make everyone happy?

-- 
Peter Geoghegan

  10. Re: Unimpressed with pg_attribute_always_inline

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-09T00:56:47Z

    Peter Geoghegan <pg@bowt.ie> writes:
> Anyway, ISTM that it should be possible to make
> pg_attribute_always_inline have no effect in typical debug builds.
> Wouldn't that make everyone happy?

That would improve matters, but do we have access to the -O switch
level as an #if condition?  The use-case I'm generally worried about
involves recompiling individual files at -O0, with something like
	make PROFILE=-O0 nodeHashjoin.o
so trying to, say, get configure to adjust the macro isn't really going
to help.

The other half of the problem is the warnings, but I think Thomas'
patch would alleviate that.

			regards, tom lane

  11. Re: Unimpressed with pg_attribute_always_inline

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-09T01:09:27Z

    On 1/8/18 19:56, Tom Lane wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
>> Anyway, ISTM that it should be possible to make
>> pg_attribute_always_inline have no effect in typical debug builds.
>> Wouldn't that make everyone happy?
> 
> That would improve matters, but do we have access to the -O switch
> level as an #if condition?

See __OPTIMIZE__ and __NO_INLINE__ here:
https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

However, at <https://gcc.gnu.org/onlinedocs/gcc/Inline.html> it says,
"GCC does not inline any functions when not optimizing unless you
specify the ‘always_inline’ attribute for the function".  So,
apparently, if the goal is to turn off inlining when not optimizing,
then we should just use the normal inline attribute.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: Unimpressed with pg_attribute_always_inline

    Andres Freund <andres@anarazel.de> — 2018-01-09T01:13:17Z

    Hi,

On 2018-01-08 20:09:27 -0500, Peter Eisentraut wrote:
> On 1/8/18 19:56, Tom Lane wrote:
> > Peter Geoghegan <pg@bowt.ie> writes:
> >> Anyway, ISTM that it should be possible to make
> >> pg_attribute_always_inline have no effect in typical debug builds.
> >> Wouldn't that make everyone happy?
> > 
> > That would improve matters, but do we have access to the -O switch
> > level as an #if condition?
> 
> See __OPTIMIZE__ and __NO_INLINE__ here:
> https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

Yea, __OPTIMIZE__ might work.

> However, at <https://gcc.gnu.org/onlinedocs/gcc/Inline.html> it says,
> "GCC does not inline any functions when not optimizing unless you
> specify the ‘always_inline’ attribute for the function".  So,
> apparently, if the goal is to turn off inlining when not optimizing,
> then we should just use the normal inline attribute.

See http://archives.postgresql.org/message-id/20180109001935.s42ovj3uwmwygqzu%40alap3.anarazel.de

Greetings,

Andres Freund

  13. Re: Unimpressed with pg_attribute_always_inline

    Peter Geoghegan <pg@bowt.ie> — 2018-01-09T01:13:33Z

    On Mon, Jan 8, 2018 at 5:09 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> However, at <https://gcc.gnu.org/onlinedocs/gcc/Inline.html> it says,
> "GCC does not inline any functions when not optimizing unless you
> specify the ‘always_inline’ attribute for the function".  So,
> apparently, if the goal is to turn off inlining when not optimizing,
> then we should just use the normal inline attribute.

The compiler isn't obligated to inline anything with the normal inline
attribute. The whole point of always_inline is that the programmer may
know better than the compiler about inlining in some specific cases,
and may therefore want to make inlining absolutely mandatory. IIUC,
that's almost what we want, except that it also inlines with -O0,
which we do not want.

Have I missed the point here?

-- 
Peter Geoghegan

  14. Re: Unimpressed with pg_attribute_always_inline

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-09T01:20:09Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Jan 8, 2018 at 5:09 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> However, at <https://gcc.gnu.org/onlinedocs/gcc/Inline.html> it says,
>> "GCC does not inline any functions when not optimizing unless you
>> specify the ‘always_inline’ attribute for the function".  So,
>> apparently, if the goal is to turn off inlining when not optimizing,
>> then we should just use the normal inline attribute.

> The compiler isn't obligated to inline anything with the normal inline
> attribute. The whole point of always_inline is that the programmer may
> know better than the compiler about inlining in some specific cases,
> and may therefore want to make inlining absolutely mandatory. IIUC,
> that's almost what we want, except that it also inlines with -O0,
> which we do not want.

> Have I missed the point here?

No, you have it right.  I checked locally and confirmed Andres' assertion
that by default, gcc (my version anyway) is not persuaded to inline
ExecHashJoinImpl simply by "inline", but "always_inline" persuades it.
Maybe at some level higher than -O2, or with some other weird flag,
it would do what we want; but we probably don't want to mess with global
compiler flags for this.

			regards, tom lane

  15. Re: Unimpressed with pg_attribute_always_inline

    Andres Freund <andres@anarazel.de> — 2018-01-09T01:30:10Z

    Hi,

On 2018-01-08 20:20:09 -0500, Tom Lane wrote:
> No, you have it right.  I checked locally and confirmed Andres' assertion
> that by default, gcc (my version anyway) is not persuaded to inline
> ExecHashJoinImpl simply by "inline", but "always_inline" persuades it.
> Maybe at some level higher than -O2, or with some other weird flag,
> it would do what we want; but we probably don't want to mess with global
> compiler flags for this.

There's -finline-limit=n, but as you say, I don't think we want to mess
with this.

Greetings,

Andres Freund