Re: [HACKERS] An unlikely() experiment

[David Rowley]

On 30 October 2017 at 22:44, Andres Freund  wrote:
> On 2017-10-30 22:39:01 +1300, David Rowley wrote:
>> Today I was thinking, to get around that issue, we might be able to
>> generate another thin wrapper around elog_start() and mark that as
>> __attribute__((cold)) and fudge the macro a bit to call that function
>> instead if it can detect a compile time const level >= ERROR. I've not
>> looked at the code again to remind myself if that would be possible.
>
> Yes, that's what I was thinking too. Add a elog_fatal() wrapping
> elog_finish(), and move the if (__builtin_constant_p(elevel)) branch a
> bit earlier, and that should work.  Similar with errstart_fatal() for
> ereport().

This may have been too good to be true. I can't seem to get it to work
and I think it's because the function is inside the do{}while(0) and
the if (__builtin_constant_p(elevel) && (elevel) >= ERROR) branch,
which appears to mean that:

"The paths leading to call of cold functions within code are marked as
unlikely by the branch prediction mechanism" [1]

is not the path that the macro is in in the calling function, like we
might have hoped.

I can get the assembly to change if I put an unlikely() around the
condition or if I go and vandalize the macro to become:

#define elog(elevel, ...) \
elog_start_error(__FILE__, __LINE__, PG_FUNCNAME_MACRO)

[1] https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Function-Attributes.html



-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] An unlikely() experiment

[Andres Freund]

On 2017-10-30 22:39:01 +1300, David Rowley wrote:
> On 30 October 2017 at 22:34, Andres Freund  wrote:
> > Hi,
> >
> > On 2015-12-20 02:49:13 +1300, David Rowley wrote:
> >> Alternatively, if there was some way to mark the path as cold from within
> >> the path, rather than from the if() condition before the path, then we
> >> could perhaps do something in the elog() macro instead. I just couldn't
> >> figure out a way to do this.
> >
> > I just was thinking about this, and it turns out that
> > __attribute__((cold)) does what we need here. We could just mark
> > elog_start() and errstart() as cold, and afaict that should do the
> > trick.
> 
> I had actually been thinking about this again today. I had previously
> not thought  __attribute__((cold)) would be a good idea since if we
> mark elog_start() with that, then code paths with elog(NOTICE) and
> elog(LOG) not to mention DEBUG would be marked as cold.

That's an issue, true. Although I'm not convinced it's particularly
fatal - if you're ok with emitting debugging output in places it's
probably not the hottest codepath in the first place. And profile guided
optimization would overwrite hot/cold.

But:

> Today I was thinking, to get around that issue, we might be able to
> generate another thin wrapper around elog_start() and mark that as
> __attribute__((cold)) and fudge the macro a bit to call that function
> instead if it can detect a compile time const level >= ERROR. I've not
> looked at the code again to remind myself if that would be possible.

Yes, that's what I was thinking too. Add a elog_fatal() wrapping
elog_finish(), and move the if (__builtin_constant_p(elevel)) branch a
bit earlier, and that should work.  Similar with errstart_fatal() for
ereport().

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] An unlikely() experiment

[David Rowley]

On 30 October 2017 at 22:34, Andres Freund  wrote:
> Hi,
>
> On 2015-12-20 02:49:13 +1300, David Rowley wrote:
>> Alternatively, if there was some way to mark the path as cold from within
>> the path, rather than from the if() condition before the path, then we
>> could perhaps do something in the elog() macro instead. I just couldn't
>> figure out a way to do this.
>
> I just was thinking about this, and it turns out that
> __attribute__((cold)) does what we need here. We could just mark
> elog_start() and errstart() as cold, and afaict that should do the
> trick.

I had actually been thinking about this again today. I had previously
not thought  __attribute__((cold)) would be a good idea since if we
mark elog_start() with that, then code paths with elog(NOTICE) and
elog(LOG) not to mention DEBUG would be marked as cold. Today I was
thinking, to get around that issue, we might be able to generate
another thin wrapper around elog_start() and mark that as
__attribute__((cold)) and fudge the macro a bit to call that function
instead if it can detect a compile time const level >= ERROR. I've not
looked at the code again to remind myself if that would be possible.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] An unlikely() experiment

[Andres Freund]

Hi,

On 2015-12-20 02:49:13 +1300, David Rowley wrote:
> Alternatively, if there was some way to mark the path as cold from within
> the path, rather than from the if() condition before the path, then we
> could perhaps do something in the elog() macro instead. I just couldn't
> figure out a way to do this.

I just was thinking about this, and it turns out that
__attribute__((cold)) does what we need here. We could just mark
elog_start() and errstart() as cold, and afaict that should do the
trick.

Thanks to Jakub from #gcc for pointing that out.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] An unlikely() experiment

[Andres Freund]

On 2015-12-20 14:21:14 +1300, David Rowley wrote:
> On 20 December 2015 at 03:06, Andres Freund  wrote:
> > 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)
> >
> 
> Interesting idea. Would you think that would be something we could do a
> complete replace on, or are you thinking just for the hotter code paths?

More or less complete. Generally, logging shouldn't be a hot code
path. A single wrong branch won't be noticeable in case we're logging
something, not to speak of an actual error case.  As far as I can see
there's unfortunately no way to declare a branch unlikely from inside
that branch, otherwise I'd have said we should just stick an unlikely
equivalent in the elog/ereport definition itself.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] An unlikely() experiment

[David Rowley]

On 20 December 2015 at 03:06, Andres Freund  wrote:

> On 2015-12-20 02:49:13 +1300, David Rowley wrote:
> > 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.
>
>
Good thinking. It would be interesting to see benchmarks with the hacked
version of elog()

To make sure I understand correctly: Before doing that you manually
> added the errcheck()'s manually? At each elog(ERROR) callsite?
>
>
Yeah manually at most call sites. Although I did nothing in cases like;

if (somethinggood)
   dosomethinggood();
else
  elog(ERROR, ...);

Quite possibly in that case we could use likely(), but there's also cases
where elog() is in the default: in a switch(), or contained in an else in
an if/else if/else block.

I also left out errcheck() in places where the condition called a function
with some side affect. I did this as I also wanted to test a hard coded
false condition to see how much overhead remained. I did nothing with
ereport().


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

Interesting idea. Would you think that would be something we could do a
complete replace on, or are you thinking just for the hotter code paths?

I've attached the patch this time, it should apply cleanly to bbbd807. I'd
would be good to see some other performance tests on some other hardware.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


errcheck1.patch.bz2
Description: BZip2 compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] An unlikely() experiment

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers