ci: Use multiple codegen units on non-dist bots by alexcrichton · Pull Request #44675 · rust-lang/rust

alexcrichton · 2017-09-18T15:11:21Z

This commit is yet another attempt to bring down our cycle times by
parallelizing some of the long-and-serial parts of the build, for example
optimizing the libsyntax, librustc, and librustc_driver crate. The hope is that
any perf loss from codegen units is more than made up for with the perf gain
from using multiple codegen units.

The value of 16 codegen units here is pretty arbitrary, it's basically just a
number which hopefully means that the cores are always nice and warm.

rust-highfive · 2017-09-18T15:11:26Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2017-09-18T15:13:02Z

r? @aidanhs

aidanhs · 2017-09-18T15:14:59Z

@bors r+ p=1

bors · 2017-09-18T15:14:59Z

📌 Commit 1f9b02b has been approved by aidanhs

ci: Use multiple codegen units on non-dist bots This commit is yet another attempt to bring down our cycle times by parallelizing some of the long-and-serial parts of the build, for example optimizing the libsyntax, librustc, and librustc_driver crate. The hope is that any perf loss from codegen units is more than made up for with the perf gain from using multiple codegen units. The value of 16 codegen units here is pretty arbitrary, it's basically just a number which hopefully means that the cores are always nice and warm.

Rollup of 10 pull requests - Successful merges: #44364, #44466, #44537, #44640, #44651, #44657, #44661, #44668, #44671, #44675 - Failed merges:

Mark-Simulacrum · 2017-09-18T17:09:26Z

@bors r-

There's only 2 cores usually though at least on Travis, so this would seem like too many? I'm not too sure how that works out in practice though...

Rollup of 11 pull requests - Successful merges: #44364, #44466, #44537, #44548, #44640, #44651, #44657, #44661, #44668, #44671, #44675 - Failed merges:

mattico · 2017-09-18T17:46:25Z

This isn't definitive data, but it's worth noticing that this build took about 10 minutes longer than every other recent PR.

aidanhs · 2017-09-18T18:08:02Z

After a brief look, it looks like it might be making compilation of rustc itself a touch faster, but building+running the tests a bunch slower. At minimum it'd be good to see the effect on full builds.

alexcrichton · 2017-09-18T18:28:22Z

This PR is the likely cause of https://ci.appveyor.com/project/rust-lang/rust/build/1.0.4720, so I'll look into that when I get a chance.

@Mark-Simulacrum note that the number of cores and the number of codegen units don't tend to have a correlation on one another. With the support nowadays we'll never run more than 2 jobs in parallel, and trans will even attempt to finish codegen on some translation units before it moves on to even start translating the next unit.

I selected 16 here knowing that Travis only has 2 cores to ensure that we always keep the builders hot. Otherwise if we generate 2 codegen units one of them could finish codegen instantly while the other would spend the whole time translating, not actually getting us any benefit. The hope here is that with enough codegen units we can keep the cpus nice and busy and make sure that one's not idle for too much longer while the last cgu is finishing.

@mattico thanks for looking I'll probably turn this down to like 4 or 6 to have a minimal impact for now on compile times. Most of the time this just means there's missing #[inline] annotations.

Mark-Simulacrum · 2017-09-18T23:13:43Z

It may be worth compiling the core crates and tools (libstd, libtest, librustc) with codegen units set to ~16 and then compile tests, which generally compile very fast, without codegen units, since I imagine the added parallelism there may be hurting us.

Feel free to re-r+ whenever (from my comment, at least). I didn't know that we never run more than two codegen units at the same time -- that seems like it would hurt people with more powerful computers (3 or more CPU cores).

michaelwoerister · 2017-09-19T09:55:19Z

Another thing to note is that rustc recently gained the ability to start LLVM already after the first CGU has been translated. So having more CGUs than cpu-cores potentially does make sense, since the second thread can start working earlier.

aidanhs · 2017-09-19T10:21:39Z

@Mark-Simulacrum

I didn't know that we never run more than two codegen units at the same time -- that seems like it would hurt people with more powerful computers (3 or more CPU cores).

I don't think that's what @alexcrichton is saying. Instead, I believe the idea is that compilation is split into N bits of work, and then those bits of work are executed with as many units of parallelism as you have CPU cores. The problem is that you don't know if one of the bits of work is going to take 10x longer all the others, but if you do and you've just split into 2 units then you have one core idly for ages. By splitting into more you're reducing the probability of hitting this worst case (in theory).

