Move test::Bencher to a new (unstable) std::bench module
#66290
Conversation
This is a first step toward stabilizing it and the `#[bench]` attribute: rust-lang#66287 To avoid also moving much of the `test` crate `Bencher` is now only responsible for runnning user code and measuring the time it takes, not anymore for doing statistical analysis. This separation is based on `&mut dyn FnMut` callbacks. This introduces dynamic dispatch, which in general could affect performance characteristics of a program. However I expect benchmarking results not to be affected here since the {`Instant::new`; user code; `Instant::elapsed`} sequence is kept monomorphized and not crossing a dynamic dispatch boundary. This also adds a lifetime parameter to the `Bencher` struct, which is is technically a breaking change to an unstable type. I expect the impact to be low on existing users: only those with `#[deny(warnings)]` or `#[deny(elided_lifetimes_in_paths)]` would need to change their code. (See next commit.) This lint is `allow` by default.
…ith `-D warnings`
This fixes errors such as:
```rust
error: hidden lifetime parameters in types are deprecated
--> src/libserialize/hex/tests.rs:56:29
|
56 | pub fn bench_to_hex(b: &mut Bencher) {
| ^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
|
= note: `-D elided-lifetimes-in-paths` implied by `-D warnings`
```
Note however that the relevant lint is `allow` by default,
so most crates are not similarly affected.
|
r? @rkruppe (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -454,6 +454,7 @@ pub mod f64; | |||
| pub mod thread; | |||
| pub mod ascii; | |||
| pub mod backtrace; | |||
| pub mod bench; | |||
There was a problem hiding this comment.
Shouldn't the module also have a stability annotation?
There was a problem hiding this comment.
Yes? It’s near the top of libstd/bench.rs, like for most other modules under std::
There was a problem hiding this comment.
I saw that, but I guess it isn't necessary on the mod item itself?
There was a problem hiding this comment.
This (inner) attribute is already on the mod item. If we switch to inline module syntax, this PR is equivalent to:
pub mod bench {
#![unstable(…)]
// …
}Which is equivalent to an outer attribute:
#[unstable(…)]
pub mod bench {
// …
}
|
May I ask why dynamic dispatch and not generic? |
|
Dynamic dispatch avoids forcing a type parameter on all functions with the |
|
Ping from triage: |
|
This PR has served its initial purpose of demonstrating feasibility. I’ll close it now to avoid keeping it open for a long time on triage’s radar. We can re-open it if and when we decide in #66287 or in an RFC that this the approach we want. |
This is a first step toward stabilizing it and the
#[bench]attribute: #66287To avoid also moving much of the
testcrateBencheris now only responsible for runnning user code and measuring the time it takes, not anymore for doing statistical analysis. This separation is based on&mut dyn FnMutcallbacks.This introduces dynamic dispatch, which in general could affect performance characteristics of a program. However I expect benchmarking results not to be affected here since the {
Instant::new; user code;Instant::elapsed} sequence is kept monomorphized and not crossing a dynamic dispatch boundary.This also adds a lifetime parameter to the
Bencherstruct, which is is technically a breaking change to an unstable type. I expect the impact to be low on existing users: only those with#[deny(warnings)]or#[deny(elided_lifetimes_in_paths)]would need to change their code. (See second commit.) This lint isallowby default.