Skip to content
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

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Mar 7, 2024

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:

  • There is just more work to do, as different features mean we need to compile packages multiple times.
  • More compilation artifacts means it also takes more time to compress/decompress by the cache step.
  • The cargo test pass that comes after cargo 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 from cargo clippy now. That also means that there are fewer reasons to keep it in the same job. Thus this PR splits the cargo clippy + test jobs into separate cargo clippy and cargo 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.

Job Time Diff
cargo clippy (macos-latest) 10m 51s
cargo clippy (macos-14) 2m 53s -73%
     
cargo doc (macos-latest) 2m 25s
cargo doc (macos-14) 1m 39s -32%

@xStrom xStrom marked this pull request as ready for review March 7, 2024 14:38
name: cargo doc
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [windows-latest, macos-latest, ubuntu-latest]
os: [windows-latest, macos-14, ubuntu-latest]
Copy link
Member

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

Copy link
Member Author

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.

@xStrom xStrom added this pull request to the merge queue Mar 14, 2024
Merged via the queue into linebender:main with commit 91c945a Mar 14, 2024
11 checks passed
@xStrom xStrom deleted the concurrentci branch March 14, 2024 14:43
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants