Обсуждение: What to do with inline warnings?

What do we do with warnings generated by -Winline?

I see this now:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels
-fno-strict-aliasing-fwrapv -g -Werror -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2   -c -o 
 
tuplesort.o tuplesort.c -MMD -MP -MF .deps/tuplesort.Po
cc1: warnings being treated as errors
tuplesort.c: In function ‘comparetup_index_btree’:
tuplesort.c:2474: error: inlining failed in call to ‘myFunctionCall2’: --param large-stack-frame-growth limit reached
tuplesort.c:2525: error: called from here
tuplesort.c:2474: error: inlining failed in call to ‘myFunctionCall2’: --param large-stack-frame-growth limit reached
tuplesort.c:2525: error: called from here

and this:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels
-fno-strict-aliasing-fwrapv -g -Werror -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2   -c -o 
 
tqual.o tqual.c -MMD -MP -MF .deps/tqual.Po
cc1: warnings being treated as errors
tqual.c: In function ‘HeapTupleSatisfiesVacuum’:
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1057: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1061: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1073: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1077: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1092: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1099: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1146: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1164: error: called from here
tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
tqual.c:1171: error: called from here

Peter Eisentraut <peter_e@gmx.net> writes:
> What do we do with warnings generated by -Winline?

I believe we just put that in to see how many places inlining was being
done or not.  If you want to compile with -Werror you'd better take it out.

> tqual.c: In function ‘HeapTupleSatisfiesVacuum’:
> tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
> tqual.c:1057: error: called from here
> tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
> tqual.c:1061: error: called from here

Hmm, it's a bit disturbing that the compiler is taking it upon itself to
decide that these calls are "unlikely".
        regards, tom lane

On Wed, May 14, 2008 at 12:45:49PM -0400, Tom Lane wrote:
> > tqual.c: In function ‘HeapTupleSatisfiesVacuum’:
> > tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
> > tqual.c:1057: error: called from here
> > tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
> > tqual.c:1061: error: called from here
>
> Hmm, it's a bit disturbing that the compiler is taking it upon itself to
> decide that these calls are "unlikely".

Perhaps would should give it some idea about how likely they'd be,
because clearly it has no idea now.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

"Martijn van Oosterhout" <kleptog@svana.org> writes:

> On Wed, May 14, 2008 at 12:45:49PM -0400, Tom Lane wrote:
>> > tqual.c: In function ‘HeapTupleSatisfiesVacuum’:
>> > tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
>> > tqual.c:1057: error: called from here
>> > tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
>> > tqual.c:1061: error: called from here
>>
>> Hmm, it's a bit disturbing that the compiler is taking it upon itself to
>> decide that these calls are "unlikely".
>
> Perhaps would should give it some idea about how likely they'd be,
> because clearly it has no idea now.

Well they're buried in umpteen nested if/else blocks. I'm not really convinced
it's wrong actually. Each individual call site is actually quite unlikely in
the grand scheme of things.

The compiler is faced with two alternatives (or some mixture of the two).

Either a) it doesn't inline the function in which case the entire
HeapTupleSatisfiesMVCC likely fits in cpu cache and the entirety of
SetHintBits also likely fits in cache. On the other hand it incurs the
function call overhead on every call.

Or b) it inlines the function in all its myriad of call sites bloating
HeapTupleSatisfiesMVCC quite a bit. That avoids the function call overhead but
reduces the likelihood that the function remains in cache.

I think this is actually worth profiling on different architectures to make
sure we're not doing more harm than good here. Bloating HeapTupleSatisfiesMVCC
by what looks offhand to be probably easily a factor of 2 if not quite a bit
more could actually be slowing it down significantly. Using valgrind in
cachegrind mode might also be interesting.

The Linux kernel does have some macros meant to mark unlikely branches
(usually assertion failures) but I'm not sure how they work. And Gcc also has
a few optimizations which are driven by profiling data but I it doesn't sound
like this is one of them.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!

"Martijn van Oosterhout" <kleptog@svana.org> writes:

> On Wed, May 14, 2008 at 12:45:49PM -0400, Tom Lane wrote:
>> > tqual.c: In function ‘HeapTupleSatisfiesVacuum’:
>> > tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
>> > tqual.c:1057: error: called from here
>> > tqual.c:88: error: inlining failed in call to ‘SetHintBits’: call is unlikely and code size would grow
>> > tqual.c:1061: error: called from here
>>
>> Hmm, it's a bit disturbing that the compiler is taking it upon itself to
>> decide that these calls are "unlikely".
>
> Perhaps would should give it some idea about how likely they'd be,
> because clearly it has no idea now.

Fwiw, these two call sites are only for when HeapTupleSatisfiesMVCC finds a
tuples which has been moved away by VACUUM FULL... The latter for when it
finds such a tuple but the VACUUM FULL aborted.

It seems quite likely that the compiler is actually right (by chance) and we
shouldn't be optimizing those cases at the expense of more common cases.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production
Tuning

Gregory Stark <stark@enterprisedb.com> writes:
> Fwiw, these two call sites are only for when HeapTupleSatisfiesMVCC finds a
> tuples which has been moved away by VACUUM FULL... The latter for when it
> finds such a tuple but the VACUUM FULL aborted.

> It seems quite likely that the compiler is actually right (by chance) and we
> shouldn't be optimizing those cases at the expense of more common cases.

I trimmed the original message quite a bit --- there were a lot more
than two call sites that it was deciding not to inline.  Maybe it's
making the right choices or maybe not.
        regards, tom lane

On Wed, May 14, 2008 at 08:25:10PM +0100, Gregory Stark wrote:
> The Linux kernel does have some macros meant to mark unlikely branches
> (usually assertion failures) but I'm not sure how they work. And Gcc also has
> a few optimizations which are driven by profiling data but I it doesn't sound
> like this is one of them.

There's a macro called __builtin_expect() where you can specify what
the expected value of an expression is at runtime, so gcc can use this
to decide which branch is more likely, or how often a loop might run.
Normally you wrap it into macros like:

#define likely(x)    __builtin_expect(x,1)
#define unlikely(x)  __builtin_expect(x,0)

So you say things like:

if( likely( x==0 ) )

And gcc will optimise that the branch is likely to be taken. Using
macros means that you can arrange it so that for non-gcc compilers it's
a no-op.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

On Wed, 2008-05-14 at 20:25 +0100, Gregory Stark wrote:
> The Linux kernel does have some macros meant to mark unlikely branches
> (usually assertion failures) but I'm not sure how they work. And Gcc also has
> a few optimizations which are driven by profiling data but I it doesn't sound
> like this is one of them.

GCC's profile-driven optimization can be used to guide decisions about
both branch prediction/likelihood and function inlining. IMHO it is
definitely worth building the infrastructure to get Postgres builds with
profile-driven optimization -- certainly more maintainable and less
arbitrary than builtin_expect() and friends to me.

-Neil