Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4) - Mailing list pgsql-hackers

On 2013-01-10 10:55:20 +0100, Andres Freund wrote:
> 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...

The attached patch:

* adds configure checks for __VA_ARGS__ and __builtin_unreachable
  support
* provides a pg_unreachable() macro that expands to
  __builtin_unreachable() or abort()
* changes #define elog ... into #define elog(elevel, ...) if varargs are
  available

It does *not* combine elog_start and elog_finish into one function if
varargs are available although that brings a rather measurable
size/performance benefit.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services