Supports LRU eviction on disk cache, change the default expire type to accessDate instead of modificationDate #3759
Conversation
|
I have done this in my image loading library for quite a long time (though I used to use modification date) https://github.com/timonus/TJImageCache/blob/main/TJImageCache/TJImageCache.m#L196 |
|
Sounds great. I'll have a check later. Actually these code was not changed at least 5 years...😂 So it's broken at the beginning ? |
| } | ||
| NSData *data = [NSData dataWithContentsOfFile:filePath options:self.config.diskCacheReadingOptions error:nil]; | ||
| if (data) { | ||
| [[NSURL fileURLWithPath:filePath] setResourceValue:[NSDate date] forKey:NSURLContentAccessDateKey error:nil]; |
There was a problem hiding this comment.
Is this time consuming ?
If so, need a extra if (self.config.diskCacheExpireType == SDImageCacheConfigExpireTypeAccessDate) guard
If not, it's OK
There was a problem hiding this comment.
It is quite fast, significantly faster than the data read in my (very quick) testing.
Logged times are (1) time to read data, (2) time to write content access date, and (3) 2/1
| _diskCacheWritingOptions = NSDataWritingAtomic; | ||
| _maxDiskAge = kDefaultCacheMaxDiskAge; | ||
| _maxDiskSize = 0; | ||
| _diskCacheExpireType = SDImageCacheConfigExpireTypeModificationDate; |
There was a problem hiding this comment.
It's ok to change default value, but this PR will marked as Behavior Changes and I'll point this on release changelog
Actually I don't know why the original author use modifyas expire type, since it's not actually LRU (we usually don't write to same cache key image)
|
Can you rebase and run the test cases again (or update the test case with your changes, since you change the default value) The master branch fix the CI issue |
The OS doesn't automatically bump access time when reading files, so LRU would effectively be equivalent to "least recently downloaded" in SDWebImage.
This is possibly controversial, but I suspect most clients of SDWebImage would prefer files purged based on their last time of use instead of the date when they were downloaded.
|
Alright, rebased and linting seems to pass. It looks like unit tests are all failing to build but I don't think that's caused by the contents of this PR. I updated the one test that referenced modified date here. |
|
@dreampiggy anything I can do to nudge this along? I'm unable to run the tests locally without installing Cocoapods, which I need to make other changes for and I'd rather not do. I don't think the tests are failing due to my change but if they are could you let me know? |
|
The |
Co-authored-by: DreamPiggy <lizhuoli1126@126.com>
Co-authored-by: DreamPiggy <lizhuoli1126@126.com>
|
Ah, thank you! Sorry I didn't catch that. |
|
Merge for now. But the release date of 5.20 may be delayed due to other break changes I want to added. |
New Pull Request Checklist
I have read and understood the CONTRIBUTING guide
I have read the Documentation
I have searched for a similar pull request in the project and found none
I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)
I have added the required tests to prove the fix/feature I am adding
I have updated the documentation (if necessary)
I have run the tests and they pass
I have run the lint and it passes (
pod lib lint)Pull Request Description
NSFileManagerdoes not automatically increment access date when reading files off disk, which means that use ofSDImageCacheConfigExpireTypeAccessDateis effectively identical to usingSDImageCacheConfigExpireTypeCreationDate. This doesn't seem like the correct behavior on SDWebImage's part, and it isn't what's documented here. This PR makes it so that SDWebImage adjusts the file content access date when files are read off disk to address the issue.I've also added a commit that updates the default cache behavior to
SDImageCacheConfigExpireTypeAccessDatesince I believe that's what's commonly expected. People generally want cache purging by time-of-use in my experience instead of using the time the file was downloaded at. If you strongly disagree I can omit that change!