alexcrichton · 2017-09-19T14:50:13Z

@bors: r=aidanhs p=0

Hm ok I can't reproduce this locally, so I'm going to try to see the error on AppVeyor again. Also reduced to 8 codegen units.

@Mark-Simulacrum ah yes @aidanhs is right, the intention here is to reduce the chance of having one core running lots of work and one empty with work.

bors · 2017-09-19T14:50:14Z

📌 Commit d670a77 has been approved by aidanhs

alexcrichton · 2017-09-19T14:50:28Z

Also I believe the codegen-units setting here only applie to rustc/std, not any tests.

michaelwoerister · 2017-09-20T09:39:15Z

Could we get more CPU cores on the machines that run large test suites? Running tests scales pretty well with the number of cores.

alexcrichton · 2017-09-20T16:10:47Z

@michaelwoerister I wish! Unfortunately though it's not so easy :(

michaelwoerister · 2017-09-20T16:24:28Z

Try adding echo -f "16" > /dev/cpu/num-cpus to your docker image. Don't forget the -f. That one is needed in order to work around travis's licensing model and the laws of physics.

bors · 2017-09-21T12:10:06Z

⌛ Testing commit d670a7775309e9c0233907b426bd76e2c356004b with merge 6e7b5cd731ae1549bf0c52fa71af137046a5ec42...

alexcrichton · 2017-09-21T21:03:10Z

@bors: r=aidanhs

bors · 2017-09-21T21:03:11Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: don't suggest placing use statements into expanded code #44215

bors · 2017-09-21T21:03:12Z

📌 Commit d670a77 has been approved by aidanhs

alexcrichton · 2017-09-21T21:03:29Z

@bors: r=aidanhs

bors · 2017-09-21T21:03:30Z

📌 Commit e157893 has been approved by aidanhs

bors · 2017-09-21T23:40:34Z

⌛ Testing commit e157893859643e6fdecb21a9254f735bb2f4d926 with merge a47067e228c2ce4bcf5197be5f8fada509718c08...

bors · 2017-09-21T23:44:08Z

💔 Test failed - status-appveyor

This commit is yet another attempt to bring down our cycle times by parallelizing some of the long-and-serial parts of the build, for example optimizing the libsyntax, librustc, and librustc_driver crate. The hope is that any perf loss from codegen units is more than made up for with the perf gain from using multiple codegen units. The value of 8 codegen units here is pretty arbitrary, it's basically just a number which hopefully means that the cores are always nice and warm. Also a previous version of this commit bounced on Windows CI due to libstd being compiled with multiple codegen units, so only the compiler is now compiled with multiple codegen units.

alexcrichton · 2017-09-21T23:54:25Z

@bors: r=aidanhs

bors · 2017-09-21T23:54:26Z

📌 Commit 27c26da has been approved by aidanhs

aidanhs · 2017-09-22T00:07:47Z

