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

feat: run benchmarks in non-bench targets #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ariofrio
Copy link

@ariofrio ariofrio commented Feb 21, 2023

As discussed in and closes #84. I also made it use the bench profile by default when --benchmarks is supplied, and add the --bench argument to the target args like cargo bench does (otherwise it runs unit tests instead).

For reference, you can (optionally) specify which benchmarks to run by adding them as target args:

cargo instruments -t time --benchmarks -- <my_bench_function_name>

Otherwise it'll run all of the benchmarks in the target.

Limitations:

  • When there are multiple targets with benchmarks, I expect cargo instruments -t time --benchmarks to fail because it'll build multiple targets and it expects a single one (like if --benchmarks hadn't been supplied). AFAIK, cargo instruments can only run one target, so it has no analogous operation to cargo bench running benchmarks in multiple targets by default, so maybe this is OK. An alternative would be to run benchmarks in the main target by default, but this might be unexpected for users of cargo bench. Workaround is to specify a single target like cargo instruments -t time --benchmarks --bin <crate_name>. Might be nice to point users in this direction in the error message.
  • I'm still unsure about the name of the flag being --benchmarks since everywhere else in cargo "bench" is used, but overloading the --bench option seems more confusing. I could see an argument for --benchmark as it looks like an action, instead of using two plurals in a row ("instruments", "benchmarks"). But --benchmarks makes it clear that it'll run all of them in the chosen target.
  • If the user naively uses cargo instruments --bench without a value, the error message doesn't ask them to use --benchmarks instead. But I did add a note about it in the --help message for --bench.
  • We're prepending --bench to the target args like cargo bench does, but I haven't checked the cargo bench source code to see if there are exceptions to this behavior (say, when the default test harness is disabled).
  • Maybe cargo instruments --bench should imply --benchmarks? We'd probably want to use the bench profile by default, for example. I'm not using non-default test harnesses or separate bench targets though, so I haven't tested what the best behavior here should be.
  • Cannot run benchmarks in lib or test targets.

@ariofrio
Copy link
Author

Thinking about it further, an alternative design could be to make bench and run a subcommand instead of an option, analogous to the way bare cargo works. This could open the door to other modes. Does a test mode make sense for the future? Are there other modes that would?

It would look like this:

cargo instruments run -t time
cargo instruments bench -t time

Maybe make the subcommand name optional and default to run, though I'm not sure how hard it is to make clap and structopts do that.

@ariofrio
Copy link
Author

Another limitation of this pull request is that it often rebuilds the whole target even if I just ran cargo bench right before. I'm not sure why this is happening, it seems to me that using the same profile should re-use the build artifacts.

@cmyr
Copy link
Owner

cmyr commented Mar 7, 2023

hey sorry was just away for a couple weeks.

I am a bit confused by this.

The behaviour I would like is that you use --bench to select a benchmark target (something marked with #[bench]) and then profile that target. I don't think it is important for us to support running other targets in benchmark mode (am I overlooking something?) and so I think we should set whatever options we need to make building a benchmark work, when we see the --bench flag. I would like to avoid adding a new option unless absolutely necessary.

@zxch3n
Copy link

zxch3n commented Aug 28, 2023

@cmyr cargo criterion cannot run successfully when using cargo instruments currently

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.

Cannot run benches in the main target, examples, or libs
3 participants