Skip site navigation (1) Skip section navigation (2)

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

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>,pgsql-hackers(at)postgresql(dot)org
Subject: Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date: 2013-01-10 09:55:20
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
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.


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


Andres Freund

 Andres Freund	         
 PostgreSQL Development, 24x7 Support, Training & Services

In response to


pgsql-hackers by date

Next:From: Marko TiikkajaDate: 2013-01-10 12:23:10
Subject: pg_dump --pretty-print-views
Previous:From: Simon RiggsDate: 2013-01-10 09:48:23
Subject: Re: Index build temp files

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group