Avoid using rand::thread_rng in the stdlib benchmarks.
#96626
Conversation
rustbot
added
the
T-libs
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
r? @m-ou-se (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
There was previous try to update rand in #86963, so maybe you hit the same errors on the path. |
This comment has been minimized.
This comment has been minimized.
|
|
I'm willing to revert that patch if it causes trouble, though I think it should be fine -- this is only an update of the dev-dependencies in the stdlib, and doesn't touch rand anywhere else in the compiler, which seems like a bit of a quagmire. My feeling here his also matches the comment in #86963 (comment) (emph mine):
But yeah, I don't mind reverting the first patch in this PR (which does the update) either. |
|
Ah, I see. I suppose this may break the stdlib tests on wasm. Hmm, I'll see if I can check locally. I suspect that we don't actually need high quality randomness in the stdlib's tests (they're mostly for generating random data to test against), although we could just seed an rng from entropy from the |
|
Ah. That's quite a headache, because I don't think it'd be good behavior for stdlib to enable Either way, given that updating the version of |
|
@bors r+ |
|
📌 Commit 0812759 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (12d3f10): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
…, r=Mark-Simulacrum Update `rand` in the stdlib tests, and remove the `getrandom` feature from it. The main goal is actually removing `getrandom`, so that eventually we can allow running the stdlib test suite on tier3 targets which don't have `getrandom` support. Currently those targets can only run the subset of stdlib tests that exist in uitests, and (generally speaking), we prefer not to test libstd functionality in uitests, which came up recently in rust-lang#104095 and rust-lang#104185. Additionally, the fact that we can't update `rand`/`getrandom` means we're stuck with the old set of tier3 targets, so can't test new ones. ~~Anyway, I haven't checked that this actually does allow use on tier3 targets (I think it does not, as some work is needed in stdlib submodules) but it moves us slightly closer to this, and seems to allow at least finally updating our `rand` dep, which definitely improves the status quo.~~ Checked and works now. For the most part, our tests and benchmarks are fine using hard-coded seeds. A couple tests seem to fail with this (stuff manipulating the environment expecting no collisions, for example), or become pointless (all inputs to a function become equivalent). In these cases I've done a (gross) dance (ab)using `RandomState` and `Location::caller()` for some extra "entropy". Trying to share that code seems *way* more painful than it's worth given that the duplication is a 7-line function, even if the lines are quite gross. (Keeping in mind that sharing it would require adding `rand` as a non-dev dep to std, and exposing a type from it publicly, all of which sounds truly awful, even if done behind a perma-unstable feature). See also some previous attempts: - rust-lang#86963 (in particular rust-lang#86963 (comment) which explains why this is non-trivial) - rust-lang#89131 - rust-lang#96626 (comment) (I tried in that PR at the same time, but settled for just removing the usage of `thread_rng()` from the benchmarks, since that was the main goal). - rust-lang#104185 - Probably more. It's very tempting of a thing to "just update". r? `@Mark-Simulacrum`
…Simulacrum Update `rand` in the stdlib tests, and remove the `getrandom` feature from it. The main goal is actually removing `getrandom`, so that eventually we can allow running the stdlib test suite on tier3 targets which don't have `getrandom` support. Currently those targets can only run the subset of stdlib tests that exist in uitests, and (generally speaking), we prefer not to test libstd functionality in uitests, which came up recently in rust-lang/rust#104095 and rust-lang/rust#104185. Additionally, the fact that we can't update `rand`/`getrandom` means we're stuck with the old set of tier3 targets, so can't test new ones. ~~Anyway, I haven't checked that this actually does allow use on tier3 targets (I think it does not, as some work is needed in stdlib submodules) but it moves us slightly closer to this, and seems to allow at least finally updating our `rand` dep, which definitely improves the status quo.~~ Checked and works now. For the most part, our tests and benchmarks are fine using hard-coded seeds. A couple tests seem to fail with this (stuff manipulating the environment expecting no collisions, for example), or become pointless (all inputs to a function become equivalent). In these cases I've done a (gross) dance (ab)using `RandomState` and `Location::caller()` for some extra "entropy". Trying to share that code seems *way* more painful than it's worth given that the duplication is a 7-line function, even if the lines are quite gross. (Keeping in mind that sharing it would require adding `rand` as a non-dev dep to std, and exposing a type from it publicly, all of which sounds truly awful, even if done behind a perma-unstable feature). See also some previous attempts: - rust-lang/rust#86963 (in particular rust-lang/rust#86963 (comment) which explains why this is non-trivial) - rust-lang/rust#89131 - rust-lang/rust#96626 (comment) (I tried in that PR at the same time, but settled for just removing the usage of `thread_rng()` from the benchmarks, since that was the main goal). - rust-lang/rust#104185 - Probably more. It's very tempting of a thing to "just update". r? `@Mark-Simulacrum`
This is kind of an anti-pattern because it introduces extra nondeterminism for no real reason. In thread_rng's case this comes both from the random seed and also from the reseeding operations it does, which occasionally does syscalls (which adds additional nondeterminism). The impact of this would be pretty small in most cases, but it's a good practice to avoid (particularly because avoiding it was not hard).
Anyway, several of our benchmarks already did the right thing here anyway, so the change was pretty easy and mostly just applying it more universally. That said, the stdlib benchmarks aren't particularly stable (nor is our benchmark framework particularly great), so arguably this doesn't matter that much in practice.
Anyway, this also bumps theNevermind, too much of a headache.randdev-dependency to 0.8, since it had fallen somewhat out of date.