[CI] Improve windows machine CI test time #8730
Conversation
|
@Jefffrey I think I did some progress to improve win64 test performance. |
|
@Jefffrey @andygrove @alamb please review Thanks to @Jefffrey investigations in #8696 (comment) it looks like the issue with newer During this PR I found that increasing |
Do we need to increase opt-level for all OSs or just windows? |
|
Thanks @alamb for the feedback. Addressed all the comments |
There was a problem hiding this comment.
Improvements to Windows CI runtime is amazing 👍
I've raised an issue on Rust repo: rust-lang/rust#119560
But until that can be futher investigated/triaged, this will help reduce the CI runtime (current 3 hours is ages...)
For Linux and Mac runners, do those changes have any significant effect on test runtime? Otherwise maybe won't set those options to try keep things simple and as close to local environment as we could?
| @@ -290,6 +298,7 @@ jobs: | |||
| # with a OS-dependent path. | |||
| - name: Setup Rust toolchain | |||
| run: | | |||
| rustup update stable | |||
There was a problem hiding this comment.
Is the update here strictly necessary? Or should already be covered as part of the toolchain install below?
There was a problem hiding this comment.
i'll check it, maybe its not needed.
| # avoid rust stack overflows on tpc-ds tests | ||
| RUST_MINSTACK: "3000000" |
There was a problem hiding this comment.
This feels like something we should fix in tpc-ds tests, instead of accounting for it after the fact (I know I've had some trouble running those tests locally due to stack issue, so could happen to others)
There was a problem hiding this comment.
that is true, during testing I saw spontaneous stack overflow issue on physical q54 TPC-DS test, that was the reason to bump the stack size. But I agree its good to create a follow up ticket and figure out why q54 is not the same as others, maybe the bigger problem is behind it.
| # do not produce debug symbols to keep memory usage down | ||
| # hardcoding other profile params to avoid profile override values | ||
| # More on Cargo profiles https://doc.rust-lang.org/cargo/reference/profiles.html?profile-settings#profile-settings | ||
| RUSTFLAGS: "-C debuginfo=0 -C opt-level=0 -C incremental=false -C codegen-units=256" |
There was a problem hiding this comment.
I think some of these flags are already enabled, technically? Just for Linux though, not Mac/Windows
(Well debuginfo was set to 0, not sure on incremental)
There was a problem hiding this comment.
Thats good point, I would like to have configuration in a single place which is rust.yml, I'll create a followup to remove CARGO_INCREMENTAL and backtraces and other rust conf from action.yaml
I compared the linux and mac times on the most recent commit to master Mac took slightly longer (16m on this pr compared to 12m on main) Linux was faster (8m on this PR, compared to 9m on main) I think the differences are not significant and within normal variances |
Which issue does this PR close?
Closes #8696.
Rationale for this change
CI tests for windows machines are extremely slow and running 3+ hours now, whereas other architectures are able to finish within 30 mins.
This PR adds more granular build options for each architecture.
With this change windows build time is 60 minutes, which is 2x slower than others OS but still 3x better than current windows run
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?