Block RNGs: remove unaligned memory cast

[dhardy]

Fix #779. Review please @RalfJung.

The point of the removed specialisations was to avoid one copy. Since the output type may not have the correct alignment and we wish to copy bytes in the same order, we have no choice but to use a buffer anyway. (We could complicate things by checking the alignment at run-time, but I don't think it's worthwhile given the very small performance cost of the extra copy.)

Benchmarking with 10*1024 byte buffer does show a small impact:

# before
test gen_bytes_chacha20             ... bench:  17,775,692 ns/iter (+/- 290,563) = 576 MB/s
test gen_bytes_hc128                ... bench:   4,243,890 ns/iter (+/- 69,032) = 2412 MB/s
test gen_bytes_isaac                ... bench:   7,238,757 ns/iter (+/- 123,797) = 1414 MB/s
test gen_bytes_isaac64              ... bench:   3,900,653 ns/iter (+/- 47,162) = 2625 MB/s
# after
test gen_bytes_chacha20             ... bench:  18,007,036 ns/iter (+/- 384,358) = 568 MB/s
test gen_bytes_hc128                ... bench:   4,634,505 ns/iter (+/- 55,119) = 2209 MB/s
test gen_bytes_isaac                ... bench:   7,338,592 ns/iter (+/- 90,396) = 1395 MB/s
test gen_bytes_isaac64              ... bench:   4,066,367 ns/iter (+/- 55,543) = 2518 MB/s

The second commit cleans up some warnings in the benches.

[burdges]

It's not worth it to use read_unaligned like you mentioned previously?

[dhardy]

This isn't about unaligned reads, it's about unaligned writes masquerading as aligned ones (because of cast to &[u32] or &[u64]). I guess we could change BlockRngCore::generate to take *mut u32 arg or some such, but I really don't think these small performance losses are worth pushing unsafe code into all block-rng implementations.

[RalfJung]

Is there a way that align_to could be useful here to find the "aligned" part of the buffer? That would work on all platforms then, not just x86.

The change itself seems fine; this is what Miri is testing in #781.

[dhardy]

Finding the aligned subset of dest doesn't help unless we allow bytes of the RNG to be skipped. We don't wish to do this arbitrarily (or based on something as tenuous as input alignment) because in some cases reproducibility is important, and this would be hard to document and a breaking change from past behaviour.

I'll leave this until tomorrow for any further review, then we can merge the PRs.

[vks]

Finding the aligned subset of dest doesn't help unless we allow bytes of the RNG to be skipped.

Maybe we should add an API that requires aligned slices?

[dhardy]

As in fill_aligned_bytes? Possible I guess.

What I don't understand is why everyone keeps suggesting complicated ways to keep this (very) small highly-specific optimisation.

[burdges]

It's not worth complicating the trait over this. lol

[LukasKalbertodt]

Would it be possible to backport this bugfix to rand_core 0.4? Currently it's not possible to run quickcheck tests through Miri because this bug always makes the tests fail. And I think quite a few crates have not switched to the new rand version yet.

But I can also understand if this is too much work. I just wanted to ask ^_^

[dhardy]

Sure, I guess it's possible.