Skip to content
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

Make all alt builders produce parallel-enabled compilers #64722

Merged
merged 2 commits into from Oct 2, 2019

Conversation

@Mark-Simulacrum
Copy link
Member

commented Sep 23, 2019

We're not quite ready to ship parallel compilers by default, but the alt
builders are not used too much (in theory), so we believe that shipping
a possibly-broken compiler there is not too problematic.

r? @nikomatsakis

We're not quite ready to ship parallel compilers by default, but the alt
builders are not used too much (in theory), so we believe that shipping
a possibly-broken compiler there is not too problematic.
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2019

Going to nominate for infra and compiler teams as I'd like to check that the claims I've made are true:

  • usage is minimal, mostly to deal with LLVM-related regressions (-Zthreads=1 is probably enough to sidestep any bugs in parallel that could impact this)
  • we are comfortable altering them in such a "big" way.
@pnkfelix

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

Discussed in T-compiler meeting. We ourselves don't see any problem with doing this as described here.

In particular, @nikomatsakis wanted to stress that we do plan to keep LLVM assertions enabled for these builds. This will mean that these builds help flush out parallel bugs, but they will not be a good basis for comparative benchmarking.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

(leaving nominated tag on, but at this point that is directed at T-infra, not T-compiler.)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

r? @alexcrichton

I believe we discussed this in the infra meeting and there weren't any major objections -- could you just make sure this is doing what it's intended and such and approve if you think we're good to go here?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

Would it be possible to configure these compilers to be built with -j1 for everything except the codegen backend? I'm basically curious if we can just enable parallelism but not use it.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

I think that's "just" -Zthreads=1 at runtime but I can take a look to both verify that theory and, if so, hard code that as the default (perhaps via a cfg flag or something).

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

Ah ok, great! In that case actually, I think it'd be best to default to -Zthreads=1 by default? Basically leave everything as-is, but just producing "parallel capable" compilers here.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

Alright, I pushed up another commit that makes the default for query parallelism 1, instead of num_cpus as before. This does not affect the codegen parallelism.

@@ -1259,7 +1259,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"prints the LLVM optimization passes being run"),
ast_json: bool = (false, parse_bool, [UNTRACKED],
"print the AST as JSON and halt"),
threads: Option<usize> = (None, parse_opt_uint, [UNTRACKED],
threads: Option<usize> = (Some(1), parse_opt_uint, [UNTRACKED],

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 30, 2019

Member

It seems a bit odd to have this be Option<usize> when the default isn't None, could this instead change threads_from_count, and a comment could be included why it's defaulted to one?

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Sep 30, 2019

Author Member

Sure, yeah.

@Mark-Simulacrum Mark-Simulacrum force-pushed the Mark-Simulacrum:alt-parallel branch from 550df4d to f271c15 Sep 30, 2019
This changes the default parallelism for parallel compilers to one,
instead of the previous default, which was "num cpus". This is likely
not an optimal default long-term, but it is a good default for testing
whether parallel compilers are not a significant regression over a
sequential compiler.

Notably, this in theory makes a parallel-enabled compiler behave
exactly like a sequential compiler with respect to the jobserver.
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

📌 Commit f271c15 has been approved by alexcrichton

@Mark-Simulacrum Mark-Simulacrum force-pushed the Mark-Simulacrum:alt-parallel branch from f271c15 to 1a1067d Sep 30, 2019
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

Was a bit overeager pushing, there's a new commit up (no significant differences)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

📌 Commit 1a1067d has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
…xcrichton

Make all alt builders produce parallel-enabled compilers

We're not quite ready to ship parallel compilers by default, but the alt
builders are not used too much (in theory), so we believe that shipping
a possibly-broken compiler there is not too problematic.

r? @nikomatsakis
@Centril Centril referenced this pull request Oct 1, 2019
bors added a commit that referenced this pull request Oct 1, 2019
Rollup of 5 pull requests

Successful merges:

 - #64404 (Add long error explanation for E0495)
 - #64722 (Make all alt builders produce parallel-enabled compilers)
 - #64840 (SelfProfiler API refactoring and part one of event review)
 - #64890 ([const-prop] Handle remaining MIR Rvalue cases)
 - #64910 (syntax: cleanup param, method, and misc parsing)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
…xcrichton

Make all alt builders produce parallel-enabled compilers

We're not quite ready to ship parallel compilers by default, but the alt
builders are not used too much (in theory), so we believe that shipping
a possibly-broken compiler there is not too problematic.

r? @nikomatsakis
@tmandry tmandry referenced this pull request Oct 1, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
…xcrichton

Make all alt builders produce parallel-enabled compilers

We're not quite ready to ship parallel compilers by default, but the alt
builders are not used too much (in theory), so we believe that shipping
a possibly-broken compiler there is not too problematic.

r? @nikomatsakis
@tmandry tmandry referenced this pull request Oct 1, 2019
bors added a commit that referenced this pull request Oct 1, 2019
Rollup of 8 pull requests

Successful merges:

 - #63416 (apfloat: improve doc comments)
 - #64722 (Make all alt builders produce parallel-enabled compilers)
 - #64820 (BTreeSet intersection, is_subset & difference optimizations)
 - #64840 (SelfProfiler API refactoring and part one of event review)
 - #64910 (syntax: cleanup param, method, and misc parsing)
 - #64912 (Remove unneeded `fn main` blocks from docs)
 - #64933 (Fixes #64919. Suggest fix based on operator precendence.)
 - #64952 (Update cargo.)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Oct 2, 2019
…xcrichton

Make all alt builders produce parallel-enabled compilers

We're not quite ready to ship parallel compilers by default, but the alt
builders are not used too much (in theory), so we believe that shipping
a possibly-broken compiler there is not too problematic.

r? @nikomatsakis
bors added a commit that referenced this pull request Oct 2, 2019
Rollup of 11 pull requests

Successful merges:

 - #64649 (Avoid ICE on return outside of fn with literal array)
 - #64722 (Make all alt builders produce parallel-enabled compilers)
 - #64801 (Avoid `chain()` in `find_constraint_paths_between_regions()`.)
 - #64805 (Still more `ObligationForest` improvements.)
 - #64840 (SelfProfiler API refactoring and part one of event review)
 - #64885 (use try_fold instead of try_for_each to reduce compile time)
 - #64942 (Fix clippy warnings)
 - #64952 (Update cargo.)
 - #64974 (Fix zebra-striping in generic dataflow visualization)
 - #64978 (Fully clear `HandlerInner` in `Handler::reset_err_count`)
 - #64979 (Update books)

Failed merges:

 - #64959 (syntax: improve parameter without type suggestions)

r? @ghost
@bors bors merged commit 1a1067d into rust-lang:master Oct 2, 2019
4 checks passed
4 checks passed
pr Build #20190930.68 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.