Обсуждение: 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