Add more arguments to x perf
#126853
Add more arguments to x perf
#126853
Conversation
rustbot
added
S-waiting-on-review
|
This PR modifies If appropriate, please update |
| impl Default for PerfArgs { | ||
| fn default() -> Self { | ||
| Self { cmd: PerfCommand::Eprintln, opts: SharedOpts::default() } |
There was a problem hiding this comment.
Shouldn't we run all the benchmarks by default with something like PerfCommand::All when the user doesn't specify a particular benchmark module (e.g., x perf)?
There was a problem hiding this comment.
Just to clarify, these are individual profilers (eprintln, cachegrind, etc.), not benchmarks. I don't think that it makes sense to run all of them at once, it's like running cargo check/build/test at once, they just do very different things and serve different purposes. We could select some default, but I'm not sure if there even is a default profiler.
Once we actually add benchmarking in the future, I would propose x perf to just run benchmarks, but since we only have profilers now, I would opt into requiring users to select the profiler that they want to use.
There was a problem hiding this comment.
IMO it makes more sense to run samply (perhaps even together with cachegrind) rather than running eprintln by default. Currently the eprintln default doesn't really provide a good overall report about the compiler.
There was a problem hiding this comment.
So, just to clarify, I would prefer to keep the current situation, where you cannot just do x perf, but you have to select some profiler. Because currently I don't think there is a good default - for both samply and cachegrind, you need to have an external tool installed, so that isn't IMO a good default.
This Default implementation should be basically unreachable!.
onur-ozkan
added
S-waiting-on-author
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Btw, since this now actually does something, maybe we could let users know by adding an entry in the config tracker? |
This comment has been minimized.
This comment has been minimized.
Kobzol
force-pushed
the
bootstrap-perf-args
branch
from
973fe8b
to
a29b18a
Compare
| impl Default for PerfArgs { | ||
| fn default() -> Self { | ||
| Self { cmd: PerfCommand::Eprintln, opts: SharedOpts::default() } |
There was a problem hiding this comment.
IMO it makes more sense to run samply (perhaps even together with cachegrind) rather than running eprintln by default. Currently the eprintln default doesn't really provide a good overall report about the compiler.
| /// Run `profile_local samply` | ||
| /// This executes the compiler on the given benchmarks and profiles it with `samply`. | ||
| /// You need to install `samply`, e.g. using `cargo install --locked samply`. | ||
| Samply, |
There was a problem hiding this comment.
I just ran x perf samply and it turns out it needs root access (or I have to update /proc/sys/kernel/perf_event_paranoid). We probably need to extend this documentation here.
There was a problem hiding this comment.
Right, so this is a bit tricky. In general, many of the profilers require some custom configuration of the system, and also some postprocessing. This is documented in rustc-perf, and I'm not sure if it is a good idea to duplicate the documentation here.
The tool should let you know exactly what you should do w.r.t. perf_event_paranoid, so it is self-documenting in this sense, but if you prefer, we could add a mention of perf_event_paranoid (probably to rustc-perf?).
Furthermore, even if you run the profiler, it just generates a file on disk and unless you know what to do, it won't be very useful to you. So maybe we could create specific postprocessing steps in bootstrap, for example to open a web browser with the gathered profile after running samply. But even that is not that easy, because what if you run more than one benchmark? :)
Doing profiling and benchmarking does require some specific knowledge at the moment. We could make it easier for the user, but I'm not sure if that code belongs here or into rustc-perf, and how far should we go in this.
onur-ozkan
added
S-blocked
|
Closing in favor of #127002. |
Continues work from #126318, adds a CLI for running
rustc-perfprofiling commands.This is probably most of what we can do so far, I'll add support for benchmarking once
rustc-perfgets a terminal output for comparing benchmark results.r? @onur-ozkan