Re: refactoring - share str2*int64 functions

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
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 13:10:22
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sun, Sep 01, 2019 at 01:57:06PM +0200, Fabien COELHO wrote:
> 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.

> On reflection, I'm not sure that add_u64 and sub_u64 overflow with uint128
> are useful. The res < a or b > a tricks should suffice, just like for u16
> and u32 cases, and it may cost a little less anyway.

Actually, I agree and this is something I can see as well with some
extra measurements. mul_u64 without int128 is twice slower, while
add_u64 and sub_u64 are 15~20% faster.

> I would suggest keep the overflow extension as "contrib/overflow_test". For
> mul tests, I'd suggest not to try only min/max values like add/sub, but also
> "standard" multiplications that overflow or not. It would be good if "make
> check" could be made to work", for some reason it requires "installcheck".

Any extensions out of core can only work with "installcheck", and
check is not supported (see I am still not convinced that
this module is worth the extra cycles to justify its existence

> 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. Are you sure that you
didn't just undefine HAVE_INT128? This would cause
HAVE__BUILTIN_OP_OVERFLOW to still be active in all the code paths.
Here are a couple of results from my side with this query, FWIW, and
some numbers for all the compile flags (-O2 used):
select pg_overflow_check(10000, 10000, 2000000000, 'XXX', 'XXX');
1) uint16:
1-1) mul:
- built-in: 5.5s
- fallback: 5.5s
1-2) sub:
- built-in: 5.3s
- fallback: 5.4s
1-3) add:
- built-in: 5.3s
- fallback: 6.2s
2) uint32:
2-1) mul:
- built-in: 5.1s
- fallback: 5.9s
2-2) sub:
- built-in: 5.2s
- fallback: 5.4s
2-3) add:
- built-in: 5.2s
- fallback: 6.2s
2) uint64:
2-1) mul:
- built-in: 5.1s
- fallback (with uint128): 8.0s
- fallback (without uint128): 18.1s
2-2) sub:
- built-in: 5.2s
- fallback (with uint128): 7.1s
- fallback (without uint128): 5.5s
2-3) add:
- built-in: 5.2s
- fallback (with uint128): 7.1s
- fallback (without uint128): 6.3s

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.

> Note that checks depends on value, so actual performance may vary depending
> on actual val1 and val2 passed. I used 10000 10000 like your example.

Sure. Still that offers helpful hints as we do the same operations
for all code paths the same number of times.

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.

So, do you have more comments?

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Paul A Jungwirth 2019-09-01 13:26:11 Re: range_agg
Previous Message Fabien COELHO 2019-09-01 11:57:06 Re: refactoring - share str2*int64 functions