-
Notifications
You must be signed in to change notification settings - Fork 8
bump rand_core to 0.10.0-rc.2
#95
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
Conversation
|
cc @tarcieri |
d42c214 to
1accbdb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #95 +/- ##
==========================================
+ Coverage 97.44% 97.46% +0.01%
==========================================
Files 14 14
Lines 1722 1734 +12
==========================================
+ Hits 1678 1690 +12
Misses 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Cargo.toml
Outdated
| # need `crypto-bigint` with `alloc` to test `BoxedUint` | ||
| crypto-bigint = { version = "0.7.0-pre.5", default-features = false, features = ["alloc"] } | ||
| rand_chacha = "0.9" | ||
| rand_chacha = "0.10.0-rc.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps switch to chacha20 v0.10.0-rc.4 here? I think the plan is to deprecate rand_chacha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can drop it altogether and use the one re-exported by rand?
https://docs.rs/rand/0.10.0-rc.1/rand/rngs/struct.ChaCha20Rng.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rand_chacha::Chacha20Rng was Clone-able, chacha20::Chacha20Rng is not anymore. I've made a PR in rand-core to add a fork method in SeedableRng here:
rust-random/rand_core#17
This now depends on it.
1accbdb to
badc078
Compare
badc078 to
54ba25d
Compare
src/multicore.rs
Outdated
| loop { | ||
| if let Some(result) = self.sieve.next() { | ||
| return Some(result); | ||
| return Some((self.rng.fork(), result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the SieveIterator now injects a new forked Rng for each item it streams. This is the only way I've found to pass the rng to each predicate.
I think the weight of the operation is fairly minimal, but ... worth pointing it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the RNG, but probably negligible compared to testing a large number for primality. And definitely more correct than cloning the RNG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the RNG,
To elaborate on that, cloning is probably cheaper than forking/splitting, but for RNGs that are counter based, splitting is cheap (essentially a clone and generate a nonce). That said, forking is the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally you would rekey instead of generating a random nonce (ChaCha has 256-bit keys and 64-bit nonces for the variant we're using for RNGs, making collisions under random nonces too likely for comfort).
But the difference there is just extracting a bit more RNG output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baloo can you just use *Rng::from_rng here for now with a TODO to use fork, so it isn't blocked on another rand_core release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what the default implementation of fork() does in rust-random/rand_core#17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baloo can you just use
*Rng::from_rnghere for now with a TODO to use fork, so it isn't blocked on anotherrand_corerelease?
done.
8c948c7 to
18e8361
Compare
18e8361 to
7455a05
Compare
|
@tarcieri I assume Don't worry about the build-docs and semver failures, I'll deal with it in another PR Edit: actually, the doc-auto-cfg error comes from |
src/multicore.rs
Outdated
| loop { | ||
| if let Some(result) = self.sieve.next() { | ||
| return Some(result); | ||
| return Some((self.rng.fork(), result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the RNG,
To elaborate on that, cloning is probably cheaper than forking/splitting, but for RNGs that are counter based, splitting is cheap (essentially a clone and generate a nonce). That said, forking is the way to go.
|
@fjarri yes, we're in the process of bumping everything to use I've already cut a |
9a7e805 to
b05b17a
Compare
b05b17a to
9030605
Compare
|
The other failures look unrelated. Would it be possible to get this landed and get another prerelease out? Unfortunately this |
| })) | ||
| Ok(iter | ||
| .par_bridge() | ||
| .find_map_any(|(mut rng, c)| if predicate(&mut rng, &c) { Some(c) } else { None })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, wait, how does that work? Why don't we need to clone the RNG anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clone on an RNG is not a really good idea as either forks of the clone will have a similar internal state and will yield the same bits.
Hence the idea to use SeedableRng::fork instead (which needs to take a &mut to mutate the internal state of the source RNG).
rand_chacha used to carry Clone: https://docs.rs/rand_chacha/latest/rand_chacha/struct.ChaCha20Core.html#impl-Clone-for-ChaCha20Core (rand_chacha is getting deprecated)
chacha20:rngs does not anymore: https://docs.rs/chacha20/0.10.0-rc.5/chacha20/struct.ChaCha20Rng.html
Here instead, the rng is injected by the iterator in each of the items (using fork).
It's a couple of lines down from this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fork comes in the next rc of rand_core, I will send a followup to migrate)
`--html-in-header` apparently only works with the `--no-deps` option when `cargo doc` is invoked, which `cargo semver-checks` isn't doing This removes the rustdocflags overrides in `.cargo/config.toml` by deleting the file first.
|
That's weird, I'm getting a lot of compilation errors locally after this. Going to publish a new release after I figure it out. |
|
|
|
@fjarri thanks for the release! Got everything updated |
No description provided.