bpo-37863: Optimize Fraction.__hash__()

[rhettinger]

https://bugs.python.org/issue37863

[tim-one]

Wow - that was fast! 😉 Just left a comment about a simplification.

[serhiy-storchaka]

In what case ValueError is raised?

[tim-one]

ValueError is raised if and only if _PyHASH_MODULUS divides self._denominator. There is no modular inverse if and only if the gcd of the base and the modulus is non-trivial (not 1), but the modulus is a prime in this case, so in this case that's the same as the base being a multiple of the modulus.

[serhiy-storchaka]

Thank you. We need a test for this.

[serhiy-storchaka]

Any benchmarking results?

[rhettinger]

Baseline:

$ ./python.exe -r11 -s 'from fractions import Fraction as F' -s 'f=F(123412343, 58145823453)' 'hash(f)'
10000 loops, best of 11: 21.5 usec per loop
$ ./python.exe -r11 -s 'from fractions import Fraction as F' -s 'f=F(1243, 5813)' 'hash(f)'
20000 loops, best of 11: 19.8 usec per loop
$ ./python.exe -r11 -s 'from fractions import Fraction as F' -s 'f=F(122345234523453412343, 58145234523452345823453)' 'hash(f)'
10000 loops, best of 11: 23.7 usec per loop

Patched

$ ./python.exe -r11 -s 'from fractions import Fraction as F' -s 'f=F(123412343, 58145823453)' 'hash(f)'
100000 loops, best of 11: 3.17 usec per loop
$ ./python.exe -r11 -s 'from fractions import Fraction as F' -s 'f=F(1243, 5813)' 'hash(f)'
100000 loops, best of 11: 2.04 usec per loop
$ ./python.exe -r11 -s 'from fractions import Fraction as F' -s 'f=F(122345234523453412343, 58145234523452345823453)' 'hash(f)'
50000 loops, best of 11: 5.69 usec per loop

[serhiy-storchaka]

Please add a test for the denominator being a multiple of the modulus.

[serhiy-storchaka]

Thank you. We need a test for this.

[bedevere-bot]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

[tim-one]

Added a suggestion for something else to try, avoiding the remaining division (mod) at the cost of another hash() call. At best minor, but possibly significant.

[tim-one]

Another possibility is to write this as

hash_ = hash(hash(abs(self._numerator)) * dinv)

to avoid the (in general) 122-by-61 bit % division. The int hash should compute the same thing, but doesn't do any division (it's all bit rotations and adds, one pair per internal int "digit"). (While the docs don't point this out, reduction by _PyHASH_MODULUS in binary is akin to "casting out nines" in decimal to find a remainder mod 9 - no division is really needed.)

[mdickinson]

LGTM

[serhiy-storchaka]

Great!

[bedevere-bot]

@rhettinger: Please replace # with GH- in the commit message next time. Thanks!