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
>From 94a3f93f4c8a381358a4dc52a43ec60a8f3e33f6 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Fri, 11 Jan 2013 16:58:17 +0100
Subject: [PATCH] Mark elog() as not returning if the capabilities of the C
compiler allow it
To do that:
* Add a configure check for __VA_ARGS__ support
* Provide pg_unreachable() macro which expands to__builtin_unreachable() if the
compiler supports it, abort() otherwise
Marking it as such improves code generation and reduces executable size.
---
config/c-compiler.m4 | 41 +++++++++++++++++++++++++++++++++++++++++
configure.in | 2 ++
src/include/c.h | 10 ++++++++++
src/include/utils/elog.h | 18 +++++++++++++++---
4 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 7cbb8ec..f3efe72 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -161,6 +161,47 @@ fi])# PGAC_C_TYPES_COMPATIBLE
+# PGAC_C_VA_ARGS
+# -----------------------
+# Check if the C compiler understands C99 style variadic macros and define
+# HAVE__VA_ARGS if so.
+AC_DEFUN([PGAC_C_VA_ARGS],
+[AC_CACHE_CHECK(for __VA_ARGS__, pgac_cv__va_args,
+[AC_TRY_COMPILE([#include <stdio.h>],
+[#define debug(...) fprintf(stderr, __VA_ARGS__)
+debug("%s", "blarg");
+],
+[pgac_cv__va_args=yes],
+[pgac_cv__va_args=no])])
+if test x"$pgac_cv__va_args" = xyes ; then
+AC_DEFINE(HAVE__VA_ARGS, 1,
+ [Define to 1 if your compiler understands C99 __VA_ARGS__ in macros.])
+fi])# PGAC_C_VA_ARGS
+
+
+
+# PGAC_C_UNREACHABLE
+# -----------------------
+
+# Check if the C compiler understands __builtin_unreachable() and define
+# HAVE__BUILTIN_UNREACHABLE if so.
+#
+# NB: Don't get the idea of putting a for(;;); or such before the
+# __builtin_unreachable() in that case some compilers remove it before linking
+# and only a warning instead of an error will be produced.
+AC_DEFUN([PGAC_C_UNREACHABLE],
+[AC_CACHE_CHECK(for __builtin_unreachable, pgac_cv__builtin_unreachable,
+[AC_TRY_LINK([],
+[__builtin_unreachable();],
+[pgac_cv__builtin_unreachable=yes],
+[pgac_cv__builtin_unreachable=no])])
+if test x"$pgac_cv__builtin_unreachable" = xyes ; then
+AC_DEFINE(HAVE__BUILTIN_UNREACHABLE, 1,
+ [Define to 1 if your compiler understands __builtin_unreachable.])
+fi])# PGAC_C_UNREACHABLE
+
+
+
# PGAC_PROG_CC_CFLAGS_OPT
# -----------------------
# Given a string, check if the compiler supports the string as a
diff --git a/configure.in b/configure.in
index e2682f3..13ac2ab 100644
--- a/configure.in
+++ b/configure.in
@@ -1106,6 +1106,8 @@ AC_C_VOLATILE
PGAC_C_FUNCNAME_SUPPORT
PGAC_C_STATIC_ASSERT
PGAC_C_TYPES_COMPATIBLE
+PGAC_C_VA_ARGS
+PGAC_C_UNREACHABLE
PGAC_STRUCT_TIMEZONE
PGAC_UNION_SEMUN
PGAC_STRUCT_SOCKADDR_UN
diff --git a/src/include/c.h b/src/include/c.h
index c30df8b..77c3319 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -831,6 +831,16 @@ typedef NameData *Name;
#define STATIC_IF_INLINE
#endif /* PG_USE_INLINE */
+/*
+ * Cross Platform support to mark locations as unreachable to avoid warnings
+ * and to aid code generation.
+ */
+#ifdef HAVE__BUILTIN_UNREACHABLE
+#define pg_unreachable() __builtin_unreachable()
+#else
+#define pg_unreachable() abort()
+#endif
+
/* ----------------------------------------------------------------
* Section 7: random stuff
* ----------------------------------------------------------------
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index cbbda04..d6e1054 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -109,7 +109,7 @@
#define ereport_domain(elevel, domain, rest) \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
(errfinish rest) : (void) 0), \
- ((elevel) >= ERROR ? abort() : (void) 0)
+ ((elevel) >= ERROR ? pg_unreachable() : (void) 0)
#define ereport(elevel, rest) \
ereport_domain(elevel, TEXTDOMAIN, rest)
@@ -210,10 +210,22 @@ extern int getinternalerrposition(void);
/*----------
* Old-style error reporting API: to be used in this way:
* elog(ERROR, "portal \"%s\" not found", stmt->portalname);
+ *
+ * If we have support for variadic macros available we can signal that elog
+ * won't return with an elevel >= ERROR which can lead to better code being
+ * generated.
*----------
*/
-#define elog elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), elog_finish
-
+#ifdef HAVE__VA_ARGS
+#define elog(elevel, ...) \
+ elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), \
+ elog_finish(elevel, __VA_ARGS__), \
+ ((elevel) >= ERROR ? pg_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
elog_finish(int elevel, const char *fmt,...)
--
1.7.12.289.g0ce9864.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
