-
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
compiletest: Default to one CGU when compiling tests. #47779
Conversation
So I've compare the timing of this PR against the master branch and two other unrelated PRs. It doesn't seem that there's any advantage on Travis CI's docker, in fact it becomes slower. Not sure about the performance on Windows and macOS. Raw dataTable obtained by running
Normalized against total time taken for `make all`
|
Ping from triage @aidanhs! Do we want to try this? |
Yeah I think so. But - isn't codegen-units=1 the default for |
I had lost track of what codegen-units is currently doing. After reading #45444 and #47834, I now see that codegen units are enabled for everything except beta/stable dist builds. I'll try this locally in the next couple of weeks to see if I observe the similar speedups, in which case we can take a more careful look at why it slows travis down. |
This PR just leads to test being compiled with one codegen unit. The compiler is not affected. If this doesn't make much of a difference, it's probably better to close this PR, since it will run tests in a non-default compiler mode. |
@michaelwoerister what was your test platform where you observed the time go from 8m30s to 7m05s? (the data that says "this makes no difference" was from linux, but that's not the primary build platform that we need to speed up anyway, is it?) |
I tested on Ubuntu 16.04 with a 4C/8T CPU. |
Ping from triage @aidanhs :) Did you have a chance to take a look why builds on travis are slowed down? |
I'm sorry, I didn't - give me another couple of days to give it a try :) |
I tried this locally via
Definitely seems to make a difference. I had a thought though - codegen-units allows rustc itself to parallelise, and rust tests are run in parallel. Doesn't this mean that we're parallelising squared? Watching the load avg on my machine seems to confirm this - it hovers around 8 with this PR enabled (i.e. with codegen-units=1), but becomes at least double without it. Will do some more digging. |
Yes, that's one of the two main opportunities for saving time here. Splitting a program into multiple CGUs introduces some overhead but because we are already running so many compiler instances in parallel that additional overhead won't amortize itself. The second reason one CGU might be faster for small programs (at least with However, I'm not sure how much we should change settings here from a test coverage perspective. Multiple CGUs and ThinLTO are the current defaults so I'm wary of running the test suite in a different mode. |
Sounds like this is a valid usecase for #48532, but well disabling multiple CGUs works either. |
Ping from triage, @aidanhs ! |
Gonna reassign to... r? @kennytm |
@bors r+ Let's try this. |
📌 Commit 3ffa6da has been approved by |
⌛ Testing commit 3ffa6da with merge 1c1e3fa0c13d7b542bafb3b6401bbd57403d2cbc... |
💔 Test failed - status-travis |
Failed with wasm32-unknown-unknown 🤔
|
Ping from triage, @michaelwoerister ! Will you have time to address the test failures? |
I've actually come around to the opinion that we should not do this as it would cause our tests to be run in a non-standard compilation mode. |
@aidanhs, maybe you want to give this a try. It reduced the execution time for
run-pass
from 8 min 30s to 7 min 5s on my machine (though that was just one test run, so ymmv).r? @aidanhs