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 once1. 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
Attachments:
fix-always-inline-warnings.patchapplication/octet-stream; name=fix-always-inline-warnings.patchDownload
From 117dc0e5a72db2bb292819955804ed1e48b723e7 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Thu, 4 Jan 2018 22:16:28 +1300
Subject: [PATCH] Fix warnings about pg_attribute_always_inline.
MSVC warned about the use of both __forceinline and inline. We can't just
remove inline or GCC 7 will complain, so move it into the macro. GCC 2.95
warned that it didn't understand always_inline, so choose GCC 4.x as a pretty
old cut-off point that is known to understand it.
Thomas Munro, per buildfarm and Tom Lane.
Discussion: https://postgr.es/m/32278.1514863068@sss.pgh.pa.us
---
src/backend/executor/nodeHashjoin.c | 3 +--
src/include/c.h | 4 ++--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 8f2b634b124..03d78042fa0 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -161,8 +161,7 @@ static void ExecParallelHashJoinPartitionOuter(HashJoinState *node);
* the other one is "outer".
* ----------------------------------------------------------------
*/
-pg_attribute_always_inline
-static inline TupleTableSlot *
+static pg_attribute_always_inline TupleTableSlot *
ExecHashJoinImpl(PlanState *pstate, bool parallel)
{
HashJoinState *node = castNode(HashJoinState, pstate);
diff --git a/src/include/c.h b/src/include/c.h
index 34a7fa67b45..01f943a1392 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -147,8 +147,8 @@
#endif
/* GCC, Sunpro and XLC support always_inline via __attribute__ */
-#if defined(__GNUC__)
-#define pg_attribute_always_inline __attribute__((always_inline))
+#if (defined(__GNUC__) && __GNUC__ > 3) || defined(__SUNPRO_C) || defined(__IBMC__)
+#define pg_attribute_always_inline __attribute__((always_inline)) inline
/* msvc via a special keyword */
#elif defined(_MSC_VER)
#define pg_attribute_always_inline __forceinline
--
2.15.0
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 once1. 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