mapbox / mapbox-navigation-ios

Turn-by-turn navigation logic and UI in Swift on iOS
https://docs.mapbox.com/ios/navigation/
Other
862 stars 312 forks source link

Enable configuration of file caching footprint #1384

Open akitchen opened 6 years ago

akitchen commented 6 years ago

As we add support for caching more types of resources in the SDK we should start being more intentional about the file cache footprint.

This work will introduce developer configurability of cache footprint (both disk and memory) and disk cache age, and introduce sane defaults for new configuration options.

As a developer, I should be able to:

Note that both the ImageCache and DataCache currently use a FileCache under the hood. The object relationships should be reconsidered as a part of this work in order to ensure that multiple FileCache instances don't conflict with each other in practice -- for example, it could become a singleton, or multiple instances might need to manage their own cache directories on disk.

Also note that the memory cache eviction heuristic is an implementation detail we aren't concerned with.

It is not believed to be necessary to implement a refCount mechanism at this time. A few comments have been removed from this issue in order to keep it focused on this description, which has been edited.

bsudekum commented 6 years ago

This sounds good to me. One suggestion, to guard against a crazy situation where either a bug/long route/edge case produces gigabytes of cached images or sound files, it might make sense to have a rather large default instead of unlimited. I think it would only be necessary to configure maxDiskCacheSize. Maybe something like 2gb?

JThramer commented 6 years ago

Posterity: It seems to me that the proper way to implement this would to be to keep a "metadata array" around in the cache that describes the cached elements. Maybe an array of type T, where T contains filesize, timestamp, and a reference to the cached item (weak pointer for memory cache, file URL for disk cache), which will allow us to check for oversize/outdated elements. The tricky part about this is that in the DiskCache, the metadata array will also need to be serialized so it persists between runs.

akitchen commented 6 years ago

This might make sense, and @1ec5 offered a similar suggestion some weeks back (citation needed)

I don't think we'd need to keep a pointer/reference, but we do need a unique identifier to key off of (the cacheKey perhaps?).

Also, if we're going down this path, I'd like to add a last_accessed_at time stamp to enable prioritizing least recently accessed objects for eviction.

All of this feels like it might be over-engineering if the footprint is small; we might want to take a closer look at the cache usage across long-running app installs. Simply deleting old objects based on the creation-time filesystem timestamp might be enough... there is a 'FileAttributeKey` for that, and of course size as well, so we might be able to get away with not introducing a metadata table unless we want to implement LRU eviction.

1ec5 commented 6 years ago

It seems to me that the proper way to implement this would to be to keep a "metadata array" around in the cache that describes the cached elements.

This might make sense, and @1ec5 offered a similar suggestion some weeks back (citation needed)

Yes, this is what I had in mind: https://github.com/mapbox/mapbox-navigation-ios/issues/1391#issuecomment-387507267.