Deprecate random and weak_rng, add SmallRng
#296
Conversation
`SmallRng` is an opaque wrapper type similar to `StdRng`. See rust-random#289.
| //! means that often a simple call to `rand::random()` or `rng.gen()` will | ||
| //! suffice, but sometimes an annotation is required, e.g. | ||
| //! `rand::random::<f64>()`. | ||
| //! The key function is `Rng::gen()`. It is polymorphic and so can be used to |
There was a problem hiding this comment.
This probably wants a bit more re-writing, but this doesn't have to be done now.
There was a problem hiding this comment.
Yes, I think we should advertise Rng::sample or Distribution::sample instead and possibly get rid of Rng::gen. But this is still under discussion in #293.
| use rand::seq::*; | ||
|
|
||
| #[bench] | ||
| fn misc_shuffle_100(b: &mut Bencher) { | ||
| let mut rng = weak_rng(); | ||
| let mut rng = SmallRng::from_rng(&mut thread_rng()).unwrap(); |
There was a problem hiding this comment.
This is really not very ergonomic; we've gone from 1 identifier to 4 with lots of extra syntax too. Any better ideas?
Error handling is pedantic because thread_rng will almost never fail, but we can't really be removed from from_rng.
We could allow something like thread_rng().new_rng::<SmallRng>() but it's not a lot better and adds a function to Rng which doesn't have many other uses.
We could just rename weak_rng to small_rng. Not sure.
There was a problem hiding this comment.
I'm not sure it's an issue outside of small examples. Usually you want to reuse thread_rng(). Maybe something like the following makes it more clear in the examples:
// Create a big, expensive to initialize and slower, but unpredictable RNG.
// This automatically done only once per thread.
let mut thread_rng = thread_rng();
// Create small, cheap to initialize and fast RNG with a random seed.
// This is very unlikely to fail.
let mut small_rng = SmallRng::from_rng(&mut thread_rng).unwrap();This is even more verbose, but it teaches the correct pattern.
There was a problem hiding this comment.
It's also wrong: thread_rng() is only slow on the first call per thread. SmallRng::new() would be faster if thread_rng was never otherwise used — but chances are good that it would be used somewhere, I guess (at least once std's hash function randomisation uses this, not its own version).
Yes, copy-pasting this is fine, but often people don't look up the doc, they just vaguely remember how to do something and think, ah, SmallRng::new() should work.
There was a problem hiding this comment.
It's also wrong: thread_rng() is only slow on the first call per thread.
That's what I was trying to say with "This automatically done only once per thread.", but I guess it could be said more clearly.
There was a problem hiding this comment.
Any better ideas?
We could implement NewRng for SmallRng such that it uses thread_rng(). However, this requires getting rid of the impl for SeedableRng or specialization, which is not stable yet.
There was a problem hiding this comment.
I think using NewSeeded everywhere is fine. It is also how things used to be until a few months ago. But it would be nice to mention the thread_rng() method in the documentation just to point out a potential (small) performance advantage. How often will initializing the RNG be a bottleneck?
And I am still not sure thread_rng() is used all that much always. There is std::collections::HashMap that may also use it in the future, and maybe a crates dependency. If it is otherwise not used, spinning up thread_rng for one initialization of SmallRng is wasteful.
|
Oh, and I was less sure about removing |
|
Looks good to me! |
Now initializing using `NewRng` is used to simplify the examples. The documentation for `SmallRng` recommends using `thread_rng` when initializing a lot of generators.
I changed the examples to use |
|
Looks good to me! @pitdicker feel free to merge. One small thing stands out, but it's definitely beyond the scope of this PR: |
Seems like a good idea to me (after getting used to it...)! |
Deprecate `random` and `weak_rng`, add `SmallRng`
See #289.