-
Notifications
You must be signed in to change notification settings - Fork 565
Fix #1692: Add example for profiling diff locally #2696
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR. If you have write access, feel free to merge this PR if it does not need reviews. You can request a review using |
This comment has been minimized.
This comment has been minimized.
07db40e to
210e162
Compare
|
r? rustc-dev-guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you show this command instead
cargo run --release --bin collector
this would remove the need to indicate where the exe is located
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done now, apologize for taking so long. I have also updated the branch to be on the latest main. (Took a few commits to get right but now it should be proper :)) Does it look okay now? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not have bothered with updating the branch, unless there was a conflict with main. I have submitted #2712 to remove the bot messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you for the update :) There was no conflict with main, just how I am used to working to rebase onto the latest main branch. Good to know that it is not necessary!
This comment has been minimized.
This comment has been minimized.
9be9888 to
b9c4ece
Compare
This comment has been minimized.
This comment has been minimized.
b9c4ece to
36d9f2e
Compare
This comment has been minimized.
This comment has been minimized.
36d9f2e to
a5ae29b
Compare
This comment has been minimized.
This comment has been minimized.
a5ae29b to
b45c95a
Compare
This comment has been minimized.
This comment has been minimized.
Added an example for profiling an external crate diff locally. The original issue also mentions that the debuginfo-level = 1 should be highlighted, but that has been solved by a different commit and as such was not included here.
Changed so cargo specifies the binary collector, removing the need to link to its local binary. Clarified that the SHAs should be from the rustc-repo, but the command should be ran in the rustc-perf repo.
b45c95a to
cadcc71
Compare
| The collector can then be run using cargo, specifying the collector binary. It expects the following arguments: | ||
| - `<PROFILE>`: Profiler selection for how performance should be measured. For this example we will use Cachegrind. | ||
| - `<RUSTC>`: The Rust compiler revision to benchmark, specified as a commit SHA from `rust-lang/rust`. | ||
| Optional arguments allow running profiles and scenarios as described above. `--include` in `x perf` is instead `--exact-match`. More information regarding the mandatory and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean
--includeinx perfis instead--exact-match
| Optional arguments allow running profiles and scenarios as described above. `--include` in `x perf` is instead `--exact-match`. More information regarding the mandatory and | ||
| optional arguments can be found in the [rustc-perf-readme-profilers]. | ||
|
|
||
| Then, for the case of generating a profile diff for the crate `serve_derive-1.0.136`, for two commits `<SHA1>` and `<SHA2>` from the `rust-lang/rust` repository, run the following in the `rustc-perf` repo (where the collector was built in the previous step): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding
where the collector was built in the previous step
I would not show the command to build it, since the command to run it also builds it
|
sorry this is taking so long, but had to learn how this works so I can confidently approve... just really one more thing before I can do so |
Added an example for profiling an external crate diff locally. #1692 also mentions that the debuginfo-level = 1 should be mentioned, but that has been solved by a different commit and as such was not included here.