Skip to content

Increase 64-bit rotation from 20 to 26 bits on finalizer #56

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

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

lsunsi
Copy link
Contributor

@lsunsi lsunsi commented Feb 5, 2025

The current finalizer consisting of a right-rotation of 20 bits can create fairly catastrophic results if the bottom input bits are low-entropy and the hash table grows beyond 2^20 elements, see rust-lang/rust#135477 (comment).

In this PR I bumped the number of bits to 26 from 20 in order to allow for bigger hash tables. This is a palliative solution because it increases the chance of collisions from what I can gather. The proper solution that would render the whole rotention irrelevant would be to fix the hashbrown implementation in a way I could not describe here but @orlp and @Noratrieb definitely could if you ask them haha.

Further, I changed the tests in an ad-hoc manner because I couldn't do it otherwise, so if it makes sense to do it in another way please let me know.

Also, I changed the version number to 2.1.1 because the only change was a private constant that should not implicate anything anywhere, but of course if 2.2.0 makes more sense I'll change it right away.

If any other change is required, please let me know!

@lsunsi lsunsi changed the title fix: increase 64bit rotation length Increase 64-bit rotation from 20 to 26 bits on finalizer Feb 5, 2025
@Noratrieb Noratrieb self-assigned this Feb 5, 2025
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

thanks! I'll merge and release this later today (if I forget please ping me again)

@Noratrieb Noratrieb merged commit 2588fc1 into rust-lang:master Feb 5, 2025
10 checks passed
@lsunsi lsunsi deleted the increase-64bit-rotation-length branch February 5, 2025 20:06
@niebayes
Copy link

@Noratrieb While rustc_hash is not marked as stable, introducing breaking changes in a minor version is quite disruptive. The recent rotation change has caused unintended issues with our database project. It would be preferable to reserve breaking changes for major version releases.

@Noratrieb
Copy link
Member

Noratrieb commented Feb 19, 2025

The primary intended purpose of this crate is hashmap hashing, where stability doesn't really matter. I don't think this crate should be used for anything else, it's a very bad quality hash. That could probably be better noted in the readme.

bgw added a commit to vercel/next.js that referenced this pull request Apr 30, 2025
This includes rust-lang/rustc-hash#56

Apparently 2.1.0 has a bug that can cause lots of hashtable collisions for very large maps with more than `2^20` elements. We have some very large maps, so this seems potentially concerning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants