-
Notifications
You must be signed in to change notification settings - Fork 211
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
plain_hasher/uint: remove some unsafe code and don't assume endianness #407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
What I found surprising is that the performance of uint::from_little_endian
and uint::from_big_endian
has improved according to our benches! And plain_hasher::write
has not regressed. Amazing!
Will need to look at the generated code to confirm.
I'll submit a PR to fix the benches from optimizing away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -30,17 +30,18 @@ impl Hasher for PlainHasher { | |||
fn write(&mut self, bytes: &[u8]) { | |||
debug_assert!(bytes.len() == 32); | |||
let mut bytes_ptr = bytes.as_ptr(); | |||
let mut prefix_ptr = &mut self.prefix as *mut u64 as *mut u8; | |||
let mut prefix_bytes = self.prefix.to_le_bytes(); | |||
|
|||
unroll! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the unroll!
is still needed here – did you try without it? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, it's stll needed :(
write_plain_hasher time: [1.9603 us 1.9624 us 1.9647 us]
change: [+381.78% +382.63% +383.51%] (p = 0.00 < 0.05)
Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
1 (1.00%) low severe
7 (7.00%) high mild
4 (4.00%) high severe
Tests for
plain_hasher
anduint
both fail on big endian architectures because they assume a certain byte layout. With these patches tests for both pass on big and small endian systems.