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.
oc.sql
Description: application/sql
