Skip to content

Enable specialization with aHash#207

Merged
bors merged 16 commits into
rust-lang:masterfrom
tkaitchuck:specialize
Dec 10, 2020
Merged

Enable specialization with aHash#207
bors merged 16 commits into
rust-lang:masterfrom
tkaitchuck:specialize

Conversation

@tkaitchuck

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@Amanieu

Amanieu commented Oct 12, 2020

Copy link
Copy Markdown
Member

CI is failing.

@tkaitchuck

tkaitchuck commented Oct 12, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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".

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread src/map.rs
@Amanieu

Amanieu commented Oct 18, 2020

Copy link
Copy Markdown
Member

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

@tkaitchuck

tkaitchuck commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

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

Comment thread README.md Outdated
@tkaitchuck

Copy link
Copy Markdown
Contributor Author

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

@tkaitchuck tkaitchuck requested a review from Amanieu November 25, 2020 07:00
@tkaitchuck

Copy link
Copy Markdown
Contributor Author

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

@Amanieu

Amanieu commented Nov 30, 2020

Copy link
Copy Markdown
Member

LGTM once the CI issues are fixed.

@bors

bors commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

☔ 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.

3 participants