+ let cgus = if mode == Mode::Libstd {
+ self.config.rust_codegen_units
+ } else {
+ self.config.rustc_codegen_units.unwrap_or(self.config.rust_codegen_units)


Could we make rustc_codegen_units do what it says on the tin and just apply to rustc (rather than it being an 'all-but-std' flag)? That seems to be where the majority of time goes anyway when I build the compiler.

Sure yeah. This I expect to be one of those "obscure options that no one ever touches", but I can change it after this PR lands or if it bounces.

bors · 2017-09-22T02:01:19Z

⌛ Testing commit 27c26da with merge f7b98020a75f8e6701715473ffd3896bcc35a6fd...

bors · 2017-09-22T05:01:39Z

💔 Test failed - status-travis

kennytm · 2017-09-22T06:39:33Z

@bors retry

The two macs 3-hour timed out. Not sure if related to the current Travis incident.

It may also mean using multiple CGUs may slow down on the mac CIs though.

bors · 2017-09-22T13:10:54Z

⌛ Testing commit 27c26da with merge 2075597...

ci: Use multiple codegen units on non-dist bots This commit is yet another attempt to bring down our cycle times by parallelizing some of the long-and-serial parts of the build, for example optimizing the libsyntax, librustc, and librustc_driver crate. The hope is that any perf loss from codegen units is more than made up for with the perf gain from using multiple codegen units. The value of 16 codegen units here is pretty arbitrary, it's basically just a number which hopefully means that the cores are always nice and warm.

alexcrichton · 2017-09-22T14:05:12Z

@bors r- retry

This caused rebuilds during testing I think that I want to investigate

alexcrichton · 2017-09-22T23:19:02Z

Ok looking at the logs this is not clearly a win, the bootstrap was faster and the tests took way slower. Now I don't really expect predictable performance out of the OSX builders, but at least for now it seems like this is not the win we're hoping for.

michaelwoerister · 2017-09-25T08:18:31Z

It was worth a try.

rust-highfive assigned aturon Sep 18, 2017

rust-highfive assigned aidanhs and unassigned aturon Sep 18, 2017

alexcrichton mentioned this pull request Sep 18, 2017
Rollup of 10 pull requests #44676
Closed

bors added a commit that referenced this pull request Sep 18, 2017
Auto merge of#44676- alexcrichton:rollup, r=alexcrichton
4a8bc8a
Rollup of 10 pull requests - Successful merges: #44364, #44466, #44537, #44640, #44651, #44657, #44661, #44668, #44671, #44675 - Failed merges:

carols10cents added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 18, 2017

alexcrichton mentioned this pull request Sep 18, 2017
Rollup of 11 pull requests #44678
Merged

bors added a commit that referenced this pull request Sep 18, 2017
Auto merge of#44678- alexcrichton:rollup, r=alexcrichton
0701b37
Rollup of 11 pull requests - Successful merges: #44364, #44466, #44537, #44548, #44640, #44651, #44657, #44661, #44668, #44671, #44675 - Failed merges:

alexcrichton force-pushed the many-cgu branch from 1f9b02b to d670a77Compare September 19, 2017 14:49

alexcrichton force-pushed the many-cgu branch from d670a77 to e157893Compare September 21, 2017 21:03

alexcrichton force-pushed the many-cgu branch from e157893 to 27c26daCompare September 21, 2017 23:54

aidanhs reviewed Sep 22, 2017
View reviewed changes

alexcrichton closed this Sep 22, 2017

alexcrichton deleted the many-cgu branch September 23, 2017 04:43

alexcrichton mentioned this pull request Oct 20, 2017
rustbuild: Compile rustc with ThinLTO #45400
Merged

This was referenced Jan 20, 2018
Parallelize rustc via multi-process approach #47518
Closed
Tracking issue: crater is slow rust-lang/crater#136
Open
[partitioning] Don't create CGUs below a certain size #47318
Closed

aidanhs mentioned this pull request Feb 3, 2018
compiletest: Default to one CGU when compiling tests. #47779
Closed

Uh oh!

Uh oh!

Conversation

alexcrichton commented Sep 18, 2017

Uh oh!

rust-highfive commented Sep 18, 2017

Uh oh!

alexcrichton commented Sep 18, 2017

Uh oh!

aidanhs commented Sep 18, 2017

Uh oh!

bors commented Sep 18, 2017

Uh oh!

Mark-Simulacrum commented Sep 18, 2017

Uh oh!

mattico commented Sep 18, 2017

Uh oh!

aidanhs commented Sep 18, 2017

Uh oh!

alexcrichton commented Sep 18, 2017

Uh oh!

Mark-Simulacrum commented Sep 18, 2017

Uh oh!

michaelwoerister commented Sep 19, 2017

Uh oh!

aidanhs commented Sep 19, 2017

Uh oh!

alexcrichton commented Sep 19, 2017

Uh oh!

bors commented Sep 19, 2017

Uh oh!

alexcrichton commented Sep 19, 2017

Uh oh!

michaelwoerister commented Sep 20, 2017

Uh oh!

alexcrichton commented Sep 20, 2017

Uh oh!

michaelwoerister commented Sep 20, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

alexcrichton commented Sep 21, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

alexcrichton commented Sep 21, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

alexcrichton commented Sep 21, 2017

Uh oh!

bors commented Sep 21, 2017

Uh oh!

aidanhsSep 22, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichtonSep 22, 2017

Choose a reason for hiding this comment

Uh oh!

bors commented Sep 22, 2017

Uh oh!

bors commented Sep 22, 2017

Uh oh!

kennytm commented Sep 22, 2017

Uh oh!

bors commented Sep 22, 2017

Uh oh!

alexcrichton commented Sep 22, 2017

Uh oh!

alexcrichton commented Sep 22, 2017

Uh oh!

michaelwoerister commented Sep 25, 2017

Uh oh!

Reviewers