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

Add optional features #426

Open
bheisler opened this issue Jun 29, 2020 · 8 comments
Open

Add optional features #426

bheisler opened this issue Jun 29, 2020 · 8 comments
Labels
Breaking Change Requires an API-breaking change. Waiting for next major version.
Milestone

Comments

@bheisler
Copy link
Owner

Criterion.rs has a pretty large dependency tree, which can impact compilation time. Mostly this is on clean-compiles, which are relatively rare, but it also increases the amount of code the linker has to process on incremental builds as well. Criterion.rs should provide some Cargo features to cut down the size of the tree. Now is a good time to do this as well, since cargo-criterion will soon have its first alpha release - since that can be compiled and installed once with no impact on the linker, dependencies and code there have a much smaller compile-time cost than they do in Criterion.rs.

Here are some ideas:

  • Now that cargo-criterion is available to handle generating charts, I think the HTML reports should become an optional feature in 0.4, probably disabled by default. All of my new-feature development on the reports will probably happen in cargo-criterion as well, so the HTML reports will probably be deprecated and removed in 0.5.
  • A lot of the dependency tree is needed to do the statistical analysis and save results to disk. Both of these tasks are also handled by cargo-criterion, so this code is only needed when running with cargo bench. We could add a feature that represents cargo bench support. That way, users could disable it if they're using cargo criterion. I think this should be enabled by default though; users would be surprised if they can't use Criterion.rs with cargo bench out of the box.
  • When I added support for regex-based benchmark filtering, I turned off all of the nonessential features of the regex crate. Surprisingly, it still takes up quite a bit of compile time, so allowing regexes as filters could be a feature as well. I think it should be enabled by default though.

Disabling them all would leave Criterion.rs with just the code necessary to perform measurements and send them to cargo-criterion for analysis and reporting.

The last release in the 0.3.* series will expose these features, but it will just use them to enable warnings about any behavior that might change in 0.4.

@bheisler bheisler added the Breaking Change Requires an API-breaking change. Waiting for next major version. label Jun 29, 2020
@bheisler bheisler added this to the Version 0.4.0 milestone Jun 29, 2020
@bheisler
Copy link
Owner Author

bheisler commented Jul 5, 2020

While I'm at it, the CSV output should probably also become a non-default feature, slated for deletion. I'm not aware of anybody ever using it, and it's probably superseded for most real use cases by cargo-criterion's --message-format=json option.

@bheisler
Copy link
Owner Author

Didn't mean to close this.

@Stargateur
Copy link

Stargateur commented Mar 23, 2021

For people that don't want to use this feature you print an annoying big warning for each test. Consider have a opt-out.

@Luthaf
Copy link

Luthaf commented May 28, 2021

Now that cargo-criterion is available to handle generating charts, I think the HTML reports should become an optional feature in 0.4, probably disabled by default. All of my new-feature development on the reports will probably happen in cargo-criterion as well, so the HTML reports will probably be deprecated and removed in 0.5.

Would disabling the HTML reports also disable plot generation as well? I don't use these plots and I would love to be able to prune the dependency tree/remove the need for gnuplot.

@briansmith
Copy link

My use case for wanting to disable as much is possible is that I run cargo test --benches in CI just to make sure my benchmarks compile and run successfully. I don't need the plotting, HTML, saving results to disk, etc.

I see other people have contributed PRs to make a lot of this stuff optional already, and they were closed because they are breaking changes. I want to voice my support for moving on from 0.3.x to 0.4.x so that we make the functionality optional, and I would be happy to offer assistance.

@lemmih
Copy link
Collaborator

lemmih commented Jul 27, 2021

My use case for wanting to disable as much is possible is that I run cargo test --benches in CI just to make sure my benchmarks compile and run successfully. I don't need the plotting, HTML, saving results to disk, etc.

I see other people have contributed PRs to make a lot of this stuff optional already, and they were closed because they are breaking changes. I want to voice my support for moving on from 0.3.x to 0.4.x so that we make the functionality optional, and I would be happy to offer assistance.

I've created a new branch for version-0.4. Assistance and testing would be much appreciated. Discussion about what should be included in 0.4 and what should be pushed to 0.5 would also be a big help.

@emilk
Copy link

emilk commented Dec 11, 2023

We can probably switch out regex for regex-lite and save a bunch of compilation time

@faern
Copy link
Contributor

faern commented May 19, 2024

I would love a smaller dependency tree also. But mostly for a very different reason: Supply chain security. It had not been mentioned in this thread, so I figured I better add this aspect also. From this point of view it becomes more important to look at things like this when selecting dependencies:

  • Are the authors reputable/trustworthy?
  • How do they respond to issues found in their crates?
  • Do they have a good CI?
  • How securely do they seem to handle their git and crates.io access?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Requires an API-breaking change. Waiting for next major version.
Projects
None yet
Development

No branches or pull requests

7 participants