Skip to content
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

Stabilize #[bench] and Bencher? #66287

Open
SimonSapin opened this issue Nov 11, 2019 · 8 comments
Open

Stabilize #[bench] and Bencher? #66287

SimonSapin opened this issue Nov 11, 2019 · 8 comments
Labels
A-libtest Area: `#[test]` / the `test` library B-unstable Blocker: Implemented in the nightly compiler and unstable. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

SimonSapin commented Nov 11, 2019

I’ll take the liberty of copying a section of @hsivonen’s Rust 2020 blog post:

Non-Nightly Benchmarking

The library support for the cargo bench feature has been in the state “basically, the design is problematic, but we haven’t had anyone work through those issues yet” since 2015. It’s a useful feature nonetheless. Like I said a year ago and the year before, it’s time to let go of the possibility of tweaking it for elegance and just let users use it on non-nighly Rust.

Indeed the existing benchmarking support has basically not changed in years, and I’m not aware of anyone planning to work on it. To keep reserving the right to make breaking changes is not useful at this point. Custom test frameworks offer another way forward for when someone does want to work on better benchmarking.

So I’d like to propose a plan:

  • Move test::Bencher to std::bench::Bencher. I have a PR coming soon that Move test::Bencher to a new (unstable) std::bench module #66290 demonstrates that this is possible. This move avoids the need to stabilize the test crate.
  • Optionally, make some surface-only API tweaks to Bencher. For example, the public bytes field could become a parameter to some method.
    • Although not strictly necessary for changes to an unstable type, we can keep the existing bytes field and iter method unchanged as unstable + deprecated for a while.
  • Stabilize the #[bench] attribute and just enough of Bencher to make it usable with #[bench]. (For example, no stable constructor.)

@rust-lang/libs, @rust-lang/lang, do you feel this needs an RFC?

@SimonSapin SimonSapin added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. A-libtest Area: `#[test]` / the `test` library labels Nov 11, 2019
SimonSapin added a commit to SimonSapin/rust that referenced this issue Nov 11, 2019
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.
@Centril
Copy link
Contributor

Centril commented Nov 11, 2019

do you feel this needs an RFC?

Given that "basically, the design is problematic" I'd say yes, we should have an RFC that justifies and explains the overall design (as if it was an unimplemented feature).

cc @rust-lang/dev-tools

@alexcrichton
Copy link
Member

I'd like to paste parts of this comment here:

I'm personally pretty wary and against stabilizing something just because progress isn't happening on it. Stabilization takes real work and has a cost. Simply because an API exists doesn't mean we should stabilize it, even if it's been sitting for months. I think it's pretty bad design to get things into the standard library, then point out it's sat with no feedback for months and propose stabilization.

I would ask again that the justification for stabilization has "it's not been touched for years" only taken as an input of whether to start the stabilization process, not an implicit ratification that the current design is "obviously good enough because no one has changed it".

There have been repeated attempts to stabilize the benchmarking infrastructure in the past and they've all been stymied because the libtest benchmarking support just isn't that great. It's great for quick and dirty things, but like #[test], it does not have extensibility hooks, it's not always the easiest to use, etc. Stabilization often also gets quickly into the weeds of "what to do about std::test?" or "what about custom test frameworks?" and such. I personally think it's very difficult to avoid these questions when proposing stabilization.

Given the history of this feature I would personally expect an RFC to justify why the current design is suitable for stabilization (or the proposal here). Again, though, the RFC will need to justify its design irrespective of the amount of time the design has sat in libtest for now.

@Manishearth
Copy link
Member

Manishearth commented Nov 11, 2019 via email

@SimonSapin
Copy link
Contributor Author

@Manishearth Yes, this proposal specifically does not involve opening a design discussion beyond small API-surface-only tweaks, so as to not divert efforts and limited team bandwidth from custom test frameworks or other work areas. (And I don’t feel up to personally leading such a discussion at the moment.)

@Manishearth
Copy link
Member

Oh, yeah, I agree, I actually think we should find a path for stabilizing bench itself at this point.

One thing I will point out is that a lot of people want some form of bench framework (and are likely willing to put in the work to design and stabilize just that), whereas far fewer people want custom test frameworks, and the work for that is proportionally larger.

I don't want to say that the justification for stabilization is "it hasn't been touched for years", but I do want to say that the fact that benching has been blocked on custom test frameworks has led to this annoying impasse where there just aren't enough interested people to do the huge amount of work to complete custom test frameworks. I'd still prefer if that happened, but that doesn't seem likely. I feel like as a community we have a tendency to fall into this trap, so I do see this issue as a push for a way out.

That said, I already opened an RFC for this and it got closed: rust-lang/rfcs#2287 . The reasoning there hasn't changed much, though I'd argue that the languishing of custom test frameworks is a valid argument to say that we should revisit this.

@SimonSapin
Copy link
Contributor Author

"It hasn't been touched for years" isn’t an argument for stabilization, it’s a sign that that more time as unstable is unlikely to lead to improvements to this benchmarking framework. Instead, I suspect that improvements will come as something entirely new that can be used through custom frameworks.

I think this leaves two realistic outcomes for #[bench]:

  • It is eventually removed, perhaps after alternatives become available through custom frameworks. The pFCP to close the previous RFC planned for this: Benchmarking / cargo bench rfcs#2287 (comment)

  • It is stabilized with its current design. People who are already happy enough with it can now use it on the Stable channel. It can then slowly fall out of use when something better comes up later. The Rust project is stuck with maintaining it "forever", but #[bench] is very similar to #[test] that we’re already stuck with. I feel that the additional maintenance burden might be light enough that the benefit until an alternative is available may be worth it.

    I understand there’s not a lot of appetite for this and that we generally set the bar high, but perhaps we can live with something known not to be perfect.

@gilescope
Copy link
Contributor

As a random datapoint: I would love to see exercism.io students being able to do simple benchmarks on their solutions to see if one way is faster than another. I know criterion is the gold plated solution but it would be nice to have something that works out of the box on stable.

Don't let perfect be the enemy of the good: If measuring performance is easier, then more people will do it, lowering the CO2 impact that rust solutions have on the world.

@Oliboy50
Copy link

Oliboy50 commented May 26, 2021

As a newcomer to Rust, I just wanted to say that it feels frustrating to learn that there is an awesome built-in benchmarking feature (which is something that I've never seen in the few languages I've worked with) but we "cannot" use it and doesn't seem like it will happen soon (since the following is written in the doc: The tracking issue for this feature is: None. )

Why is it even mentioned in The Book then? 😢

https://doc.rust-lang.org/book/ch11-01-writing-tests.html

The 0 measured statistic is for benchmark tests that measure performance. Benchmark tests are, as of this writing, only available in nightly Rust. See the documentation about benchmark tests to learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: `#[test]` / the `test` library B-unstable Blocker: Implemented in the nightly compiler and unstable. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
Status: No status
Development

No branches or pull requests

6 participants