Add disk cache infrastructure back with tests #351
Closed
Conversation
This file contains 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
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #351 +/- ##
==========================================
- Coverage 85.85% 0.00% -85.86%
==========================================
Files 24 24
Lines 2871 2680 -191
==========================================
- Hits 2465 0 -2465
- Misses 406 2680 +2274
... and 22 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
vchuravy
force-pushed
the
vc/diskcache2
branch
2 times, most recently
from
cb25a34 to
49f3f67
Compare
claforte
reviewed
Aug 2, 2022
vchuravy
commented
Aug 2, 2022
|
Without caching: First run: Second run: |
|
In discussion with @williamfgc, maybe we shouldn't make the cache_key static, so that an application can set it at startup? I would most likely put in the git-hash of the application. |
vchuravy
commented
Aug 3, 2022
vchuravy
commented
Aug 3, 2022
|
What causes the 5s regression going from 'without cache' to 'first run'? |
|
We were discussing with @jpsamaroo... |
I think that would be rather hard to do. This is still a stop-gap towards proper precompilation caching support. |
Apply suggestions from code review
|
On Julia 1.9 and current CUDA#master with no disk-cache first compilation got a lot faster. |
|
Now first run with caching: Second run hitting the cache: So
|
|
On an Oceananigans test case from time spent in setup went from 150s spent in |
maleadt
reviewed
Apr 4, 2023
|
|
||
| !!! warning | ||
| The disk cache is not automatically invalidated. It is sharded upon | ||
| `cache_key` (see [`set_cache_key``](@ref)), the GPUCompiler version |
There was a problem hiding this comment.
Suggested change
| `cache_key` (see [`set_cache_key``](@ref)), the GPUCompiler version | |
| `cache_key` (see [`set_cache_key`](@ref)), the GPUCompiler version |
maleadt
reviewed
Apr 4, 2023
| end | ||
|
|
||
| key(ver::VersionNumber) = "$(ver.major)_$(ver.minor)_$(ver.patch)" | ||
| cache_path() = @get_scratch!(cache_key() * "-kernels-" * key(VERSION)) |
There was a problem hiding this comment.
Maybe include "cache" in the directory name? Or make this a subdirectory of the existing compile_cache scratch directory? That way the cache would also get wiped on reset_runtime, which is done when recompiling CUDA.jl. Or is that unwanted?
There was a problem hiding this comment.
Might also be confusing to have compiled in the scratch dir, containing the runtime bitcode, and cache for compiled kernels :-) maybe cache/{runtime,jobs}?
I know we're bikeshedding here :-)
There was a problem hiding this comment.
That way the cache would also get wiped on reset_runtime, which is done when recompiling CUDA.jl. Or is that unwanted?
I was trying to add a dependency on the version of GPUCompiler/CUDA. Cache invalidation is a big potential footgun here.
maleadt
reviewed
Apr 4, 2023
| @@ -173,7 +206,18 @@ end | |||
| job = CompilerJob(src, cfg) | |||
|
|
|||
| asm = nothing | |||
| # TODO: consider loading the assembly from an on-disk cache here | |||
| # can we load from the disk cache? | |||
| if disk_cache() | |||
There was a problem hiding this comment.
I was frustrated by the need to recompile GPUCompiler to turn caching on and off.
I originally had it be a compile time preference which is what we would need for it to be @static
There was a problem hiding this comment.
I didn't realize we had non compile-time preferences...
maleadt
reviewed
Apr 4, 2023
| @@ -182,6 +226,10 @@ end | |||
| end | |||
|
|
|||
| asm = compiler(job) | |||
|
|
|||
| if disk_cache() && !isfile(path) | |||
Found by @simone-silvestri when running with a large number of nodes and a shared filesystem. |
|
That |
3 tasks
|
Replaced by #557 |
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.
Using Preferences.jl instead of environment variables and split the cache on a user defined key, GPUCompiler version, and Julia version.