Skip to content

Compile some CGUs in parallel at the start of codegen #67889

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 11, 2020
Merged

Compile some CGUs in parallel at the start of codegen #67889

merged 2 commits into from
Jan 11, 2020

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 5, 2020

This brings the compilation time for syntex_syntax from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling n CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for syntex_syntax.

Based on #67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2020
michaelwoerister
michaelwoerister reviewed Jan 6, 2020
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with nits addressed.

I hope we really can get rid of OnGoingCodegen entirely at some point.

@michaelwoerister
Copy link
Member

Heads up, @bjorn3: This PR changes the codegen backend protocol.

@bjorn3
Copy link
Member

bjorn3 commented Jan 6, 2020

Thanks for the ping @michaelwoerister! This is in a part which I don't yet use though. Because cranelift-module compiles a function as soon as you define it, isn't possible for me to use multiple threads until there are parallel rustc builds for every nightly.

@bors
Copy link
Collaborator

bors commented Jan 6, 2020

☔ The latest upstream changes (presumably #67886) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

This looks like a nice small improvement!

In the limit I'm looking forward to when we can fully reap the benefits here, including:

  • Removal of the coordinator thread for LLVM CGU management
  • Removal of cost approximation to know when to CGU and when to optimize
  • Straight-line code to express "codegen and optimize everything" all in one function that's much easier to read than the coordinator thread

I don't think we can get here until we remove the single-threaded compiler completely though. While this is adding complexity to an already complex backend it's not too too bad and has some modest wins. I think the biggest wins though are going to come when we're able to remove the coordinator thread entirely.

@michaelwoerister
Copy link
Member

Thanks, @Zoxc.

@bors r+

I'm r+ing under the assumption that the job server questions is resolved to @Mark-Simulacrum's and @alexcrichton's satisfaction. I don't know the details there.

@bors
Copy link
Collaborator

bors commented Jan 9, 2020

📌 Commit e3a36c0d78c69ea59a6bde4bae0729dc3545a7b4 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2020
@bors
Copy link
Collaborator

bors commented Jan 9, 2020

☔ The latest upstream changes (presumably #67988) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 9, 2020
@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 9, 2020

@bors r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Jan 9, 2020

📌 Commit 69bacd0 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2020
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
@Centril
Rollup merge of rust-lang#67889 - Zoxc:parallel-cgus, r=michaelwoerister
2f0527c 
Compile some CGUs in parallel at the start of codegen

This brings the compilation time for `syntex_syntax` from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling `n` CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for `syntex_syntax`.

Based on rust-lang#67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum
@Centril Centril mentioned this pull request Jan 11, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
@Centril
Rollup merge of rust-lang#67889 - Zoxc:parallel-cgus, r=michaelwoerister
72bf6f4 
Compile some CGUs in parallel at the start of codegen

This brings the compilation time for `syntex_syntax` from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling `n` CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for `syntex_syntax`.

Based on rust-lang#67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum
@Centril Centril mentioned this pull request Jan 11, 2020
Closed
bors added a commit that referenced this pull request Jan 11, 2020
@bors
Auto merge of #68124 - Centril:rollup-4h2ybcj, r=Centril
13ad02f 
Rollup of 9 pull requests

Successful merges:

 - #67000 (Promote references to constants instead of statics)
 - #67756 (Collector tweaks)
 - #67889 (Compile some CGUs in parallel at the start of codegen)
 - #67930 (Rename Result::as_deref_ok to as_deref)
 - #68018 (feature_gate: Remove `GateStrength`)
 - #68070 (clean up E0185 explanation)
 - #68072 (Fix ICE #68058)
 - #68114 (Don't require `allow_internal_unstable` unless `staged_api` is enabled.)
 - #68120 (Ban `...X` pats, harden tests, and improve diagnostics)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
@Centril
Rollup merge of rust-lang#67889 - Zoxc:parallel-cgus, r=michaelwoerister
7b741fb 
Compile some CGUs in parallel at the start of codegen

This brings the compilation time for `syntex_syntax` from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling `n` CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for `syntex_syntax`.

Based on rust-lang#67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum
@Centril Centril mentioned this pull request Jan 11, 2020
Merged
bors added a commit that referenced this pull request Jan 11, 2020
@bors
Auto merge of #68126 - Centril:rollup-cz5u7xx, r=Centril
bfd0487 
Rollup of 8 pull requests

Successful merges:

 - #67756 (Collector tweaks)
 - #67889 (Compile some CGUs in parallel at the start of codegen)
 - #67930 (Rename Result::as_deref_ok to as_deref)
 - #68018 (feature_gate: Remove `GateStrength`)
 - #68070 (clean up E0185 explanation)
 - #68072 (Fix ICE #68058)
 - #68114 (Don't require `allow_internal_unstable` unless `staged_api` is enabled.)
 - #68120 (Ban `...X` pats, harden tests, and improve diagnostics)

Failed merges:

r? @ghost
@bors bors merged commit 69bacd0 into rust-lang:master Jan 11, 2020
@Zoxc Zoxc deleted the parallel-cgus branch January 11, 2020 19:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Zoxc @michaelwoerister @bjorn3 @bors @alexcrichton @rust-highfive