Re: refactoring - share str2*int64 functions

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 pgxs.mk). I am still not convinced that
this module is worth the extra cycles to justify its existence
though.

> 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?
--
Michael