Skip to content

Integer can be hashed rapidly as well #58440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

adienes
Copy link
Member

@adienes adienes commented May 16, 2025

as noted in #58388 (comment) , using hash_bytes for BigInt limbs in isolation will break some hashing invariants. accordingly this PR updates also the hash_integer fallback to use rapidhash

some surgery was needed to make BigFloat and Rational and such still match, but I think (hope?) I got it all

below are some benchmarks. the y axis is nanoseconds and the x axis is an input of size 1234^x, so length = 10 means input hash(big(1234^10))

hashing BigFloat got a small bit slower, but this already allocates and already seems less common than hashing BigInt (whose hashing remains non-allocating)

image
image

@adienes adienes added performance Must go faster bignums BigInt and BigFloat hashing labels May 16, 2025
@@ -209,7 +275,7 @@ end
else
pos = 1
i = buflen
while i 48
if i > 48
Copy link
Member Author

Choose a reason for hiding this comment

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

this was just an unrelated existing typo

@@ -182,7 +182,7 @@ isinteger(::AbstractIrrational) = false
iszero(::AbstractIrrational) = false
isone(::AbstractIrrational) = false

hash(x::Irrational, h::UInt) = 3*objectid(x) - h
hash(x::Irrational, h::UInt) = 3h - objectid(x)
Copy link
Member Author

Choose a reason for hiding this comment

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

also unrelated to the meat of this PR, but makes hash(::Irrational) more consistent with hash(::Any

@adienes
Copy link
Member Author

adienes commented May 16, 2025

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

1 packages crashed only on the current version.

  • The process was aborted: 1 packages

5 packages crashed on the previous version too.

✖ Packages that failed

24 packages failed only on the current version.

  • Package has test failures: 3 packages
  • Package tests unexpectedly errored: 1 packages
  • Tests became inactive: 1 packages
  • Test duration exceeded the time limit: 19 packages

1444 packages failed on the previous version too.

✔ Packages that passed tests

21 packages passed tests only on the current version.

  • Other: 21 packages

5221 packages passed tests on the previous version too.

~ Packages that at least loaded

4 packages successfully loaded only on the current version.

  • Other: 4 packages

2925 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

921 packages were skipped on the previous version too.

@adienes
Copy link
Member Author

adienes commented May 18, 2025

the LegendrePolynomials.jl threaded tests fail more reliably on this PR, but they also seem to fail sometimes on master so it's unclear to me how much of that is this PR's fault, as I (think) my usage of the mutating GMP function is legal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants