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

Switch StdRng and thread_rng to HC-128 #277

Merged
merged 3 commits into from
Mar 3, 2018

Conversation

pitdicker
Copy link
Contributor

As discussed in dhardy#53.

I also changed THREAD_RNG_RESEED_THRESHOLD to 32 MiB, which seems a more reasonable number to me.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

A couple of little tweaks needed, but I approve, despite the apparent performance issue on ARMv7. Feedback in the linked thread was positive

src/lib.rs Outdated
#[derive(Clone, Debug)]
pub struct StdRng(IsaacWordRng);
pub struct StdRng(Hc128Rng);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this leave IsaacWordRng unused? We might as well remove it. Ah, you didn't update the Seed type...

/// Like [`StdRng`], `ThreadRng` is a cryptographically secure PRNG. The current
/// algorithm used is [HC-128], which is an array-based PRNG that trades memory
/// usage for better performance. This makes it similar to ISAAC, the algorithm
/// used in `ThreadRng` before rand 0.5.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this means it can implement CryptoRng now.

@pitdicker
Copy link
Contributor Author

I was too busy with the docs that I forget to check the rebase 😕. I will update the tests in a moment, but am getting some strange failures...

A couple of little tweaks needed, but I approve, despite the apparent performance issue on ARMv7.

I would like to try a couple of benchmarks on ARM, but am not sure how to set it up.

@pitdicker
Copy link
Contributor Author

Wow, testing on my phone is super easy with cargo dinghy test!
rand still passes all tests on ARM 😄. But cargo dinghy bench only runs the benchmarks as test, without performance numbers. So that will need some more experimenting.

@dhardy
Copy link
Member

dhardy commented Mar 2, 2018

Use of nautical terms reminds me that my phone is a Jolla, with native shell and GCC support... Rustup is also happy to install. Slow but managed to get some results at least:

test gen_bytes_chacha12    ... bench:  10,633,598 ns/iter (+/- 61,043) = 96 MB/s
test gen_bytes_chacha20    ... bench:  16,384,802 ns/iter (+/- 48,325) = 62 MB/s
test gen_bytes_chacha8     ... bench:   7,751,370 ns/iter (+/- 61,750) = 132 MB/s
test gen_bytes_hc128       ... bench:   4,181,004 ns/iter (+/- 21,645) = 244 MB/s
test gen_bytes_isaac       ... bench:   2,996,947 ns/iter (+/- 18,311) = 341 MB/s
test gen_bytes_isaac64     ... bench:   2,478,126 ns/iter (+/- 18,311) = 413 MB/s
test gen_bytes_xorshift    ... bench:   2,661,644 ns/iter (+/- 297,298) = 384 MB/s

Funny when Isaac64 beats Xorshift.

@pitdicker
Copy link
Contributor Author

Funny when Isaac64 beats Xorshift.

Sounds like there is some work left to do on ARM...

Travis seems to have given up, but because this passes on all other platforms I can't imagine it to fail. Ready to merge?

@dhardy
Copy link
Member

dhardy commented Mar 3, 2018

I need to investigate those benchmarks further; some of them seemed to hang.

Oh you still didn't implement CryptoRng? That was added as part of #273 I think.

@dhardy
Copy link
Member

dhardy commented Mar 3, 2018

Oh, Xorshift wins on u32 and u64. Tests sometimes hang but when they do give a result, the results do appear to be repeatable. Isaac still beats HC128 everywhere in performance, but not by too much.

test gen_bytes_chacha12    ... bench:  10,666,902 ns/iter (+/- 71,778,443) = 95 MB/s
test gen_bytes_chacha20    ... bench:  16,412,455 ns/iter (+/- 38,287,657) = 62 MB/s
test gen_bytes_chacha8     ... bench:   7,772,730 ns/iter (+/- 48,848) = 131 MB/s
test gen_bytes_hc128       ... bench:   4,182,483 ns/iter (+/- 13,923) = 244 MB/s
test gen_bytes_isaac       ... bench:   3,004,071 ns/iter (+/- 54,952) = 340 MB/s
test gen_bytes_isaac64     ... bench:   2,460,653 ns/iter (+/- 55,563) = 416 MB/s
test gen_bytes_xorshift    ... bench:   2,652,987 ns/iter (+/- 68,843) = 385 MB/s

test gen_u32_chacha12      ... bench:      47,129 ns/iter (+/- 381) = 84 MB/s
test gen_u32_chacha20      ... bench:      69,453 ns/iter (+/- 763) = 57 MB/s
test gen_u32_chacha8       ... bench:      35,758 ns/iter (+/- 172,155) = 111 MB/s
test gen_u32_hc128         ... bench:      20,893 ns/iter (+/- 60) = 191 MB/s
test gen_u32_isaac         ... bench:      18,317 ns/iter (+/- 1,097) = 218 MB/s
test gen_u32_isaac64       ... bench:      16,791 ns/iter (+/- 763) = 238 MB/s
test gen_u32_xorshift      ... bench:       9,254 ns/iter (+/- 95) = 432 MB/s

test gen_u64_chacha12      ... bench:      89,727 ns/iter (+/- 1,253) = 89 MB/s
test gen_u64_chacha20      ... bench:     135,030 ns/iter (+/- 1,961) = 59 MB/s
test gen_u64_chacha8       ... bench:      67,148 ns/iter (+/- 1,145) = 119 MB/s
test gen_u64_hc128         ... bench:      37,398 ns/iter (+/- 381) = 213 MB/s
test gen_u64_isaac         ... bench:      28,811 ns/iter (+/- 763) = 277 MB/s
test gen_u64_isaac64       ... bench:      23,850 ns/iter (+/- 1,526) = 335 MB/s
test gen_u64_xorshift      ... bench:      10,685 ns/iter (+/- 190) = 748 MB/s

@dhardy dhardy merged commit fdb6c38 into rust-random:master Mar 3, 2018
@pitdicker pitdicker deleted the switch_stdrng branch March 3, 2018 12:12
@pitdicker
Copy link
Contributor Author

Those benchmarks are very strange, gen_u64_xorshift should have roughly the same Mb/s as gen_u32_xorshift. But thanks for testing, and for implementing CryptoRng.

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.

2 participants