Re: Keep elog(ERROR) and ereport(ERROR) calls in the cold path

On Fri, 26 Jun 2020 at 04:35, Andres Freund <andres@anarazel.de> wrote:
> On 2020-06-25 13:50:37 +1200, David Rowley wrote:
> > In the attached, I've come up with a way that works. Basically I've
> > just added a function named errstart_cold() that is attributed with
> > __attribute__((cold)), which will hint to the compiler to keep
> > branches which call this function in a cold path.
>
> I recall you trying this before? Has that gotten easier because we
> evolved ereport()/elog(), or has gcc become smarter, or ...?

Yeah, I appear to have tried it and found it not to work in [1]. I can
only assume GCC got smarter in regards to how it marks a path as cold.
Previously it seemed not do due to the do/while(0). I'm pretty sure
back when I tested last that ditching the do while made it work, just
we can't really get rid of it.

> > To make the compiler properly mark just >= ERROR calls as cold, and
> > only when the elevel is a constant, I modified the ereport_domain
> > macro to do:
> >
> > if (__builtin_constant_p(elevel) && (elevel) >= ERROR ? \
> > errstart_cold(elevel, domain) : \
> > errstart(elevel, domain)) \
>
> I think it'd be good to not just do this for ERROR, but also for <=
> DEBUG1. I recall seing quite a few debug elogs that made the code worse
> just by "being there".

I think that case is different. We don't want to move the entire elog
path into the cold path for that. We'd only want to hint that errstart
is unlikely to return true if elevel <= DEBUG1

> > diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
> > index e976201030..8076e8af24 100644
> > --- a/src/backend/utils/error/elog.c
> > +++ b/src/backend/utils/error/elog.c
> > @@ -219,6 +219,19 @@ err_gettext(const char *str)
> >  #endif
> >  }
> >
> > +#if defined(HAVE_PG_ATTRIBUTE_HOT_AND_COLD) && defined(HAVE__BUILTIN_CONSTANT_P)
> > +/*
> > + * errstart_cold
> > + *           A simple wrapper around errstart, but hinted to be cold so that the
> > + *           compiler is more likely to put error code in a cold area away from the
> > + *           main function body.
> > + */
> > +bool
> > +pg_attribute_cold errstart_cold(int elevel, const char *domain)
> > +{
> > +     return errstart(elevel, domain);
> > +}
> > +#endif
>
> Hm. Would it make sense to have this be a static inline?

I thought about that but didn't try it to ensure it still worked ok. I
didn't think it was that important to make sure we don't get the extra
function hop for ERRORs. It seemed like a case we'd not want to really
optimise for.

> >  /*
> >   * errstart --- begin an error-reporting cycle
> > diff --git a/src/include/c.h b/src/include/c.h
> > index d72b23afe4..087b8af6cb 100644
> > --- a/src/include/c.h
> > +++ b/src/include/c.h
> > @@ -178,6 +178,21 @@
> >  #define pg_noinline
> >  #endif
> >
> > +/*
> > + * Marking certain functions as "hot" or "cold" can be useful to assist the
> > + * compiler in arranging the assembly code in a more efficient way.
> > + * These are supported from GCC >= 4.3 and clang >= 3.2
> > + */
> > +#if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3))) || \
> > +     (defined(__clang__) && (__clang_major__ > 3 || (__clang_major__ == 3 && __clang_minor__ >= 2)))
> > +#define HAVE_PG_ATTRIBUTE_HOT_AND_COLD 1
> > +#define pg_attribute_hot __attribute__((hot))
> > +#define pg_attribute_cold __attribute__((cold))
> > +#else
> > +#define pg_attribute_hot
> > +#define pg_attribute_cold
> > +#endif
>
> Wonder if we should start using __has_attribute() for things like this.
>
> https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fattribute.html
>
> I.e. we could do something like
> #ifndef __has_attribute
> #define __has_attribute(attribute) 0
> #endif
>
> #if __has_attribute(hot)
> #define pg_attribute_hot __attribute__((hot))
> #else
> #define pg_attribute_hot
> #endif
>
> clang added __has_attribute in 2.9 (2010), gcc added it in 5 (2014), so
> I don't think we'd loose too much.

Thanks for pointing that out. Seems like a good idea to me. I don't
think we'll upset too many people running GCC 4.4 to 5.0. I can't
imagine many people serious about performance will be using a
PostgreSQL version that'll be released in 2021 with a pre 2014
compiler.

> >  #ifdef HAVE__BUILTIN_CONSTANT_P
> > +#ifdef HAVE_PG_ATTRIBUTE_HOT_AND_COLD
> > +#define ereport_domain(elevel, domain, ...)  \
> > +     do { \
> > +             pg_prevent_errno_in_scope(); \
> > +             if (__builtin_constant_p(elevel) && (elevel) >= ERROR ? \
> > +                      errstart_cold(elevel, domain) : \
> > +                      errstart(elevel, domain)) \
> > +                     __VA_ARGS__, errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
> > +             if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
> > +                     pg_unreachable(); \
> > +     } while(0)
> > +#else                                                        /* !HAVE_PG_ATTRIBUTE_HOT_AND_COLD */
> >  #define ereport_domain(elevel, domain, ...)  \
> >       do { \
> >               pg_prevent_errno_in_scope(); \
> > @@ -129,6 +141,7 @@
> >               if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
> >                       pg_unreachable(); \
> >       } while(0)
> > +#endif                                                       /* HAVE_PG_ATTRIBUTE_HOT_AND_COLD */
> >  #else                                                        /* !HAVE__BUILTIN_CONSTANT_P */
> >  #define ereport_domain(elevel, domain, ...)  \
> >       do { \
> > @@ -146,6 +159,9 @@
>
> Could we do this without another copy? Feels like we should be able to
> just do that in the existing #ifdef HAVE__BUILTIN_CONSTANT_P if we just
> add errstart_cold() independent HAVE_PG_ATTRIBUTE_HOT_AND_COLD.

Yeah. I just did it that way so we didn't get the extra function hop
in compilers that don't support __attribute((cold)). If I can inline
errstart_cold() and have the compiler still properly determine that
it's a cold function, then it seems wise to do it that way. If not,
then I'll need to keep a separate macro.

David

[1] https://www.postgresql.org/message-id/20171030094449.ffqhvt5n623zvyja%40alap3.anarazel.de