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

From Andres Freund
Subject Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date
Msg-id 20130110093104.GA4329@awork2.anarazel.de
Whole thread Raw
In response to Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)  (Andres Freund <andres@2ndquadrant.com>)
Responses Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
List pgsql-hackers
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
+#endifextern 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
about30kb 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?

Greetings,

Andres Freund

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



pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]
Next
From: Simon Riggs
Date:
Subject: Re: Index build temp files