Contributor

tkaitchuck commented on Oct 12

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

Amanieu commented on Oct 12

CI is failing.

tkaitchuck commented on Oct 13 •

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 commented on Oct 13

@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 commented on Oct 14

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

README.md

Outdated

Show resolved

src/map.rs

Show resolved

Amanieu commented on Oct 18

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

tkaitchuck commented on Oct 21 •

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

tkaitchuck commented 25 days ago

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

tkaitchuck commented 25 days ago

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

Amanieu commented 20 days ago

LGTM once the CI issues are fixed.

bors commented 11 days ago

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

Amanieu commented 11 days ago

Needs a rebase.

tkaitchuck commented 10 days ago

Amanieu commented 10 days ago

@bors r+

bors commented 10 days ago

Commit f1d3137 has been approved by Amanieu

bors commented 10 days ago

Testing commit f1d3137 with merge 132995a...

bors commented 10 days ago

Test successful - checks-travis
Approved by: Amanieu
Pushing 132995a to master...