Re: refactoring - share str2*int64 functions - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: refactoring - share str2*int64 functions
Date
Msg-id alpine.DEB.2.21.1909011914520.11815@lancre
Whole thread Raw
In response to Re: refactoring - share str2*int64 functions  (Michael Paquier <michael@paquier.xyz>)
Responses Re: refactoring - share str2*int64 functions
List pgsql-hackers
Michaël,

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

Yep.

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

> So, do you have more comments?

No other comments.

-- 
Fabien.
Attachment

pgsql-hackers by date:

Previous
From: David Fetter
Date:
Subject: Proposal: roll pg_stat_statements into core
Next
From: Pavel Stehule
Date:
Subject: Re: Proposal: roll pg_stat_statements into core