rustc: Support --jobs options for limiting parallelism in various parts of the compiler - #159675
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
| /// `None` means everything is single-threaded and synchronization can be disabled. | ||
| #[derive(Clone, Copy)] | ||
| pub struct Jobs { | ||
| pub frontend: Option<NonZero<u8>>, |
There was a problem hiding this comment.
| pub frontend: Option<NonZero<u8>>, | |
| pub frontend: Option<NonZero<usize>>, |
Probably can convert to usize after the range validation in parse_jobs_one is done, all the places that want this value accept usize.
There was a problem hiding this comment.
usize is used here now.
| check_upper_limit(backend, opt_name); | ||
| if zno_parallel_backend { | ||
| early_dcx | ||
| .early_fatal("cannot use both `--jobs-backend` and `-Zno-parallel-backend`"); |
There was a problem hiding this comment.
I think we can remove -Zno-parallel-backend right away in this PR for simplicity, I don't expect it to be used as much as -Zthreads.
There was a problem hiding this comment.
-Zno-parallel-backend is removed now (or rather always reports an error saying to use --jobs-backend=1).
| compiler.arg(input_file); | ||
|
|
||
| // Use a single thread for efficiency and a deterministic error message order | ||
| compiler.arg("-Zthreads=1"); |
There was a problem hiding this comment.
This is already the default, so it's not necessary (and it caused some option conflicts).
There was a problem hiding this comment.
It isn't the default when setting the rust.parallel-frontend-threads bootstrap config to a different value, right?
There was a problem hiding this comment.
I don't see any logic turning bootstrap's rust.parallel-frontend-threads into -Zthreads for tests.
Compiletest's own --parallel-frontend-threads flag adds -Zthreads to UI tests, but it's not enabled by default.
There was a problem hiding this comment.
Looks like one of the CI jobs does pass that flag:
This is an optional CI job, so if you break it, the merge would still work. Still it would be best to not break it unnecessarily.There was a problem hiding this comment.
Due to things like this I'll probably have to change the --jobs options from Opt to Multi right away.
-Zthreads=1 -Zthreads=2 works right now, but --jobs 1 --jobs 2 does not, as currently implemented.
Cargo also prohibits it - error: the argument '--jobs <N>' cannot be used multiple times, so I initially wanted to reproduce this more conservative behavior.
There was a problem hiding this comment.
This is an optional CI job, so if you break it, the merge would still work.
FWIW, making it non-optional is my next immediate task.
There was a problem hiding this comment.
I've added //@ ignore-parallel-frontend to a couple of tests causing option conflicts, test runs with --parallel-frontend-threads pass now.
This comment has been minimized.
This comment has been minimized.
| true => None, | ||
| false => match jobs { | ||
| Some(n) => n, | ||
| None => NonZero::new(32), // back compat with historical behavior |
There was a problem hiding this comment.
I don't think there was any cap when using a jobserver previously. The 32 tokens default is when there is no jobserver. And in any case I don't think the "The fixed number is used to have deterministic compilation across machines." comment is actually correct. Changing the max number of parallel threads doesn't affect determinism of the output at all. Only the amount of codegen units.
There was a problem hiding this comment.
I don't think there was any cap when using a jobserver previously.
Yes, if there's an external jobserver, then there's no static limit on main, only the dynamic limit from that jobserver.
But in that case --jobs-backend is not currently respected anyway (see the FIXME in write.rs).
I can keep the default "unlimited" behavior when addressing the FIXME (in a separate later PR).
There was a problem hiding this comment.
And in any case I don't think the "The fixed number is used to have deterministic compilation across machines." comment is actually correct.
Yeah, that comment is removed now.
There was a problem hiding this comment.
Added a variant to preserve the historical behavior precisely, even when the FIXME in write.rs is addressed.
| Opt, | ||
| "", | ||
| "jobs-backend", | ||
| "Limit on the number of parallel jobs used by backend", |
There was a problem hiding this comment.
Is this supposed to be respected when a jobserver exists?
There was a problem hiding this comment.
Yes, eventually (right now there's a FIXME about it in write.rs).
jobs_backend is the static upper limit, and the jobserver can limit work further within the 1 ..= jobs_backend range.
|
Why is I don't like the |
I expect a large number of users of
Better naming suggestions are welcome. |
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
All the comments are addressed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
…arts of the compiler
|
Implementation looks fine. And based on the MCP being accepted, I'm going to assume the CLI is fine too. Can still be adjusted before stabilization if necessary. @bors r+ |
rustc: Support `--jobs` options for limiting parallelism in various parts of the compiler `--jobs-frontend` for the frontend, `--jobs-backend` for the backend, `--jobs-linker` for the linker, and `-j`/`--jobs` for the overall limit. See the added docs for more details. Part of rust-lang/compiler-team#1005. Any suspicious option combinations are prohibited for now. r? @bjorn3 @Zoxc
…uwer Rollup of 21 pull requests Successful merges: - #160100 (Add "system" option to `override-allocator` directive) - #159675 (rustc: Support `--jobs` options for limiting parallelism in various parts of the compiler) - #159999 (Fix invalidation of stdlib in bootstrap when using non-LLVM codegen backends) - #160233 (Bubble bad path error while parsing field to avoid unecessary second error) - #160272 (rustc_metadata: Move native library search code to `rustc_codegen_ssa`) - #154202 (rustfmt: Format `cfg_select!`) - #158835 (rustc_passes: lint unused `#[path]` attributes on inline modules) - #159520 (Suggest `Vec<T>` instead of `[T]`) - #160034 (Move "macro only" check for `#[allow_internal_unsafe/unstable]` to attribute parser) - #160066 (rustc_middle: lint attribute cleanups) - #160085 (Remove various superfluous lint attributes) - #160113 (Coalesce `rustc_on_unimplemented` attributes and lint malformed filters) - #160119 (fix query cycle in `coroutine_hidden_types` for the next solver) - #160147 (tests: Remove `-Zthreads` options from tests in `ui/parallel-rustc`) - #160157 (Remove outdated comments from `va_list.rs`) - #160159 (More accurately check for interior mutability in `invalid_reference_casting` lint) - #160208 (rustdoc: Fix crash when trying to list attributes on an opaque type) - #160244 (Rename splat to rustc_splat in error messages) - #160246 (dont fire `unused_mut` on `&pin mut self`) - #160247 (Configure backport nominations for rustfmt) - #160274 (renovate: update lock files weekly)
…uwer Rollup of 22 pull requests Successful merges: - #160100 (Add "system" option to `override-allocator` directive) - #160220 (Refactor: shrink region ext traits) - #159675 (rustc: Support `--jobs` options for limiting parallelism in various parts of the compiler) - #159999 (Fix invalidation of stdlib in bootstrap when using non-LLVM codegen backends) - #160233 (Bubble bad path error while parsing field to avoid unecessary second error) - #160272 (rustc_metadata: Move native library search code to `rustc_codegen_ssa`) - #154202 (rustfmt: Format `cfg_select!`) - #159520 (Suggest `Vec<T>` instead of `[T]`) - #159710 (Add rustdoc/cargo PGO profiles to reproducible artifacts) - #160034 (Move "macro only" check for `#[allow_internal_unsafe/unstable]` to attribute parser) - #160066 (rustc_middle: lint attribute cleanups) - #160085 (Remove various superfluous lint attributes) - #160113 (Coalesce `rustc_on_unimplemented` attributes and lint malformed filters) - #160119 (fix query cycle in `coroutine_hidden_types` for the next solver) - #160147 (tests: Remove `-Zthreads` options from tests in `ui/parallel-rustc`) - #160157 (Remove outdated comments from `va_list.rs`) - #160159 (More accurately check for interior mutability in `invalid_reference_casting` lint) - #160208 (rustdoc: Fix crash when trying to list attributes on an opaque type) - #160244 (Rename splat to rustc_splat in error messages) - #160246 (dont fire `unused_mut` on `&pin mut self`) - #160247 (Configure backport nominations for rustfmt) - #160274 (renovate: update lock files weekly)
Rollup merge of #159675 - petrochenkov:jjj, r=bjorn3 rustc: Support `--jobs` options for limiting parallelism in various parts of the compiler `--jobs-frontend` for the frontend, `--jobs-backend` for the backend, `--jobs-linker` for the linker, and `-j`/`--jobs` for the overall limit. See the added docs for more details. Part of rust-lang/compiler-team#1005. Any suspicious option combinations are prohibited for now. r? @bjorn3 @Zoxc
View all comments
--jobs-frontendfor the frontend,--jobs-backendfor the backend,--jobs-linkerfor the linker, and-j/--jobsfor the overall limit.See the added docs for more details.
Part of rust-lang/compiler-team#1005.
Any suspicious option combinations are prohibited for now.
r? @bjorn3 @Zoxc