perf: buffer SipHasher128

[tgnottingham]

This is an attempt to improve Siphasher128 performance by buffering input. Although it reduces instruction count, I'm not confident the effect on wall times, or lack-thereof, is worth the change.

---

Additional notes not reflected in source comments:

• Implementation choices were guided by a combination of results from rustc-perf and micro-benchmarks, mostly the former.
• I tried a couple of different struct layouts that might be more cache friendly with no obvious effect. Update: a particular struct layout was chosen, but it's not critical to performance. See comments in source and discussion below.
• I suspect that buffering would be important to a SIMD-accelerated algorithm, but from what I've read and my own tests, SipHash does not seem very amenable to SIMD acceleration, at least by SSE.

[rust-highfive]

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

[tgnottingham]

When someone has a chance, can I get a perf run on this? Thank you :)

[jyn514]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 634f9be5f25f97b58d444472dbd7c15fe940d9ce with merge cd008531939631a26b5c0b5b63f06ffb0810fe7b...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: cd008531939631a26b5c0b5b63f06ffb0810fe7b (cd008531939631a26b5c0b5b63f06ffb0810fe7b)

[rust-timer]

Queued cd008531939631a26b5c0b5b63f06ffb0810fe7b with parent 8c54cf6, future comparison URL.

[rust-timer]

Finished benchmarking try commit (cd008531939631a26b5c0b5b63f06ffb0810fe7b): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[jyn514]

Oh wow! those are some nice numbers :) Max of -6% instruction count, about -1% overall. Wall-times are all over the place as usual but seem to have mostly decreased other than an increase in hello-world.

[tgnottingham]

Okay, with Joshua's motivating comment :), I'm feeling a little better about the possibility of this being integrated. I'll remove the "[DO NOT MERGE]" and see where it goes.

@nnethercote and @michaelwoerister, you may have feedback on this, as I know it's an area you've touched.

[tesuji]

Some style comments, will add more later.

[elichai]

• I suspect that buffering would be important to a SIMD-accelerated algorithm, but from what I've read and my own tests, SipHash does not seem very amenable to SIMD acceleration, at least by SSE.

yeah siphash doesn't respond well to SSE, it's fast enough that the time it takes to load an SSE register makes it slower than non SSE, at least for siphash64, you can see here my attempts on it for a different project: bitcoin/bitcoin#17774

[varkor]

I don't think I'm the right person to review this. I'll reassign to r? @nnethercote for now, but find another reviewer if they don't have time to review.

[nnethercote]

This mostly looks good, though I have a few minor suggestions. Thanks for writing lots of good comments.

Prior to this PR, the SipHash in libstd was much the same as this one, but now they're not. Would it be worth changing the libstd one as well?

On the performance front, instruction counts have the least variance and wall time measures the right thing. Cycles can often be a good intermediate, being a more real-world measure while also being less variable than wall times. And the cycle results look even better than the instruction counts. So that's good.

[tgnottingham]

Thanks for the review, btw. :)

Prior to this PR, the SipHash in libstd was much the same as this one, but now they're not. Would it be worth changing the libstd one as well?

I guess the libs team would have to weigh in on that. The buffering improves the rustc workload, but there are workloads that it would cause regressions for. Creating lots of hashers that only hash a couple bytes is a bad workload for example.

[tgnottingham]

@nnethercote Friendly ping--anything else you'd like to see improved?

[nnethercote]

Sorry for the slow response.

@bors r+

[bors]

📌 Commit a602d15 has been approved by nnethercote

[bors]

⌛ Testing commit a602d15 with merge 5171cc7...

[bors]

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 5171cc7 to master...

[tgnottingham]

Sorry for the slow response.

Not a problem at all, thanks for reviewing! :)

[Mark-Simulacrum]

Performance results on the merge commit were consistent with the try run in this PR, but wall times look noisy and largely unchanged. Still, less instructions is presumably better for e.g. valgrind or similar tooling, so good work :)