Refactor CI jobs to allow for more concurrency.#505
Merged
xStrom merged 3 commits intolinebender:mainfrom Mar 14, 2024
Merged
Conversation
nuzzles
reviewed
Mar 10, 2024
DJMcNab
approved these changes
Mar 14, 2024
.github/workflows/ci.yml
Outdated
| strategy: | ||
| matrix: | ||
| os: [windows-latest, macos-latest, ubuntu-latest] | ||
| os: [windows-latest, macos-14, ubuntu-latest] |
Member
There was a problem hiding this comment.
Is it necessary to document for all platforms here? We don't have any platform specific code at this level
Member
Author
There was a problem hiding this comment.
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.
DJMcNab
added a commit
to DJMcNab/xilem
that referenced
this pull request
Jun 27, 2024
DJMcNab
added a commit
to DJMcNab/xilem
that referenced
this pull request
Jun 28, 2024
github-merge-queue bot
pushed a commit
to linebender/xilem
that referenced
this pull request
Jun 28, 2024
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 testpass that comes aftercargo clippywill 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 testis more decoupled fromcargo clippynow. That also means that there are fewer reasons to keep it in the same job. Thus this PR splits thecargo clippy + testjobs into separatecargo clippyandcargo testjobs.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 + teston 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.