|From:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|To:||Michael Paquier <michael(at)paquier(dot)xyz>|
|Cc:||Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||Re: refactoring - share str2*int64 functions|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
>> I would put back unlikely() on overflow tests, as there are indeed unlikely
>> to occur and it may help some compilers, and cannot be harmful. It also
>> helps the code reader to know that these path are not expected to be taken
> Hm. I don't agree about that part, and the original signed portions
> don't do that. I think that we should let the callers of the routines
> decide if a problem is unlikely to happen or not as we do now.
Hmmm. Maybe inlining propates them, but otherwise they make sense for a
> I am still not convinced that this module is worth the extra cycles to
> justify its existence though.
They allow to quickly do performance tests, for me it is useful to keep it
around, but you are the committer, you do as you feel.
>> There is no doubt that dividing 64 bits integers is a very bad idea, at
>> least on my architecture!
> That's surprising. I cannot reproduce that.
It seems to me that somehow you can, you have a 5 to 18 seconds drop
below. There are actual reasons why some processors are more expensive
than others, it is not just marketing:-)
> 2-1) mul:
> - built-in: 5.1s
> - fallback (with uint128): 8.0s
> - fallback (without uint128): 18.1s
> So, the built-in option is always faster, and keeping the int128 path
> if available for the multiplication makes sense, but not for the
> subtraction and the addition.
> I am wondering if we should review further the signed part for add and
> sub, but I'd rather not touch it in this patch.
The signed overflows are trickier even, I have not paid attention to the
fallback code. I agree that it is better left untouched for know.
> If you have done any changes on my previous patch, or if you have a
> script to share I could use to attempt to reproduce your results, I
> would be happy to do so.
Hmmm. I did manual tests really. Attached a psql script replicating them.
# with builtin overflow detection
sh> psql < oc.sql
NOTICE: int 16 mul: 00:00:02.747269 # slow
NOTICE: int 16 add: 00:00:01.83281
NOTICE: int 16 sub: 00:00:01.8501
NOTICE: uint 16 mul: 00:00:03.68362 # slower
NOTICE: uint 16 add: 00:00:01.835294
NOTICE: uint 16 sub: 00:00:02.136895 # slow
NOTICE: int 32 mul: 00:00:01.828065
NOTICE: int 32 add: 00:00:01.840269
NOTICE: int 32 sub: 00:00:01.843557
NOTICE: uint 32 mul: 00:00:02.447052 # slow
NOTICE: uint 32 add: 00:00:01.849899
NOTICE: uint 32 sub: 00:00:01.840773
NOTICE: int 64 mul: 00:00:01.839051
NOTICE: int 64 add: 00:00:01.839065
NOTICE: int 64 sub: 00:00:01.838599
NOTICE: uint 64 mul: 00:00:02.161346 # slow
NOTICE: uint 64 add: 00:00:01.839404
NOTICE: uint 64 sub: 00:00:01.838549
> So, do you have more comments?
No other comments.
|Next Message||Pavel Stehule||2019-09-01 18:12:15||Re: Proposal: roll pg_stat_statements into core|
|Previous Message||David Fetter||2019-09-01 18:00:26||Proposal: roll pg_stat_statements into core|