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

Hi,

Thanks for picking this up again!

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 ...?


> 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 suspect that doing this for DEBUG* could also improve the code for
clang, because we obviously don't have __builtin_unreachable after those.


> I see no reason why the compiler shouldn't always fold that constant
> expression at compile-time and correctly select the correct version of
> the function for the job.  (I also tried using __builtin_choose_expr()
> but GCC complained when the elevel was not constant, even with the
> __builtin_constant_p() test in the condition)

I think it has to be constant in all paths for that...


> Master:
> drowley@amd3990x:~$ pgbench -S -T 120 postgres
> tps = 25245.903255 (excluding connections establishing)
> tps = 26144.454208 (excluding connections establishing)
> tps = 25931.850518 (excluding connections establishing)
> 
> Master + elog_ereport_attribute_cold.patch
> drowley@amd3990x:~$ pgbench -S -T 120 postgres
> tps = 28351.480631 (excluding connections establishing)
> tps = 27763.656557 (excluding connections establishing)
> tps = 28896.427929 (excluding connections establishing)

That is pretty damn cool.


> 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?


>  /*
>   * 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.




>  #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.

Greetings,

Andres Freund