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

optimzed SipHash implementation #13522

Merged
merged 1 commit into from
Apr 16, 2014
Merged

optimzed SipHash implementation #13522

merged 1 commit into from
Apr 16, 2014

Conversation

seanmonstar
Copy link
Contributor

work started from @gereeter's PR: #13114
but adjusted bits

before
test hash::sip::tests::bench_u64                            ... bench:        34 ns/iter (+/- 0)
test hash::sip::tests::bench_str_under_8_bytes              ... bench:        37 ns/iter (+/- 1)
test hash::sip::tests::bench_str_of_8_bytes                 ... bench:        43 ns/iter (+/- 1)
test hash::sip::tests::bench_str_over_8_bytes               ... bench:        50 ns/iter (+/- 1)
test hash::sip::tests::bench_long_str                       ... bench:       613 ns/iter (+/- 14)
test hash::sip::tests::bench_compound_1                     ... bench:       114 ns/iter (+/- 11)

after
test hash::sip::tests::bench_u64                            ... bench:        25 ns/iter (+/- 0)
test hash::sip::tests::bench_str_under_8_bytes              ... bench:        31 ns/iter (+/- 0)
test hash::sip::tests::bench_str_of_8_bytes                 ... bench:        36 ns/iter (+/- 0)
test hash::sip::tests::bench_str_over_8_bytes               ... bench:        40 ns/iter (+/- 0)
test hash::sip::tests::bench_long_str                       ... bench:       600 ns/iter (+/- 14)
test hash::sip::tests::bench_compound_1                     ... bench:        64 ns/iter (+/- 6)

Notably it seems smaller keys will hash faster. A long string doesn't see much gains, but compound cuts in half (once compound used a int and u64).

@brson
Copy link
Contributor

brson commented Apr 15, 2014

Nice wins.

@thestinger
Copy link
Contributor

It's really awesome to have this running faster on small keys, because it will discourage working around the issue by using hashes vulnerable to DoS attacks. I wonder how much more there is to gain here.

let mut out = 0u64;
while t < $len {
out |= $buf[t+$i] as u64 << t*8;
t += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a for loop? (And why not use the vector iterator + .enumerate() to avoid the bounds checks?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of usingusing a range iter, but a cursory glance made me think it brought more overhead and branches. I'll try a bench using for, and if it's faster, I can switch it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking something to avoid the indexing with the associated bounds checks and unwinding, something like:

for (t, x) in $buf.slice($i, $i + $len).enumerate() {
    out |= *x << t * 8
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to that added 2ns/iter to each bench.

@seanmonstar
Copy link
Contributor Author

@thestinger I wonder as well. I'm putting together a bench of a straight
copy of siphash into rust, without the Hasher methods, to see a theoretical
best.

I'm wondering if we could sacrifice some memory for speed by collecting all
the bytes into a vector before hashing (perhaps to an upper limit), instead
of hashing parts on each write.

@thestinger
Copy link
Contributor

It's possible to do a lot better than the plain single-pass C implementation by using SIMD.

@alexcrichton
Copy link
Member

This appeared to cause a test failure on windows: http://buildbot.rust-lang.org/builders/auto-win-32-nopt-t/builds/4459/steps/test/logs/stdio.

Perhaps there's a bug on 32-bit?

work started from @gereeter's PR: rust-lang#13114
but adjusted bits
@seanmonstar
Copy link
Contributor Author

@alexcrichton yep, hash of int differed base on arch. I've adjusted it.

@alexcrichton
Copy link
Member

I was worried about #8361 cropping up again, but it appears that's what happened before anyway, so I'm not too worried.

bors added a commit that referenced this pull request Apr 16, 2014
work started from @gereeter's PR: #13114
but adjusted bits

```
before
test hash::sip::tests::bench_u64                            ... bench:        34 ns/iter (+/- 0)
test hash::sip::tests::bench_str_under_8_bytes              ... bench:        37 ns/iter (+/- 1)
test hash::sip::tests::bench_str_of_8_bytes                 ... bench:        43 ns/iter (+/- 1)
test hash::sip::tests::bench_str_over_8_bytes               ... bench:        50 ns/iter (+/- 1)
test hash::sip::tests::bench_long_str                       ... bench:       613 ns/iter (+/- 14)
test hash::sip::tests::bench_compound_1                     ... bench:       114 ns/iter (+/- 11)

after
test hash::sip::tests::bench_u64                            ... bench:        25 ns/iter (+/- 0)
test hash::sip::tests::bench_str_under_8_bytes              ... bench:        31 ns/iter (+/- 0)
test hash::sip::tests::bench_str_of_8_bytes                 ... bench:        36 ns/iter (+/- 0)
test hash::sip::tests::bench_str_over_8_bytes               ... bench:        40 ns/iter (+/- 0)
test hash::sip::tests::bench_long_str                       ... bench:       600 ns/iter (+/- 14)
test hash::sip::tests::bench_compound_1                     ... bench:        64 ns/iter (+/- 6)
```

Notably it seems smaller keys will hash faster. A long string doesn't see much gains, but compound cuts in half (once compound used a `int` and `u64`).
@bors bors closed this Apr 16, 2014
@bors bors merged commit 9c1cd69 into rust-lang:master Apr 16, 2014
notriddle pushed a commit to notriddle/rust that referenced this pull request Nov 10, 2022
…=lnicola

fix: disregard type variable expectation for if expressions

Fixes rust-lang#13522

As [the comment](https://github.com/rust-lang/rust-analyzer/blob/8142d1f606dc2e52b1d2b8992671e2bd73379f28/crates/hir-ty/src/infer.rs#L1087-L1090) on `Expectation::adjust_for_branches` explains:

> If the expected type is just a type variable, then don't use an expected type. Otherwise, we might write parts of the type when checking the 'then' block which are incompatible with the 'else' branch.

Note that we already use it in match expressions. I've added tests for them too nevertheless.
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.

6 participants