Hash up to 8 bytes at once with FxHasher #1
Conversation
| bytes = &bytes[2..]; | ||
| } | ||
| if (size_of::<usize>() > 1) && bytes.len() >= 1 { | ||
| hash.add_to_hash(bytes[0] as usize); |
There was a problem hiding this comment.
Doesn't all of this mean that splitting a write call changes the hash? IIRC it shouldn't.
Could an union { bytes: [u8; size_of::<usize>()], usize: usize } buffer be used instead?
There was a problem hiding this comment.
That doesn't seem to be a documented nor a useful property.
There was a problem hiding this comment.
cc @michaelwoerister @gankro I remember discussions about this property
Note that it's potentially useful to buffer the values if, with e.g. nested enums, you're writing byte-sized values (i.e. discriminants) most of the time, one at a time.
There was a problem hiding this comment.
Since the FxHasher is only used with hash tables, I don't think that the hash must be stable. As long as it is deterministic for our use cases, it's fine, I think. It already treats (u8, u8) different from u16 where a similar argument could be made.
My view is: FxHasher should be the absolute fastest for small keys and it should do whatever it can get away with in practice.
There was a problem hiding this comment.
I still think we should try and bench this against some buffering scheme, especially if it can all be inlined down to a few applications of the usize "block" function.
EDIT: nevermind, all the leaves I was thinking off go through the write_uN methods below, so those would also need to be buffered somehow to observe a benefit.
There was a problem hiding this comment.
Yeah, we don't need to do this in this PR. The benchmarks showed that it's an improvement.
As a sidenote, using perf.rlo is a lot more complicated when testing out-of-tree crates...
|
The only problem I see here does not have to do with the PR directly: Since this is a standalone crate now, it should have tests and integrate with travis. Seeing that all tests pass makes approving a PR much simpler. |
Cargo.lock
Outdated
| [[package]] | ||
| name = "rustc-hash" | ||
| version = "0.1.0" |
There was a problem hiding this comment.
Why does a library crate have a Cargo.lock?
|
I think the version number needs to be bumped so we can publish on crates.io. |
|
I'll merge this because it was already tested as part of |
See rust-lang/rust#51019