Skip to content
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

Enable specialization with aHash #207

Merged
merged 16 commits into from
Dec 10, 2020
Merged

Conversation

tkaitchuck
Copy link
Contributor

This uses the specialization feature in nightly to improve performance for hashing some classes (u8, u16, u32, u64, u128, [u8], and String) For these classes it improves performance by 15% the test insert_ahash benchmarks.

@tkaitchuck
Copy link
Contributor Author

This uses aHash 0.5.1 which no longer depends on const-random by default. See tkaitchuck/aHash@980159d

@Amanieu
Copy link
Member

Amanieu commented Oct 12, 2020

CI is failing.

@tkaitchuck
Copy link
Contributor Author

tkaitchuck commented Oct 12, 2020

This is running into an issue with std vs no-std.

In the current version per the discussion in: tkaitchuck/aHash#48 aHash does not supply a default if both std and const-random are disabled. However the current hashbrown code assumes the default is always available. We could have it use fixed keys as HashBrown does not guarantee DOS resistance, but the only obvious way to do this is for it to be aHash specific. Making it work with a provided hasher and aHash if using default is tricky.

@tkaitchuck
Copy link
Contributor Author

@Amanieu Does this approach make sense?
I have added a std flag to HashBrown which is off by default. So by default it uses aHash with constant keys, but if std or ahash-compile-time-rng it will use those for randomization. (With preference given to runtime randomization)

My concern is that it is something of a downgrade for no-std environments which by default previously were getting the week randomization based on memory address and now will be getting just fixed constants. Obviously a memory address is in a no-std environment might be far from random, but at least one couldn't do this: https://accidentallyquadratic.tumblr.com/post/153545455987/rust-hash-iteration-reinsertion

@tkaitchuck
Copy link
Contributor Author

Right now if both flags are off I have it doing:
pub type DefaultHashBuilder = core::hash::BuildHasherDefault<ahash::AHasher>;
I could change this behavior in aHash to not always produce an identical build hasher when using the default instantiation. That would resolve the issue I mentioned above. However I am reluctant to do this as it is dis-similar to the way the standard library works. Specifically DefaultHasher's new method says the produced value is "the same as all other DefaultHasher instances created through new or default".

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/map.rs Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Oct 18, 2020

I think the current behavior is fine. Using constant keys should also improve performance a bit.

@tkaitchuck
Copy link
Contributor Author

tkaitchuck commented Oct 20, 2020

This failed on thumbv6m-none-eabi apparently because atomics are not available on thumbv6m-none-eabi per rust-lang/compiler-builtins#114

README.md Outdated Show resolved Hide resolved
@tkaitchuck
Copy link
Contributor Author

@Amanieu Please take another look. I believe this approach is a lot better.

@tkaitchuck
Copy link
Contributor Author

Remaining build issue is a pre-existing problem: #214

@Amanieu
Copy link
Member

Amanieu commented Nov 30, 2020

LGTM once the CI issues are fixed.

@bors
Copy link
Collaborator

bors commented Dec 8, 2020

☔ The latest upstream changes (presumably #215) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

This was referenced Mar 15, 2021
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.

None yet

3 participants