On 2013-01-10 10:31:04 +0100, Andres Freund wrote: > On 2013-01-10 00:05:07 +0100, Andres Freund wrote: > > On 2013-01-09 17:28:35 -0500, Tom Lane wrote: > > > (We know this doesn't > > > matter, but gcc doesn't; this version of gcc apparently isn't doing much > > > with the knowledge that elog won't return.) > > > > Afaics one reason for that is that we don't have any such annotation for > > elog(), just for ereport. And I don't immediately see how it could be > > added to elog without relying on variadic macros. Bit of a shame, there > > probably a good number of callsites that could actually benefit from > > that knowledge. > > Is it worth making that annotation conditional on variadic macro > > support? > > A quick test confirms that. With: > > diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h > index cbbda04..39cd809 100644 > --- a/src/include/utils/elog.h > +++ b/src/include/utils/elog.h > @@ -212,7 +212,13 @@ extern int getinternalerrposition(void); > * elog(ERROR, "portal \"%s\" not found", stmt->portalname); > *---------- > */ > +#ifdef __GNUC__ > +#define elog(elevel, ...) elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), > \ > + elog_finish(elevel, __VA_ARGS__), > \ > + ((elevel) >= ERROR ? __builtin_unreachable() : (void) 0) > +#else > #define elog elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), elog_finish > +#endif > > extern void elog_start(const char *filename, int lineno, const char > *funcname); > extern void > > nearly the same code is generated for both variants (this is no > additionally saved registers and such). > > Two unfinished things: > * the above only checks for gcc although all C99 compilers should > support varargs macros > * It uses __builtin_unreachable() instead of abort() as that creates > a smaller executable. That's gcc 4.5 only. Saves about 30kb in a > both a stripped and unstripped binary. > * elog(ERROR) i.e. no args would require more macro ugliness, but that > seems unneccesary > > Doing the __builtin_unreachable() for ereport as well saves about 50kb. > > Given that some of those elog/ereports are in performance critical code > paths it seems like a good idea to remove code from the > callsites. Adding configure check for __VA_ARGS__ and > __builtin_unreachable() should be simple enough?
For whatever its worth - I am not sure its much - after relying on variadic macros we can make elog into one function call instead of two. That saves another 100kb. [tinker] The builtin_unreachable and combined elog thing bring a measurable performance benefit of a rather surprising 7% when running Pavel's testcase ontop of yesterdays HEAD. So it seems worth investigating. About 3% of that is the __builtin_unreachable() and about 4% the combining of elog_start/finish. Now that testcase sure isn't a very representative one for this, but I don't really see why it should benefit much more than other cases. Seems to be quite the benefit for relatively minor cost. Although I am pretty sure just copying the whole of elog_start/elog_finish into one elog_all() isn't the best way to go at this... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
