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

Make much more functionality optional. #402

Closed
wants to merge 3 commits into from
Closed

Conversation

thomcc
Copy link

@thomcc thomcc commented Apr 11, 2020

Criterion is painful to use in many cases -- bloating the dependency tree so badly that I move benchmarks into another crate excluded from the workspace -- Otherwise just adding a single criterion benchmark expands the dev-dependency tree so much that it's a huge hit to dev ergonomics.

It has many features -- I don't mind these features on their own. I don't think criterion is a bad library, it just has many bad side effects that should be addressed. The use case I'd want to be available is just looking at CLI output, and then if need to dig in, turn on more features and accept the build time, or something.

Anyway I've probably ranted too much to make you want to accept this work. Sorry. Towards the end these patches got pretty sloppy anyway, which is why I stopped... Also I didn't feel like checking if anything other than default-features and no-default-features worked (probably they are slightly broken).

As is it now, it cuts builds on my machine from 27s to 17s for a debug build and 56s to 40s for a release. Next logical step would be removing serde (this code removes a ton of its use), then ideally rayon although it's not quite as bad and might pay for itself in terms of time IDK.

Outputs from full cargo clean; cargo +nightly build -Z timing of before/after here:

https://gist.github.com/thomcc/e1f4bda8bafa4e164483a1b942782a61

Thom Chiovoloni added 3 commits April 10, 2020 16:54
When disabled `filter` just runs benchmarks with names that contain the
filter (which was what the documentation said it did anyway)
@bheisler
Copy link
Owner

Yes, the dependency tree is quite large. I've tried to keep it as small as I can, but I feel your pain.

The way I intend to deal with this is to create a separate cargo criterion subcommand that invokes the benchmark executables and generates the plots. Then the benchmark executables themselves don't need to link in nearly so much code, and could be limited to just command-line and machine-readable output (probably using serde-json). The benchmarks would be much more lightweight, and the user would only have to compile the plot-generation code when they install the subcommand.

Unfortunately, I don't think I can hide existing code behind a cargo feature (even a default one) without breaking semver compatibility. If I'm going to do that, I might as well break it only once to introduce the subcommand.

@thomcc
Copy link
Author

thomcc commented Apr 27, 2020

Unfortunately, I don't think I can hide existing code behind a cargo feature (even a default one) without breaking semver compatibility.

I'm pretty sure this is not true. Many crates have made changes like this, including big ones like serde. Consider changes like adding no_std support, and then putting std behind a default-on feature.

@bheisler
Copy link
Owner

Hey, thanks again for the suggestion. I have some plans in this direction myself in the next breaking-change release - see #426 - but for the 0.3.* series I'll leave things as they are.

@bheisler bheisler closed this Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants