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

Bonjour Michaël,

> You are right as well that having symmetry with the signed methods is 
> much better.  In order to see the difference, you can just do that with 
> the extension attached, after of course hijacking int.h with some undefs 
> and recompiling the backend and the module: select 
> pg_overflow_check(10000, 10000, 2000000000, 'uint32', 'mul');

Ok.

>>> still it is possible to trick things with signed integer arguments.
>>
>> Is it?
>
> If you pass -1 and then you can fall back to the maximum of each 16,
> 32 or 64 bits for the unsigned (see the regression tests I added with
> the module).

> Attached is also an updated version of the module I used to validate
> this stuff.  Fabien, any thoughts?

Patch apply cleanly, compiles, "make check" ok (although changes 
are untested).

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.

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.

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

I could not test performance directly, loops are optimized out by the 
compiler. I added "volatile" on input value declarations to work around 
that. On 2B iterations I got on my laptop:

  int16: mul = 2770 ms, add = 1830 ms, sub = 1826 ms
  int32: mul = 1838 ms, add = 1835 ms, sub = 1840 ms
  int64: mul = 1836 ms, add = 1834 ms, sub = 1833 ms

  uint16: mul = 3670 ms, add = 1830 ms, sub = 2148 ms
  uint32: mul = 2438 ms, add = 1834 ms, sub = 1831 ms
  uint64: mul = 2139 ms, add = 1841 ms, sub = 1882 ms

Why int16 mul, uint* mul and uint16 sub are bad is unclear.

With fallback code triggered with:

  #undef HAVE__BUILTIN_OP_OVERFLOW

  int16: mul = 1433 ms, add = 1424 ms, sub = 1254 ms
  int32: mul = 1433 ms, add = 1425 ms, sub = 1443 ms
  int64: mul = 1430 ms, add = 1429 ms, sub = 1441 ms

  uint16: mul = 1445 ms, add = 1291 ms, sub = 1265 ms
  uint32: mul = 1419 ms, add = 1434 ms, sub = 1493 ms
  uint64: mul = 1266 ms, add = 1430 ms, sub = 1440 ms

For some unclear reason, 4 tests are significantly faster.

Forcing further down fallback code with:

   #undef HAVE_INT128

  int64: mul = 1424 ms, add = 1429 ms, sub = 1440 ms
  uint64: mul = 24145 ms, add = 1434 ms, sub = 1435 ms

There is no doubt that dividing 64 bits integers is a very bad idea, at 
least on my architecture!

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.

These results are definitely depressing because the fallback code is 
nearly twice as fast as the builtin overflow detection version. For the 
record: gcc 7.4.0 on ubuntu 18.04 LTS. Not sure what to advise, relying on 
the builtin should be the better idea…

-- 
Fabien.