-
Notifications
You must be signed in to change notification settings - Fork 136
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
Refactor CI jobs to allow for more concurrency. #505
Conversation
.github/workflows/ci.yml
Outdated
name: cargo doc | ||
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: | ||
os: [windows-latest, macos-latest, ubuntu-latest] | ||
os: [windows-latest, macos-14, ubuntu-latest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to document for all platforms here? We don't have any platform specific code at this level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the Vello repo doesn't have an active need for it. It doesn't increase wall time in most scenarios, but if there are a bunch of PRs doing CI at the same time, these extra jobs can actually start to matter. Thus I reduced the doc job to only Ubuntu here and added a note about potential future expansion.
This was most recently updated in linebender/vello#505 The main differences are: 1) Greater concurrency between clippy and testing 2) More explanations 3) MSRV checking 4) Use of `cargo hack` 5) WASM checked in CI I've also added the current values of our MSRVs to the main three crates.
With #504 merged we now do Clippy checks for each package and feature separately. That is great for catching issues but it comes with the downside of increasing the wall time our CI takes to run.
The increased wall time comes due to several factors:
cargo test
pass that comes aftercargo clippy
will do feature unification and so is guaranteed to force another compilation pass on a bunch of crates.This PR here addresses the third point, i.e. that
cargo test
is more decoupled fromcargo clippy
now. That also means that there are fewer reasons to keep it in the same job. Thus this PR splits thecargo clippy + test
jobs into separatecargo clippy
andcargo test
jobs.Splitting the jobs will have a significant impact on wall time. For wall time what ultimately matters is the slowest job, which is the old
cargo clippy + test
on Windows. I estimate that this PR will cut its wall time by ~40% from 12m30s to 7m30s. (Edit: The slowest Windows test on this PR took 7m39s. There will be run-to-run variance of course, but it's the ballpark.)The pre- and post-step overhead of the old job is about 17% - most of which is cache related and will be split between jobs now too. Not a clear split as there is some overlap in artifacts, but e.g. for Windows instead of a single 900MB cache we get a 350MB one and a 700MB one. So splitting the cache jobs won't result in a big CPU time increase and helps cut down the wall time even more.
I also switched all our macOS jobs to be explictly
macos-14
, because it's just so much faster. The following table shows the time comparison with cold cache runs. N=1 but the difference is huge and matches by previous casual observations, so I'm not concerned with run-to-run variance.