Thread

1. What to do with inline warnings?

   Peter Eisentraut <peter_e@gmx.net> — 2008-05-14T16:08:09Z

   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
   
   

2. Re: What to do with inline warnings?

   Tom Lane <tgl@sss.pgh.pa.us> — 2008-05-14T16:45:49Z

   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
   
   

3. Re: What to do with inline warnings?

   Martijn van Oosterhout <kleptog@svana.org> — 2008-05-14T17:31:33Z

   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.
   

4. Re: What to do with inline warnings?

   Gregory Stark <stark@enterprisedb.com> — 2008-05-14T19:25:10Z

   "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!
   
   

5. Re: What to do with inline warnings?

   Gregory Stark <stark@enterprisedb.com> — 2008-05-14T19:35:37Z

   "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
   
   

6. Re: What to do with inline warnings?

   Tom Lane <tgl@sss.pgh.pa.us> — 2008-05-14T19:48:36Z

   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
   
   

7. Re: What to do with inline warnings?

   Martijn van Oosterhout <kleptog@svana.org> — 2008-05-14T21:32:01Z

   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.
   

8. Re: What to do with inline warnings?

   Neil Conway <neilc@samurai.com> — 2008-05-16T00:12:39Z

   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