CI - Runner caching improvements - #5798
Conversation
…o bfops/test-runner
Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
joshua-spacetime
left a comment
joshua-spacetime
left a comment
There was a problem hiding this comment.
The numbers are fantastic, but I still have concerns over git-restore-mtime.
| # cargo has a behavior that's a bit unfortunate for us here. It does not know the mtime of the files that | ||
| # were used to build its artifacts; it just knows the mtime of the artifact. | ||
| # So, if you have a sequence of events like: | ||
| # 1. cache source commit X happens at time X | ||
| # 2. PR commit Y happens at time Y > X | ||
| # 3. cache is warmed from commit X at time Z > Y | ||
| # | ||
| # cargo then sees "artifact built at time Z is more recent than time Y, so it must be up to date". | ||
| # | ||
| # So here we restore committed mtimes to reuse warm artifacts, then touch files that | ||
| # differ from the warmed commit so they will look more recent than the artifact, | ||
| # and cargo will properly rebuild anything depending on those files. |
There was a problem hiding this comment.
I really find git-restore-mtime to be problematic for several reasons, but to name a few:
- The rust compiler doesn't use it (it uses sccache).
- Cargo itself doesn't appear to recommend it (it recommends sccache).
git-restore-mtimeopenly documents what seem to be quite relevant short-comings.- The timestamps that are being compared come from different clocks.
It just seems really brittle, and so I'd prefer not to rely on it if we don't have to. I'm happy to revisit it though if we see that sccache still leaves a lot of savings on the table.
There was a problem hiding this comment.
If your preference is to block this PR on sccache, rather than doing it in a follow-up, I can do that.
There was a problem hiding this comment.
the clock concern is an interesting one. If the goal is still to use target for native libs, wouldn't we have the same concern there? Would your intention be to keep the same special casing for the native libs that we have now?
There was a problem hiding this comment.
If the goal is still to use
targetfor native libs, wouldn't we have the same concern there?
I don't believe so. git-restore-mtime only runs on workspace files. It doesn't touch third-party deps stored in cargo's cache.
Would your intention be to keep the same special casing for the native libs that we have now?
No, special casing for native libs should not be necessary anymore.
| - *set-native-cache-keys | ||
| - &restore-jemalloc |
There was a problem hiding this comment.
It might be better to remove these native lib caches separately after updating the image. Especially if you're not planning to update the linux and windows images at the same time?
There was a problem hiding this comment.
good point, I'll fix the windows jobs
# Description of Changes This only changes Linux runners. - use runner-cached sccache dir for most things - use a runner-cached target directory for native/build-script artifacts - static `CARGO_TARGET_DIR` has been replaced with a step that sets it (because it is now based on `HOME`) - some windows jobs' path juggling has been updated correspondingly as well ## Timing results These are sums of Cargo's reported `Finished ... in ...` times, matching the table in clockworklabs#5798. The test run was [CI run 33413710732](https://github.com/clockworklabs/SpacetimeDB/actions/runs/33413710732) on `spacetimedb-new-runner-2-staging`. | Job | Cargo time in `master` | this PR | Rust sccache hit rate | | --- | ---: | ---: | ---: | | Test Suite | 9m30s - 10m02s | **6m44.9s** | 12.96% | | Upload build artifacts (Linux) | 2m27s - 2m46s | **1m07s** | 93.33% | | Lints | 1m46s - 2m09s | **55.6s** | 6.52% | | Build smoketests (Linux) | 48.9s - 57.1s | **10.4s** | 83.33% | | Check CLI docs | 56s - 1m08s | **13.0s** | 98.28% | | Test spacetimedb-update flow (Linux) | 29.6s - 30.7s | **2.9s** | 100% | | csharp-testsuite | 28.2s - 46.6s | **14.2s** | 0% | | TypeScript - Tests | 22.8s - 28.4s | **2.3s** | 100% | | Build and test wasm bindings | 25.0s - 33.2s | **3.3s** | 100% | | Docs build | 20s - 21s | **1.2s** | 100% | | Check that packages are publishable | 19s - 21s | **1.1s** | not reported | | godot-testsuite | 7.6s - 11.2s | **3.1s** | 100% | The next high-value change is to remove commit-specific metadata from shared Rust library artifacts (or otherwise confine it to a leaf artifact that does not participate in the test/lint dependency graph), then add the smoketest archive to the empty-target population phase. # API and ABI breaking changes None. # Expected complexity level and risk 2 # Testing - [x] the table is the testing # Rollback safety impact n/a --------- Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com> Co-authored-by: Zeke Foppa <bfops@users.noreply.github.com> Co-authored-by: joshua-spacetime <josh@clockworklabs.io> Co-authored-by: clockwork-labs-bot <clockwork-labs-bot@users.noreply.github.com>
4 participants
Description of Changes
This only changes things for the linux runners.
cargo ciinstead of calling the ci subcommand binaries directlymasterRemaining cache work
ringand its dependents rebuild because nested Cargo processes inherit the parenttools/cipackage'sCARGO_MANIFEST_DIR; that package-specific environment leaks into the nested builds and changes their fingerprints. This accounts for 13.70s and 10.96s Clippy builds, a 10.31s CLI-doc build, and an 88s plus two roughly 40s Test Suite builds.reused 0because the image does not contain the pnpm store used by these installs. The jobs download 2,801 packages each, except Smoketests, which downloads 429.wasm-bindgen-cliare absent from the image. The job downloads 31 MB of Node plus 355 MB of wasm binaries and spends 15.36s compilingwasm-bindgen-clifrom source.modules/sdk-test-procedurecompiling for 6.66s, Blackholio's server for 7.82s, the latest-compatible module for 7.48s, and the TypeScript helper tools for 1.76s. The latest-compatible module also intentionally resolves newer compatible crate versions rather than the repository lockfile's exact versions.templatesdirectory itself. Checkout changes that directory mtime, whilegit restore-mtimerestores tracked file mtimes rather than directory metadata, so Cargo rebuilds the CLI even when template contents are unchanged. This is the sole Rust rebuild in Upload build artifacts (30.56s).Next steps
API and ABI breaking changes
None
Expected complexity level and risk
2
Testing
Just the existing CI. The performance table is the testing.