-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Caching the /target to avoid recompilation #20186
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
Conversation
a8e906c to
d4c18df
Compare
|
Thank you @Suryansh-Dey Can you please measure the impact this change has on build times (with links to the jobs where you measured)? We have found in the past that these caches actually slow things down |
21f35b3 to
d4c18df
Compare
|
Here is the Comparison Without cache
source With cache
source |
alamb
left a comment
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.
Thank you @Suryansh-Dey
@Omega359 I think you may have removed some of these caches last time -- do you remember anything else we should look as part of this change?
|
Thanks for the analysis @Suryansh-Dey -- did you compare the timing of this branch with the timing of what happens on main (without any caching)? I think that is probably the most important comparison I looked at the most recent build job from main (without any caching) and it also takes about 2 minutes https://github.com/apache/datafusion/actions/runs/21791124499/job/62870501521
However, it may be on a different runner, so I started the tests on this PR so we can measure again |
|
Actually the CI you ran was of Rust.yaml which does have Cache. You may see the Rust Dependency Cache there. It was added in an old pr But they were missing in extended.yaml. I guess I used a bad example |
CI would run out of disk space iirc |
|
I went through some of the previous PR's related to this. It's a mixed bag, sccache was in for a bit but was yanked because of a security concern I think. Rust cache was in but at the time I think we had issues with disk space and overall I don't think it helped much since the cache hit rate wasn't amazing because of github runners have so little disk space. |
|
Closing for now till I get something more effective |



Which issue does this PR close?
None. An improvement.
Rationale for this change
Several GitHub Actions workflows were missing
rust-cache, causing redundant recompilation of dependencies and tool installations on every run. This increases CI time and resource usage unnecessarily.What changes are included in this PR?
Added
Swatinem/rust-cacheto the following workflows:license-header-check,typoshawkeye,typos-clitool installationsbuild-docscargo-depgraphinstallationlinux-test-doc-buildcargo-depgraphinstallationlinux-build-lib,linux-test-extended,hash-collisions,sqllogictest-sqliteNotes:
cargo cleansteps in extended.yml are preserved to prevent disk space exhaustion on standard GitHub runnersmainbranch to avoid polluting the cache with PR-specific buildscache-targets: falsefor tool-only caching jobs to minimize cache sizeAre these changes tested?
rust-cacheAre there any user-facing changes?
No - this is a CI-only improvement that should reduce workflow execution time.