-
Notifications
You must be signed in to change notification settings - Fork 432
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
Remove weak_rng
and random
?
#289
Comments
What is the difference between |
I completely agree to remove I would not add any more wrappers than Than we have a clear story about when to use which:
When we start to add any more wrappers to differentiate between RNGs, where do we stop? Should we have wrappers to differentiate between the memory-efficient, SIMD-capable ChaCha, and the fast, array based HC-128 (both stream cipher based), and the block cipher based AES-CTR DRBG that may have native hardware support? I think we should have just one wrapper for CSPRNGs:
|
I agree that it is nice to only have two options, possibly suggesting other, more specialized crates in the documentation. However, I'm not a fan of the name
Why not? When would |
Yet to be defined and not really on topic here (it's partly a compromise since you two can't seem to agree on which PRNG to make default). But I agree with @pitdicker that it doesn't make much sense having two. According to the benchmarks HC-128 is
|
You can also see the benchmarks I did on my ARM v7 phone where Xorshift is definitely faster, if you want more context. Any way you look at it though, the biggest reason not to use a cryptographic generator would seem to be the size of the PRNG; this is more significant if multiple PRNGs are used. |
I think it is going to be difficult for you @vks and me to find agreement, because we look at it in a different way. I think we can basically agree that all the known RNG algorithms are looking for the best balance between generating numbers as fast as possible, with simple code, and having the numbers look as 'random' as possible. The quest for performance is a very practical concern, and that which mostly interest @vks. The quest for quality is what has driven many of the 'big names' and developments behind RNGs, like George Marsaglia and Pierre L’Ecuyer, and is what I prefer. As I have said again, I have nothing (well, almost) against Xoroshiro128+. It is great for many situations, like generating many floating-point values. Both Xoroshiro128+ and PCG definitely have a place. PerformanceOn one hand, I care quite a bit about performance (@dhardy can confirm the effort I have spent optimizing stuff in Raw performance generating
Adding a tiny bit of use: converting to an
Generating an
Generating numbers in a distribution (
Generating integers in a range:
So converting the output of the RNG into a number you can actually use in your code often already halves the performance, and then you are not even using it meaningfully yet! Still, in these benchmarks Xoroshiro128+ is often 20% faster than PCG-XSL-128, and that is not a number to ignore. But that number only keeps getting lower when you consider the real-world surrounding code. QualityI think a must read when talking about RNG quality is the paper '"Good random number generators are (not so) easy to find"](http://random.mat.sbg.ac.at/results/peter/A19final.pdf) by P. Hellekalek. The best way to make sure an RNG is suitable for many kinds of different uses is by measuring how well it performs on the RNG test suites designed for this (TestU01 and PractRand). And to be honest I don't know how all the tests they perform map to which real-world situations. To be honest again there is one thing that I have against Xoroshiro, and that is its design history. Sebastiano Vigna did valuable work by improving existing Xorshift variants. His method was to run the TestU01 benchmarks countless times, to figure out the best constants to use in Xorshift* and Xorshift+. But this is basically optimising an RNG to pass a specific test suite. When another one came along, PractRand, it failed instantly. And also when others began testing and evaluating Xorshift+ and Xoroshiro+ it turned out to pass tests a less often. This indicates there are situations for which Xoroshiro128+ is not the best choice. PCG was also designed in combination with the TestU01 test suite, but with a much stronger theoretical basis. It really strives to remove any kinds of patterns. And, as in proper testing, kept weakening the RNG to figure out how much headroom there is before the tests actually fail. This explains why when the other test suite came along (PractRand) there where no problems. One argument to prefer better quality over performance is one I have seen in several other discussion in the Rust forums. One aspect is very easy to test and measure, the other very difficult. It is very easy to measure whether the PRNG you use is a performance bottleneck. It is very hard to test and measure whether the random numbers that get generated are as 'random' as you expect them to be. That is why I would like to see both, but make the 'safest choice' the default. Edit: one more think I would like to mention when it comes to quality is that I have spent effort to make the float conversion code, generating |
I think you're getting off the original topic there @pitdicker. Yes, I can confirm you have done a lot of optimisation work! I didn't want to discuss which algorithm we pick here because we already have at least two threads about that — perhaps you'd like to copy your post elsewhere? @vks are you happy to use the name |
I agree it is better to have just one. I don't feel strongly about the default, PCG is close enough to my personal optimum, and it is very easy for me to use another RNG.
Yes, I'm convinced now. It is a good name to distinguish such generators from faster ones with larger state (e.g. xorshift1024 is faster than xorshift128). |
I added a tracker to the top. Anyone want to implement the first two items? |
I'm interested. I'll try to get a PR done until Monday. |
Should I remove the functions or just deprecate them? Another problem is that |
Just deprecate if possible; you can see several examples of deprecation. Good point; I advised using |
`SmallRng` is an opaque wrapper type similar to `StdRng`. See rust-random#289.
I replaced every |
Implemented by #296. |
`SmallRng` is an opaque wrapper type similar to `StdRng`. See #289.
Hello there! I landed here after noticing that If there is no more discussion about
I'd be genuinely sad if |
I think the main arguments for removing it were that Any further thoughts on this? It's not impossible to reverse the decision now — in fact if we do, much better to do so before the 0.5 release. |
@LukasKalbertodt Good that you tracked it down to this issue! We mostly discussed Personally I would much rather see users of Rand use Anyway I don't care strongly t.b.h. For a little nice history: |
Thanks for reopening this issue! :)
I have to admit that I don't know a huge lot about And in the "slow" situation: nothing really bad happens here. If the speed is actually important, the programmer will notice and will presumably read the documentation of
I don't really think we gain a lot from writing
But there are plenty of other functions in And by the way: I wouldn't defend a if random() {
println!("Heads!")
} ... and it just works? |
That's a good argument, and I don't feel strongly about this — also, I don't believe we ever had a strong reason for removing it (as @pitdicker says, it didn't receive much love from us). Does someone want to open a PR? It's probably as simple as removing the deprecation and updating documentation, but also see: |
Another problem about |
@vks That can be kind of fixed transparently with a |
This needs to stay, the rand lib is already confusing enough to use 😄 |
Fwiw, as a drive by reader of this issue |
The idea is that let mut rng = thread_rng();
for _ in 0..10 {
let value = rng.gen();
/* do something... */
} This way the RNG does not have to be looked up from thread local storage on every iteration. Maybe one argument against The
I expect A stand-alone
I hope not too much 😞. |
@SirVer as a 'drive by reader' |
The main thing I'm looking for when I'm in need of randomness is:
This should be as straight forward as possible. I would argue you create random numbers in two ways:
I'm completely lost when it comes to the deprecated Sorry that was sort of off-topic, perhaps following @banister some bikeshedding: perhaps it's better to use the word |
I think it's nice to have this convenience function:
So I'd vote in favor of keeping it :) |
@pitdicker yeah :( unfortunately the documentation for Which is incidentally exactly why the |
Yes, I'm perfectly fine with
The problem is that most of the documentation is written by people who already have some understanding of random number generators and Rust, without much feedback from those with less background. If you have some feedback on how to make the documentation clearer / more to-the-point and less confusing, then please open an issue and let us know.
This is a different topic, but hopefully this will get you started. We seem to have lost the example implementing this for your own type unfortunately. The reason this isn't such a simple concept is — because it's not. A little background in probability theory will tell you that "in order to create random instances of my type" is a woefully inadequate specification — e.g. if you were to pick a "random year" you would have to at least pick upper and lower bounds, and might consider some years more "interesting" than others 😆 |
I'm taking another try at the documentation. @dhardy Sorry, as you already put a lot of effort into it. After every previous improvement there always seems room for some more... |
I posted a link to this thread on Reddit to get a few more people involved... I didn't quite expect this number of people ;-) EDIT: This benchmark is probably pretty flawed and its results pretty useless. You should probably just skip the rest of my comment ;-) To get some hard data, I wrote a small benchmark.
The results (generating
Honestly, I'm quite surprised how freaking fast the PRNG is. So fast in fact that the little extra work done by While my benchmark couldn't show what I initially wanted to show with it, I still stick to what I said before. Sure, And yes, the documentation of |
Three things about the benchmark:
|
fs::read_to_string will be stable as of Rust 1.26, so there’s definitely a precedent of having these sorts of functions in std. |
My main use of the Also concerned that it seems like #[derive(Rand)]
struct MyStruct {
...
}
let thing: MyStruct = rand::random(); This is the kind of excellent API that makes me very happy to program in Rust. I do not think it is worthwhile to make me learn about all the performance trade offs between different sources of randomness, or to consider a question of distribution, before I can create a random thing. Most of the time, I just don't care. If it matters to me, I will be self-motivated to learn about this and replace my code with the optimal choice for my use case. |
@dhardy yeah I understand my "generate a random instance of my type" is whofully underspecified, part of the problem being that I don't know any better. The times I wanted it I really just wanted to forward the rng/distribution my struct's members, eg Trying to stay a bit more on the topic of this issue: Looking at C#'s Random API, with Rust twist seems quite pleasant: The function What I'm trying to say is, highly technical terminology and extreme flexibility are fine as long as there's some straight forward convenience methods that map the casual user's expectations. Part of this suggestions is to drop ThreadRng as the default rng, instead favouring a simple rng initialized with a seed from the system's secure entropy. The specific type of rng returned is not that important, as long as it no longer depends on thread local state. meta: Is this too far off-topic? I'm not sure. I'm suggesting to change |
I made a PR to undeprecate |
Thanks @pitdicker @CasualX @withoutboats The discussion about deprecating
Why? To me We could rename We already have another type of generator if you don't want one tied to thread-local storage: |
@dhardy I was looking at the C# random API where Random is the class name to generate random values, analogous to a The approach C#'s random API takes also solves the performance issue of wrapping In short, |
@CasualX yes... personally I still hate the idea of using Another thing which crossed my mind is that some applications may not want to use
These would replace
|
@dhardy Libraries should probably never assume any RNG and just consume a |
Because there are roughly two main cases, and this caters for them.
Many uses of an RNG are not to "create a random value" (like |
@LukasKalbertodt If you want "quick and dirty but can't cover all the cases" look no further. @dhardy you can now feel free to abstract and specify to your heart's content ;P |
@Lokathor thanks, but I think there's still benefit in making this lib easy to use! |
@dhardy Why not have an opaque wrapper type then? This is nicer than |
Yes, you're right. But this issue has run it's course and I don't think there's any hurry to start tweaking |
I voted not, but I think it should be removed. The main problem would be backward compatibility. So maybe after introducing "editions"? |
There's not a strong compatibility issue since we're pre-1.0. Anything below 1.0 can have any change with only a minor version bump. |
Decision:
random()
functionweak_rng()
function withSmallRng
wrapperSmallRng
(but off topic here, thus not a blocker for this issue)Both the
weak_rng
and therandom
function are little helpers, but don't really do much. To be clear, in the next release users may simply:random()
withthread_rng().gen()
weak_rng()
withXorShiftRng::new()
(or a different PRNG)The first is pure convenience (but also a minor anti-pattern since caching the result of
thread_rng
can improve performance).The second has a small advantage: users may specify simply that they want a fast, weak PRNG, and don't mind if the implementation changes.
XorShiftRng
is reproducible, meaning that library updates won't change its output;weak_rng
is not reproducible, meaning the library may replace the internals with better generators in the future.Note that currently
weak_rng()
returnsXorShiftRng
which makes changing the former a breaking change; we should introduce a wrapper around the generator used (i.e.fn weak_rng() -> WeakRng
) or drop the function and only use a wrapper type (WeakRng::new()
).Note also that
thread_rng()
is staying because it is widely used and has additional features.We already wish to change the
weak_rng
algorithm and have two candidates (see dhardy#52 and dhardy#60), so it's possible we could have two variants, e.g.FastRng
andSmallRng
. This issue is not about which algorithm we choose.random()
convenience function?weak_rng()
or rename or only keep wrapper types, or remove completely?The text was updated successfully, but these errors were encountered: