Reduce synchronous I/O when checking ImageCache.isCached
#1480
Conversation
For more details see the discussion on: https://developer.apple.com/documentation/foundation/nsurl/1410614-urlbyappendingpathcomponent Since we know that we're looking for a file and a trailing slash will never be necessary, we can safely use the more specific version of the function
This is a low-cost best-effort strategy that triest to reduce as much as possible the need for I/O to check if something is cached. A file name we've never seen can just be skipped immediately. Something that got added and was removed later would still end up hitting the disk to figure that out, but that's not different from before. Unsure about the exact multi-threading semantics of that Set though, we might need to do some king of synchronization
|
@raja-baz Thanks for it.
Oh, I never thought that it would be an expensive operation. I will check it.
It is a good idea to create a cache table in memory to prevent checking the file system. But the implementation in your PR would bring quite a few false positive, which is unacceptable. Think the case that disk cache is purged by the system (it is in the cache folder so that is totally possible) after the cache table being created. The cache system would think that the cache is on disk, but finally, nothing is found. Anyway, I will consider whether there is a better way to improve the performance here. Thank you for your valuable suggestions! |
|
My design goal with the cache was to do a best-effort approach that changes as little and as low-risk as possible while being strictly better in terms of performance. A false positive means we check the file system unnecessarily before finding out that the image isn't cached. However, right now that's the case for all operations. So it is strictly better than before with no visible downside. Unless I'm missing something? Basically: I'm okay with false positive (avoidable I/O that we didn't avoid) but absolutely must never have a false negative (bug which causes network fetch when we could've loaded from disk). And I wanted to achieve that with as little change and intrusiveness in the code |
|
PS: I even had "maybe" in the name to indicate that this could have false positives |
|
Ah! I got your idea. It seems that I misunderstood your implementation. It makes sense to add these as a "precondition" check for the actual disk file checking. I will take a detailed look at these again. |
We use
ImageCache.isCachedon the main thread in our application and we noticed that it would sporadically cause the main thread to hang on I/O. Some investigation showed that this was due to 2 things.DiskStoragewhich uses a variant ofURLByAppendingPathComponent:that does disk I/O to figure out if the path it's building is for a directory or a file. In our case we know it's a file and this is completely unnecessary and we can simply useURLByAppendingPathComponent:isDirectory:and avoid this.