Re: refactoring - share str2*int64 functions

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
Date: 2019-09-01 18:07:06
Message-ID: alpine.DEB.2.21.1909011914520.11815@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


>> 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
>> often.
> 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
compiler perspective.

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


Attachment Content-Type Size
oc.sql application/x-sql 577 bytes

In response to


Browse pgsql-hackers by date

  From Date Subject
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