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

Evict old files from the cache prior to saving #270

Merged
merged 5 commits into from
Dec 31, 2024

Conversation

planetmarshall
Copy link
Contributor

uses the --evict-older-than option to evict any files from the cache that were not used during the job run. This prevents the cache from growing indefinitely as otherwise old files from previously restored caches are never deleted.

For a test I performed the following -

  1. Built CMake 3.30.6
    • evictOldFiles=true - cache Size 39Mb
    • evictOldFiles=false - cache Size 39Mb
  2. Then Built CMake 3.31.2, reusing the cache from 1. Minimal cache hits in both cases due to change in sources.
    • evictOldFiles=true - cache Size 39Mb
    • evictOldFiles=false - cache Size 78Mb
  3. Then Built CMake 3.31.2 again
    • evictOldFiles=true - cache Size 39Mb - Cache hits 100%
    • evictOldFiles=false - cache Size 78Mb - Cache hits 100%

@planetmarshall planetmarshall force-pushed the evict-old-files-pre-save branch from d6d09c9 to 0e945d8 Compare December 15, 2024 20:35
@planetmarshall planetmarshall force-pushed the evict-old-files-pre-save branch from 0e945d8 to 51af452 Compare December 15, 2024 20:37
@hendrikmuhs
Copy link
Owner

Thanks for the PR.

I thought a bit about it. First a comment:

This prevents the cache from growing indefinitely as otherwise old files from previously restored caches are never deleted.

There is already a max-size option with a default of 500M, ccache should respect that and keep the total cache size below that boundary.

Nevertheless, I think a 2nd option to evict based on age makes sense to me. I have some questions:

  • Did you think about simply exposing the --evict-older-than option as is?
  • What is reason to turn it into a bool?

I am suggesting the following:

We could support both cases, the use case of using evict-older-than as ccache designed it and your case of evicting everything that didn't hit in the current job run.

If you define evict-older-than as string, you could support additional magic values for the special cases:

  • ``(empty) for no eviction(default, current behavior)
  • job to evict everything older than this run.
  • everything else passes the value as is, an int with either d for days or s for seconds

That way we keep the option as in ccache and just add job for this special case.

@planetmarshall
Copy link
Contributor Author

There is already a max-size option with a default of 500M, ccache should respect that and keep the total cache size below that boundary.

Sure, but with the CI cache limited at 10Gb (this can be quickly exhausted if you're working on something like, say, LLVM) I think it makes sense to be able to trim the cache where possible.

Nevertheless, I think a 2nd option to evict based on age makes sense to me. I have some questions:

* Did you think about simply exposing the `--evict-older-than` option as is?

I did - but I explicitly wanted to use the job time for this parameter which is dynamic. I also thought about using the namespace feature for this - maybe something for a future enhancement.

* What is reason to turn it into a bool?

Just because the job time is an internal calculation - so you can't supply it as a static value.

I am suggesting the following:

We could support both cases, the use case of using evict-older-than as ccache designed it and your case of evicting everything that didn't hit in the current job run.

If you define evict-older-than as string, you could support additional magic values for the special cases:

* ``(empty) for no eviction(default, current behavior)

* `job` to evict everything older than this run.

* everything else passes the value as is, an int with either `d` for days or `s` for seconds

That way we keep the option as in ccache and just add job for this special case.

Yes I like this suggestion - it works for my use case. I will update the PR.

@planetmarshall planetmarshall force-pushed the evict-old-files-pre-save branch from 5664b2c to 6348297 Compare December 28, 2024 20:06
@planetmarshall planetmarshall force-pushed the evict-old-files-pre-save branch from 6348297 to 7a42304 Compare December 28, 2024 20:07
@hendrikmuhs
Copy link
Owner

👍 great! LGTM

@hendrikmuhs hendrikmuhs merged commit e7a0524 into hendrikmuhs:main Dec 31, 2024
45 checks passed
@planetmarshall planetmarshall deleted the evict-old-files-pre-save branch December 31, 2024 17:43
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.

2 participants