Implement --perf flag to lintcheck for benchmarking #14116
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Turns out I was completely overcomplicating myself, there was no need for an external tool such as becnhv2 or even the original becnh, we already had the benchmarking infrastructure right under our noses! This PR implements a new **lintcheck** option called --perf, using it as a flag will mean that lintcheck builds Clippy as a release package and hooks perf to it. The realization that lintcheck is already 90% of what a benchmarking tool needs came to me in a dream.
|
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
rustbot
added
the
S-waiting-on-review
Alexendoo
reviewed
Jan 30, 2025
| @@ -46,6 +46,11 @@ pub(crate) struct LintcheckConfig { | |||
| /// Run clippy on the dependencies of crates specified in crates-toml | |||
| #[clap(long, conflicts_with("max_jobs"))] | |||
| pub recursive: bool, | |||
| /// Also produce a `perf.data` file, implies --jobs=1, | |||
There was a problem hiding this comment.
Doesn't look like this implies is true currently, there might be a #[clap] flag for it though
There was a problem hiding this comment.
I realized it immediately after uploading the issue, just updated it to actually imply (declare a default value, and exit if it the flags don't comply)
lintcheck/src/main.rs
Outdated
Comment on lines
237
to
264
| fn build_clippy() -> String { | ||
| fn build_clippy(release_build: bool) -> String { | ||
| let output = Command::new("cargo") | ||
| .args(["run", "--bin=clippy-driver", "--", "--version"]) | ||
| .args([ | ||
| "run", | ||
| "--bin=clippy-driver", | ||
| if release_build { "-r" } else { "" }, | ||
| "--", | ||
| "--version", | ||
| ]) | ||
| .stderr(Stdio::inherit()) |
There was a problem hiding this comment.
Would we want to bump up CARGO_PROFILE_RELEASE_DEBUG when perf is enabled too?
Alexendoo
reviewed
Jan 31, 2025
lintcheck/src/main.rs
Outdated
Comment on lines
289
to
294
| if config.perf && config.max_jobs != 1 { | ||
| eprintln!( | ||
| "Lintcheck's --perf flag must be triggered only with 1 job,\nremove either the --jobs/-j flag or the --perf flag" | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Should be able to use Clap's conflicts_with for this
lintcheck/src/main.rs
Outdated
| "--", | ||
| "cargo", | ||
| ]); | ||
| cmd.env("CARGO_PROFILE_RELEASE_DEBUG", "true"); |
There was a problem hiding this comment.
This would want to go in build_clippy as we want the debuginfo on the built clippy rather than the packages we're linting
blyxyas
force-pushed
the
lintcheck-perf
branch
from
e2a6764 to
48ae7ec
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 23, 2025
In #14116 we added a benchmarking option for Lintcheck, this commit adds a new chapter to the book AND improves that option into a more usable state. It's recommended to review one commit at a time. - **Document how to benchmark with lintcheck --perf** - **Several improvements on lintcheck perf (desc.)** - Now lintcheck perf deletes target directory after benchmarking, benchmarking with a cache isn't very useful or telling of any precise outcome. - Support for benchmarking several times without having to do a cargo clean. - Compress perf.data changelog: none
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Turns out I was completely overcomplicating myself,
there was no need for an external tool such as becnhv2
or even the original becnh, we already had the benchmarking
infrastructure right under our noses!
This PR implements a new lintcheck option called
--perf, using it as a flag will mean that lintcheck
builds Clippy as a release package and hooks perf to it.
The realization that lintcheck is already 90% of what
a benchmarking tool needs came to me in a dream ☁️
changelog:none