Re: An unlikely() experiment - Mailing list pgsql-hackers

Hi,

On 2015-12-20 02:49:13 +1300, David Rowley wrote:
> Many of you might know of GCC's __builtin_expect() and that it allows us to
> tell the compiler which code branches are more likely to occur.

It also tells the reader that, which I find also rather helpful.


> The documentation on this does warn that normally it's a bad idea to
> use this "as programmers are notoriously bad at predicting how their
> programs actually perform" [1].

I think we indeed have to careful to not add this to 40/60 type
branches. But 99/1 or so paths are ok.


> So I choose to ignore that, and give
> it a try anyway... elog(ERROR) for example, surely that branch should
> always be in a cold path? ... ereport() too perhaps?

Hm, not exactly what you were thinking of, but it'd definitely be
interesting to add unlikely annotations to debug elogs, in some
automated manner (__builtin_choose_expr?). I've previously hacked
elog/ereport to only support >= ERROR, and that resulted in a speedup,
removing a log of elog(DEBUG*) triggered jumps.


> Anyway, I knocked up a patch which went an added an errcheck() macro which
> is defined the same as unlikely() is, and I stuck these around just the
> "if" conditions for tests before elog(ERROR) calls, such as:

Personally I'd rather go with a plain unlikely() - I don't think
errcheck as a name really buys us that much, and it's a bit restrictive.


> gcc version 5.2.1 on i7-4712HQ
>
> pgbench -M prepared -T 60 -S
> Master: 12246 tps
> Patched 13132 tsp (107%)
>
> pgbench -M prepared -T 60 -S -c 8 -j 8
> Master: 122982 tps
> Patched: 129318 tps (105%)

Not bad at all.


> Ok, so perhaps it's not that likely that we'd want to litter these
> errcheck()s around everywhere, so I added a bit more logging as I thought
> it's probably just a handful of calls that are making this improvement. I
> changed the macro to call a function to log the __FILE__ and __LINE__, then
> I loaded the results into Postgres, aggregated by file and line and sorted
> by count(*) desc. Here's a sample of them (against bbbd807):
>
>        file       | line | count
> ------------------+------+--------
>  fmgr.c           | 1326 | 158756
>  mcxt.c           |  902 | 147184
>  mcxt.c           |  759 | 113162
>  lwlock.c         | 1545 |  67585
>  lwlock.c         |  938 |  67578
>  stringinfo.c     |  253 |  55364
>  mcxt.c           |  831 |  47984
>  fmgr.c           | 1030 |  36271
>  syscache.c       |  978 |  28182
>  dynahash.c       |  981 |  22721
>  dynahash.c       | 1665 |  19994
>  mcxt.c           |  931 |  18594

To make sure I understand correctly: Before doing that you manually
added the errcheck()'s manually? At each elog(ERROR) callsite?


> Perhaps it would be worth doing something with the hottest ones maybe?

One way to do this would be to add elog_on() / ereport_on() macros,
directly containing the error message. Like
#define elog_on(cond, elevel, ...) \do { \          if (unlikely(cond)) \          { \               elog(elevel,
__VA_ARGS__)\          } \        } while(0)
 

Greetings,

Andres