Thread: Unimpressed with pg_attribute_always_inline
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
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
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
Attachment
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
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
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
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
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
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
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
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
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
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
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
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