-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
bootstrap: when we detect travis-ci or appveyor, use 16 codegen-units for stage0 #48843
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
3 is too low, use 8 or 16. |
Are you sure about that? |
We have 4 cores in Travis, and also that having too few codegen units may caused unbalanced load. The parallel codegen follows jobserver, which means the number of thread will not exceed the core count. |
c71c8b1
to
696ec5c
Compare
Alright, bumped to 16. |
696ec5c
to
55446e5
Compare
Thanks for the PR! I think we already enable 16 cgus + thinlto on appveyor/travis, though, so are we sure that this has an effect? |
@alexcrichton see the logs @ https://api.travis-ci.org/v3/job/350827435/log.txt
|
Oh I think that's specifically related to the llvm-3.9 builder there because it's not the "Rust LLVM" which means that ThinLTO isn't available so we build with one cgu. I believe other builders should already be using multiple cgus by default (with ThinLTO). Given that the llvm-3.9 builder is already relatively speedy (~1.5hrs) are we sure that this speeds up the builder as well? In theory more cgus means less optimizations, so this could run the risk of slowing down the run-pass test suite, for example. |
I think we should be able to see how much faster stage0 build got and now much slower stage1 build got once the CI job is done building. |
The build clocked in at 1h25m which is about the same for the other PR builds I believe |
I'm taking the build of petrochenkov@e5b86ea // https://travis-ci.org/rust-lang/rust/builds/350827835?utm_source=github_status&utm_medium=notification logs of this pr: https://travis-ci.org/rust-lang/rust/builds/350884124?utm_source=github_status&utm_medium=notification
It's a bit strange that stage1 improved as well though _ pr |
(cc #45444.) |
Note that builds on Travis have very variable performance so any two runs of the exact same code can have pretty large variations in compile times. The difference here between stage0 compiler artifacts is likely significant (as expected), but all other timings are probably "lost in the noise" in terms of the differences. That means that there's probably a small gain to get on the stage0 compiler here, but it at least doesn't significantly regress the stage1 compiler! |
Ah that's ok the major difference between stage0/stage1 with this pr (700 -> 100s) seems pretty clear to me that it'd be caused by switching from 16 cgus to one cgu (way less usage of the 4 cores we've got on Travis) If you'd like I'd be ok landing this, but given that this only affects the one Travis builder with LLVM 3.9 I'd prefer to have a configuration option for rustbuild which is set by that one builder. |
Hmm, well, my original plans were to speed up the auto branch as well.... |
Not on dist builders: https://github.com/rust-lang/rust/blob/master/src/ci/run.sh#L49 So this could have a fairly major effect there, though probably not too much. |
@Mark-Simulacrum ah yes indeed! I forgot we hadn't gotten around to turning that back off yet, I'll send a PR. |
@matthiaskrgr would you like to land this PR for the llvm-3.9 builder on Travis? |
Well, what are the options? Of course, speeding up the pull request builder a bit would already be a win so I'm not against that. |
Yeah llvm-3.9 is only used for PRs (but is also on full auto builds). I actually thought we already enabled multiple cgus for all jobs on the auto branch but turns out this was only true for the builders that run tests, not the dist builders. I sent a PR to clean up some handling in this area and switch the default for the dist builders back to multiple cgus. |
☔ The latest upstream changes (presumably #49051) made this pull request unmergeable. Please resolve the merge conflicts. |
c4d9081
to
a595c40
Compare
Please don't format the code since that should be done all at once and in a separate PR. |
… when building stage0.
ecb4dfb
to
e87abdb
Compare
Ping from triage @alexcrichton! The author pushed new commits, could you review them? |
Ah yeah as mentioned previously now that the PR has landed I think that we're actually using multiple CGUs on all builds, including the llvm-3.9 build (where at runtime we detect ThinLTO isn't available). In light of that I'm going to close this as I don't think it's necessary any more |
No description provided.