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

Use "test" profile for cargo build benchmarks. #6309

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 12, 2018

When using cargo build (without --release), build benchmarks using the "test" profile. This was causing some confusion where the benchmark is placed in the target/debug directory, and also causing some duplicates that may not be expected. It also makes it easier to debug benchmarks (previously you had to edit the [profile.bench] profile).

Closes #5575, closes #6301, closes #4240, closes #4929.

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

This seems like a good and natural implementation to me, thanks @ehuss!

Do others from @rust-lang/cargo feel strongly one way or another here?

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 13, 2018

I don't know this code well enough to know these simple questions:

Is there a way in which this will break existing users?
Is there a change to when the profile sections in Cargo.toml apply?
If I run cargo bench (no --release) I am benchmarking optimized code. True? Before and After?

@ehuss
Copy link
Contributor Author

ehuss commented Nov 13, 2018

Is there a way in which this will break existing users?

I think it is unlikely. Today, using cargo build to build a benchmark works in a weird way. It builds the benchmark with the "bench" profile, but all dependencies use the "dev" (debug/unoptimized) profile. If you use --all-targets or --benches, it is difficult/impossible to determine which artifact is the benchmark one, so I doubt anyone is relying on that.

Is there a change to when the profile sections in Cargo.toml apply?

The only change is that bench targets with cargo build go from "bench" profile to "test" profile. cargo build --release is unchanged.

If I run cargo bench (no --release) I am benchmarking optimized code. True? Before and After?

Previously, only the final target was optimized. So if you had benches/b1.rs, the code in b1.rs uses the (optimized) "bench" profile, but all dependencies, like the linked src/lib.rs, are built with the "dev" (unoptimized) profile. With this PR everything uses the corresponding debug/unoptimized profiles.
EDIT: Misread your question. cargo bench is unaffected (always uses bench/release profiles).

@dwijnand
Copy link
Member

This LGTM

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 13, 2018

This sounds like it is definitely an improvement to consistency and explainability!

However, I, as an experienced rustation, was under the impression that cargo bench always used --release. I was wrong. I can not now find where I got this miss impression from. But, how can we publicize that if you're benchmarking debug code you are probably doing something wrong?

EDIT: LGTM

@ehuss
Copy link
Contributor Author

ehuss commented Nov 13, 2018

cargo bench always used --release

Oh my, I misread what you said. Yes, bench always uses --release.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 14, 2018

📌 Commit 739c272 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 14, 2018

⌛ Testing commit 739c272 with merge 51d541f...

bors added a commit that referenced this pull request Nov 14, 2018
Use "test" profile for `cargo build` benchmarks.

When using `cargo build` (without `--release`), build benchmarks using the "test" profile. This was causing some confusion where the benchmark is placed in the `target/debug` directory, and also causing some duplicates that may not be expected. It also makes it easier to debug benchmarks (previously you had to edit the `[profile.bench]` profile).

Closes #5575, closes #6301, closes #4240, closes #4929.
@bors
Copy link
Contributor

bors commented Nov 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 51d541f to master...

@bors bors merged commit 739c272 into rust-lang:master Nov 14, 2018
@ehuss ehuss added this to the 1.32.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment