Revert #93800, fixing CI time regression #94205
Conversation
…ir-val, r=oli-obk" This reverts commit a240ccd, reversing changes made to 393fdc1. This PR was likely responsible for a relatively large regression in dist-x86_64-msvc-alt builder times, from approximately 1.7 to 2.8 hours, bringing that builder into the pool of the slowest builders we currently have. This seems to be limited to the alt builder due to needing parallel-compiler enabled, likely leading to slow LLVM compilation for some reason.
rustbot
added
the
T-compiler
|
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
why is that the case and what's the impact from removing that flag? I have to admit that it feels pretty bad to revert a PR because of anything related to parallel-compiler as iirc that feature is still pretty far from ready and not something we currently care about. if removing parallel-compiler doesn't work, r=me |
|
I'm fairly certain that these changes are not the cause and any other small changes can cause the same regression in parallel compiler. We have no one with parallel compiler knowledge to even begin debugging this. Still, let's revert and re-land the individual changes instead of all at the same time, then we at least know what minimal change makes llvm unhappy. @bors r+ |
|
📌 Commit 9f76214 has been approved by |
bors
added
S-waiting-on-bors
|
Dropping parallel compiler builds is "fine" (as I mention in PR description) but seems like a change that would need more discussion. In general, I suspect that the particular changes here are touching something in llvm and parallel compiler just happens to create the right conditions for it. Note that this seems to happen with a standard (non parallel) rustc, just compiling a rustc that is parallel, since it affects stage 0 compilations. So it's likely no particular parallel knowledge is required to debug. |
|
@bors p=1 to drop CI times |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (1103d2e): comparison url. Summary: This benchmark run did not return any relevant results. 6 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Reinstate rust-lang#93800 rust-lang#93800 caused a regression in an alt builder with parallel enabled. rust-lang#94205 reverted that PR because of the regression. For an unknown reason the regression has disappeared, so we reinstate the changes in rust-lang#93800 here. r? `@Mark-Simulacrum`
This reverts commit a240ccd (merge commit of #93800), reversing
changes made to 393fdc1.
This PR was likely responsible for a relatively large regression in
dist-x86_64-msvc-alt builder times, from approximately 1.7 to 2.8 hours,
bringing that builder into the pool of the slowest builders we currently have.
This seems to be limited to the alt builder due to needing parallel-compiler
enabled, likely leading to slow LLVM compilation for some reason. See some
investigation in this Zulip stream.
cc @lcnr @oli-obk @b-naber (per original PRs review/author)
We can re-apply this PR once the regression is fixed, but it is sufficiently large that I don't think keeping this on master is viable in the meantime unless there's a very strong case to be made for it. Alternatively, we can disable that builder (it's not critical since it's an alt build), but that obviously carries its own costs.