-Zthreads=N support implemented as an additional dimension 'Parallel'#2421
-Zthreads=N support implemented as an additional dimension 'Parallel'#2421azhogin wants to merge 1 commit into
Conversation
00b1a2c to
8b6b683
Compare
|
The problem with this is that parallelism is an entirely separate dimension: parallel |
Yes, the idea was to add only "Full/check" at first to estimate the increased execution costs and maybe implement additional modes later. Should I rename |
|
@Kobzol, do you have any thoughts/plans about this? |
|
I definitely have thoughts, but I'm sick now and don't have the energy to comment. I'll try to write something by the end of this week. |
|
I wish @azhogin wrote more about the motivation and goals when posting this.
Having some of the
|
|
Just as a note: we have I think at least one benchmark running with the parallel frontend (but 4 threads). This setup doesn’t look like it should be a scenario to me either, but let’s wait for jakub’s thoughts once he feels better. |
|
So, first, let me say that I agree that we should be preparing for benchmarking the parallel frontend in a better way, and I'm happy to help with that, both on the rustc-perf and the infra side! Regarding the implementation, we should not be adding a new scenario. As Nick said, That being said, we could do something temporary to benchmark only a few situations with the parallel frontend. And we actually already did that last year 😅 We have the serde benchmark running with 4 threads, across all scenarios and profiles. In terms of variance, it does not seem egregiously higher than the base variant (parallel vs serial), at least for the So I guess that if we want to extend this, we have the several options:
So I think that 2) is the way forward. It's not completely trivial to add a new axis to the compilation benchmarks, but it's also not super hard, it "just" needs some boilerplate code that propagates the new config option throughout the collector, the database, the website backend, and then also the frontend. We have a bunch of PRs from last year that added the One additional thing to note is that we haven't yet figured out the best metric to check here, I think. I'm not sure if cycles really work, as they will grow with the number of threads. That being said, we mostly use rustc-perf to compare historical data for the same configuration, rather than to compare across configurations, so it doesn't matter that much. But still it would be good go have a metric that can show small changes in parallel code. Regarding
Happy to talk about that in the #t-infra stream on Zulip. I think that we have enough capacity now to add more benchmarks, so that shouldn't be a big issue. |
That's what this PR attempts to achieve, covering all the benchmarks, but only for check profiles, because using all the profiles would be too much work. The number of threads depends on the number of cores on the benchmarking machines, if they have 8+ cores then I'd prefer to use
I don't think we should go this way eventually, it will ruin icount-based benchmarking which is used as a source of truth for any changes not involving redistributing work between threads.
Only wall time I guess. |
@azhogin Could you implement this ^^^ suggestion? |
40c2dcc to
177f175
Compare
882188d to
ef56c53
Compare
ef56c53 to
75e9c33
Compare
75e9c33 to
3ffe80f
Compare
'Parallel' implemented as an additional axis with default values |
|
Sorry, I didn't have time for most reviews in the past few months. Starting from next week, it should be better, I promise to take a look then. |
Kobzol
left a comment
There was a problem hiding this comment.
Left an initial review, sorry that this took so long, I didn't have much time for reviews, or Rust for that matter, in the past few months.
I'd like for the website changes to be split into a separate PR, and land just the collector + DB side first, as the PR is quite big already.
| self_profile_storage: Option<Box<dyn SelfProfileStorage>>, | ||
| bench_rustc: bool, | ||
| targets: Vec<Target>, | ||
| parallels: Vec<Parallel>, |
There was a problem hiding this comment.
| parallels: Vec<Parallel>, | |
| frontend_threads: Vec<FrontendParallelism>, |
That should describe more accurately what we are talking about.
| /// Parallel frontend options in comma-separated list | ||
| #[arg(long, value_delimiter = ',', default_value = "1,4")] | ||
| parallels: Vec<u32>, |
There was a problem hiding this comment.
| /// Parallel frontend options in comma-separated list | |
| #[arg(long, value_delimiter = ',', default_value = "1,4")] | |
| parallels: Vec<u32>, | |
| /// Parallel frontend thread values in comma-separated list | |
| #[arg(long, value_delimiter = ',', default_value = "1")] | |
| frontend_threads: Vec<u32>, |
| } else { | ||
| Scenario::all_non_incr() | ||
| }; | ||
| let parallels = if collector::version_supports_parallel_frontend(&toolchain.id) { |
There was a problem hiding this comment.
I think that for now it would be enough to just use 1 for the stable benchmarks.
| if remaining_scenarios.is_empty() { | ||
| continue; | ||
| } | ||
| for parallel in parallels { |
There was a problem hiding this comment.
| for parallel in parallels { | |
| for thread_count in frontend_threads { |
| use std::fmt; | ||
|
|
||
| #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] | ||
| pub struct Parallel(pub u32); |
There was a problem hiding this comment.
| pub struct Parallel(pub u32); | |
| pub struct FrontendThreads(pub u32); |
Or FrontendParallelism, I think that both works. We should clarify that it is related to the frontend though, Parallel is ambigous.
| } | ||
| } | ||
|
|
||
| impl fmt::Display for Parallel { |
There was a problem hiding this comment.
I don't think that the display implementation should be showing the compiler flag. I'd rather remove the Display impl completely or just let it print the inner value directly.
| let mut cmd = self.base_command(self.cwd, cargo_subcommand); | ||
| cmd.arg("-p").arg(self.get_pkgid(self.cwd)?); | ||
| if self.parallel.0 != 1 { | ||
| cmd.env("RUSTC_THREAD_COUNT", self.parallel.0.to_string()); |
There was a problem hiding this comment.
What is this environment variable? Shouldn't we be using -Zthreads?
| pub struct Parallel(pub u32); | ||
|
|
||
| impl Parallel { | ||
| pub fn par_n(self) -> String { |
| format!("par{}", self.0) | ||
| } | ||
| // Default parallel frontend options | ||
| pub fn default_opts() -> Vec<Parallel> { |
There was a problem hiding this comment.
The default for now should just be 1 thread.
-Zthreads=N support implemented as an additional dimension 'Parallel'.
Default values for parallels:
[1,4].Aggregations sample:

Primary benchmarks sample:

Secondary benchmarks sample:

Graphs sample (graphs have "scenario_parallel" names like "full_par1 / full_par4"):

Filters with par1/par4 added:
