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

Improve SmallRng initialization performance #1482

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

arthurprs
Copy link
Contributor

  • Added a CHANGELOG.md entry

Summary

Improve initialization speed for SmallRng

Motivation

SmallRng is plenty fast, but the initialization speed was suboptimal.

Details

An example binary was added to check for code generation with cargo asm.

Benchmarks were also added. The results are shown below:

before
test init_from_seed_smallrng  ... bench:           0.84 ns/iter (+/- 0.06)
test init_from_u64_smallrng  ... bench:           6.19 ns/iter (+/- 0.58)
test init_smallrng           ... bench:           9.45 ns/iter (+/- 0.18)

after
test init_from_seed_smallrng  ... bench:           0.85 ns/iter (+/- 0.08)
test init_from_u64_smallrng  ... bench:           3.42 ns/iter (+/- 0.09)
test init_smallrng           ... bench:           6.65 ns/iter (+/- 0.15)

@@ -0,0 +1,45 @@
extern crate rand;
use std::hint::black_box;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the min MSRV is problematic here, maybe this will have to be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Either that, or...

This example serves as a non-automated test to check code generation? Can we turn it into an automated test? Otherwise I suggest it just be removed.

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.

So this is just an optimisation which does not change any results?

@@ -0,0 +1,45 @@
extern crate rand;
use std::hint::black_box;
Copy link
Member

Choose a reason for hiding this comment

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

Either that, or...

This example serves as a non-automated test to check code generation? Can we turn it into an automated test? Otherwise I suggest it just be removed.

src/rngs/small.rs Show resolved Hide resolved
@dhardy dhardy added the D-changes Do: changes requested label Aug 12, 2024
@dhardy
Copy link
Member

dhardy commented Sep 10, 2024

Reminder: changes are needed to ensure the API remains consistent on 32-bit and 64-bit platforms.

Also, these new benchmarks can't be merged since #1490 moved all benchmarks to use Criterion.

@arthurprs arthurprs force-pushed the faster-small-rng-creation branch 2 times, most recently from 6224cfb to ae6ef7a Compare September 11, 2024 20:42
@arthurprs
Copy link
Contributor Author

Rebased on main

@arthurprs arthurprs force-pushed the faster-small-rng-creation branch from ae6ef7a to 5410c5d Compare September 11, 2024 21:18
@arthurprs
Copy link
Contributor Author

CI seems unhappy about a file I didn't change, not sure what's up.

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.

One minor 'bug'; otherwise looks good.

Comment on lines 59 to 60
i[0] = z.to_le() as u32;
i[1] = (z.to_le() >> 32) as u32;
Copy link
Member

Choose a reason for hiding this comment

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

We aren't round-tripping to LE bytes and back since your code now directly initializes state, hence we don't want .to_le() here.

This should trip a test, but it seems we never actually test seed_from_u64... because we don't guarantee value-stability for SmallRng anyway. (In other words, this isn't strictly a bug, but you should still remove .to_le(), and preferably also add some form of reference test for this method.)

Copy link
Contributor Author

@arthurprs arthurprs Oct 1, 2024

Choose a reason for hiding this comment

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

If I remove it, won't the result be different than before on BE? I was trying to preserve that here. But yeah, kinda pointless w/o a test to ensure that it's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that this PR should add test for the expected state of the Rng after seed_from_u64?

Copy link
Member

Choose a reason for hiding this comment

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

There's a reference test in the same file, but in this case testing a single sample should be enough. And yes, results should match from prior to your PR.

It doesn't really matter since we explicitly do not promise value stability of these RNGs, but feel like we should anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in bee90d1 and 4d6b651

src/rngs/xoshiro256plusplus.rs Outdated Show resolved Hide resolved
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.

Thanks!

@dhardy dhardy merged commit d1f961c into rust-random:master Oct 7, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-changes Do: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants