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

Add xoroshiro128+ generator. #102

Closed
wants to merge 1 commit into from

Conversation

DanielKeep
Copy link

This is based on the reference code, sourced from the author's website.

The authors position xoroshiro128+ as the successor to xorshift128+,
with better statistical properties and higher performance (see their website).

This implementation does not contain the jump function.

Summary of benchmark numbers for a x86_64-pc-windows-msvc nightly 2016-02-12:

test rand_f32                                      ... bench:       4,942 ns/iter (+/- 245) = 809 MB/s
test rand_f64                                      ... bench:       5,011 ns/iter (+/- 154) = 1596 MB/s
test rand_isaac                                    ... bench:      12,894 ns/iter (+/- 307) = 620 MB/s
test rand_isaac64                                  ... bench:       4,991 ns/iter (+/- 166) = 1602 MB/s
test rand_shuffle_100                              ... bench:       1,903 ns/iter (+/- 208)
test rand_std                                      ... bench:       5,027 ns/iter (+/- 167) = 1591 MB/s
test rand_xoroshiro                                ... bench:       1,717 ns/iter (+/- 67) = 4659 MB/s
test rand_xorshift                                 ... bench:       2,716 ns/iter (+/- 103) = 2945 MB/s

The same for i686-pc-windows-gnu nightly 2016-04-20:

test rand_f32                                      ... bench:       6,378 ns/iter (+/- 288) = 627 MB/s
test rand_f64                                      ... bench:      11,888 ns/iter (+/- 415) = 672 MB/s
test rand_isaac                                    ... bench:       5,741 ns/iter (+/- 279) = 696 MB/s
test rand_isaac64                                  ... bench:       7,243 ns/iter (+/- 266) = 552 MB/s
test rand_shuffle_100                              ... bench:       1,101 ns/iter (+/- 47)
test rand_std                                      ... bench:       5,785 ns/iter (+/- 263) = 691 MB/s
test rand_xoroshiro                                ... bench:       3,745 ns/iter (+/- 196) = 1068 MB/s
test rand_xorshift                                 ... bench:       1,818 ns/iter (+/- 82) = 2200 MB/s

Yes, it is slower than xorshift on 32-bit, though it's still faster than everything else.

This is based on the reference code given in [1], sourced from [2].

The authors position xoroshiro128+ as the successor to xorshift128+,
with better statistical properties *and* higher performance [2].

This implementation does *not* contain the `jump` function.

[1]: http://xorshift.di.unimi.it/xoroshiro128plus.c
[2]: http://xorshift.di.unimi.it/
@nagisa
Copy link
Contributor

nagisa commented Apr 29, 2016

I feel like all sorts of the odd generators (i.e. everything other than what we already have to support) belong in external crates.

@DanielKeep
Copy link
Author

@nagisa On the one hand, not having to compile every possible RNG just because you want to use one of them is good. On the other, it's called "rand", so it supporting a variety of generators doesn't seem unreasonable.

Mostly, I made this PR because someone else had exactly the same idea as I did, at more or less exactly the same moment, so I took that as a sign. If this gets turned down, I'll just spin it into an external crate; no biggie.

@bluss
Copy link
Contributor

bluss commented May 13, 2016

The next major version of rand could incorporate this as a replacement for Xorshift and as the implementation for weak_rng. The weak rng could also be "anonymized" in the API and maybe not even expose it as Xorshift or Xoroshiro or anything in particular.

#[inline]
fn next_u32(&mut self) -> u32 {
self.next_u64() as u32
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to reuse the second half of the 64-bit integer if a second 32-bit integer is requested? Or is this slower due to the additional branch?

@alexcrichton
Copy link
Contributor

Thanks for the PR @DanielKeep! The libs team discussed this PR during triage yesterday, and the conclusion was that for now we don't want to include any other RNG implementations in the rand crate directly. It's possible to ergonomically build this out of tree (due to the traits in rand) so there's not necessarily an inherent reason to include this in the rand crate itself.

Unfortunately this crate is lacking a vision of how to proceed forward which makes it difficult to say whether we want this sort of RNG eventually. We hope to start devoting some time to this crate soon, but help is always appreciated!

In the meantime I'm gonna close this, but please feel free to publish this on crates.io!

@bluss
Copy link
Contributor

bluss commented May 24, 2016

Since the author considers it to be a replacement for xorshift, I think it's logical to consider it as its replacement in rand.

@alexcrichton
Copy link
Contributor

Sure, but it's not even clear whether xorshift should itself be in rand, unfortunately.

@bluss
Copy link
Contributor

bluss commented May 25, 2016

I see. Is it too early to stagnate this crate maybe? Is it important we shoot for an 1.0 version soon? Maybe we should instead develop it more, the distributions are pretty far away from being 1.0-ready IMO.

@alexcrichton
Copy link
Contributor

To be clear, we certainly don't want this crate to stagnate! We'd love to move this crate to 1.0, but it would require someone to take up the banner of doing that, and we don't currently have anyone to do that. Until that happens additions like new RNG implementations is both controversial and also easy to do out of tree, so the decision is that in the meantime we won't change it until there's a vision moving forward.

@aturon
Copy link
Contributor

aturon commented May 25, 2016

... If someone on this thread would like to take up the banner for rand (cough @bluss? cough), please let me know :-)

@bluss
Copy link
Contributor

bluss commented May 26, 2016

Yeah it makes sense. I can be maintainer, so if we have someone be that, then we can entertain incremental and evolutionary changes to the library again?

@alexcrichton
Copy link
Contributor

@bluss certainly! We're just hesitant to make changes without an overall vision, but if we've got someone with that vision then there's something concrete to talk about and discuss. Both @huonw and @jonathandturner have taken a look at upping our rand support (and they've both got some material I believe), but if you'd like to take over I'm sure they'd be fine with that!

@vks
Copy link
Collaborator

vks commented Jun 23, 2016

@DanielKeep I published this on crates.io. Do you want me to add you to the authors or transfer ownership of the repository/crate to you?

@DanielKeep
Copy link
Author

@vks Sorry for late reply: I don't think the code is complicated enough to worry about authorship or ownership. I just translated the reference code. To everyone else: sorry for the thread bump.

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