Re: An unlikely() experiment

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