Skip to content

bpo-37863: Optimize Fraction.__hash__()#15298

Merged
rhettinger merged 7 commits into
python:masterfrom
rhettinger:fraction_hash
Aug 16, 2019
Merged

bpo-37863: Optimize Fraction.__hash__()#15298
rhettinger merged 7 commits into
python:masterfrom
rhettinger:fraction_hash

Conversation

@rhettinger

@rhettinger rhettinger commented Aug 15, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@rhettinger rhettinger added the performance Performance or resource usage label Aug 15, 2019
tim-one
tim-one reviewed Aug 15, 2019
View reviewed changes

@tim-one tim-one left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Lib/fractions.py Outdated
serhiy-storchaka
serhiy-storchaka reviewed Aug 15, 2019
View reviewed changes
Comment thread Lib/fractions.py Outdated
Comment thread Lib/fractions.py
dinv = pow(self._denominator, _PyHASH_MODULUS - 2, _PyHASH_MODULUS)
try:
dinv = pow(self._denominator, -1, _PyHASH_MODULUS)
except ValueError:

@serhiy-storchaka serhiy-storchaka Aug 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what case ValueError is raised?

@tim-one tim-one Aug 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 serhiy-storchaka Aug 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. We need a test for this.

@serhiy-storchaka

serhiy-storchaka commented Aug 15, 2019

Copy link
Copy Markdown
Member

Any benchmarking results?

@rhettinger

rhettinger commented Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

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
serhiy-storchaka requested changes Aug 15, 2019
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Lib/fractions.py
dinv = pow(self._denominator, _PyHASH_MODULUS - 2, _PyHASH_MODULUS)
try:
dinv = pow(self._denominator, -1, _PyHASH_MODULUS)
except ValueError:

@serhiy-storchaka serhiy-storchaka Aug 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. We need a test for this.

@bedevere-bot

bedevere-bot commented Aug 15, 2019

Copy link
Copy Markdown

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

tim-one
tim-one reviewed Aug 15, 2019
View reviewed changes

@tim-one tim-one left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Lib/fractions.py
else:
hash_ = abs(self._numerator) * dinv % _PyHASH_MODULUS
result = hash_ if self >= 0 else -hash_
hash_ = hash(abs(self._numerator)) * dinv % _PyHASH_MODULUS

@tim-one tim-one Aug 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
mdickinson approved these changes Aug 15, 2019
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

serhiy-storchaka
serhiy-storchaka approved these changes Aug 15, 2019
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@rhettinger
rhettinger merged commit f3cb68f into python:master Aug 16, 2019
@rhettinger
rhettinger deleted the fraction_hash branch August 16, 2019 03:58
@bedevere-bot

bedevere-bot commented Aug 16, 2019

Copy link
Copy Markdown

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

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@rhettinger @lisroach
bpo-37863: Optimize Fraction.__hash__() (python#15298)
4010868
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@rhettinger @DinoV
bpo-37863: Optimize Fraction.__hash__() (python#15298)
e6113a7
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@rhettinger @websurfer5
bpo-37863: Optimize Fraction.__hash__() (python#15298)
a7ff52d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tim-one tim-one tim-one left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rhettinger @serhiy-storchaka @bedevere-bot @mdickinson @tim-one @the-knights-who-say-ni