[ios] Cache Management API

[jmkiley]

iOS version of #14962

Tracking my progress

Core Method	iOS Method/property	implementation	iosapp example	tests	docs	benchmarking
invalidateAmbientCache()	- invalidateAmbientCache	X	X	X	X
invalidateRegion(int64_t regionID)	- invalidateOfflineRegionForPack:	X	X	X	X
maximumAmbientCacheSize(uint64_t)	setMaximumAmbientCacheSize	X	X	X	X
clearAmbientCache()	- clearAmbientCache	X	X	X	X
resetDatabase()	- resetDatabase	X	X	X	X

To Do:

• Implementation
• Document the new API.
• Revise existing documentation to clarify that MGLOfflineStorage manages both offline packs - and the ambient cache
• Add a demo to iosapp
• Add tests
  - [ ] Benchmark/time the methods. Tailwork.

cc @langsmith @chloekraw

[jmkiley]

I am currently getting this warning when I call resetDatabase in both iosapp and tests:

mapbox-gl-native/platform/default/src/mbgl/storage/offline_database.cpp

Line 128 in 24df7fe

Log::Warning(Event::Database, "Removing existing incompatible offline database");

Not sure if I'm missing something with the current Objective-C method.

cc @tmpsantos

[jmkiley]

I updated the iOS docs around setMaximumAmbientCacheSize to match the Android docs, which state that the maximum size should be set before the map and style have been loaded.

I am also still seeing the issue I commented about here, and am not sure how to handle a warning. It doesn't seem to be caught by the exception.
I was misreading the message and that was not the cause. 🤦‍♀

[jmkiley]

Thank you @fabian-guerra for helping me troubleshoot the issue with -resetDatabaseWithCompletionHandler. The issue was that I had dispatch_async(dispatch_get_main_queue(), [&, completion, error](void) { instead of dispatch_async(dispatch_get_main_queue(), ^{. It does not seem to be crashing now 🎉 .

[chloekraw]

I'd say this is not quite accurate: this method clears the entire ambient cache by marking tiles as invalid. Currently there's no way to specify which specific tiles are valid or not; all tiles in the ambient cache are considered invalid and re-downloaded if this method is triggered.

This is more efficient than deleting the ambient cache or the offline database. I also recommend referencing the specific APIs for those other two methods in parentheses so it's clear that this is the most preferred method for clearing the ambient cache.

It might even be worth making that the last line in this section of documentation:

"This is the recommended method for clearing the ambient cache. It's the most efficient and least computationally expensive method."

[jmkiley]

Okay, sounds good. I was going off the core documentation. It sounds like the behavior doesn't match this?

mapbox-gl-native/include/mbgl/storage/default_file_source.hpp

Lines 196 to 200 in 68e3cba

	* Forces Mapbox GL Native to revalidate resources stored in the ambient
	* cache with the tile server before using them, making sure they
	* are the latest version. This is more efficient than cleaning the
	* cache because if the resource is considered valid after the server
	* lookup, it will not get downloaded again.

[chloekraw]

Ahh, thanks. I'll try to sync with @tmpsantos tomorrow to clear things up. cc @zugaldia

[chloekraw]

Change to "Deletes the ambient cache."

Let's leave out "freeing storage space" -- developers will be more incentivized to use this method than they should be.

[chloekraw]

Should we also mention the reinitialization of the ambient cache here?

[jmkiley]

Per an earlier review from @friedbunny, I would like to keep it as clears so that we are starting the documentation with the verb from method name. How about Clears the ambient cache by deleting resources.?
Edited to add: I don't believe this method reinitializes the ambient cache. It removes resources from the cache, but the cache.db is not deleted.

[chloekraw]

Ah okay, fair enough. The core documentation consistently uses "erase" as the verb but the method name does start with clear, and I understand the desire to keep those consistent. I'm also ok with using "erase" as a verb instead of "delete" to match the core documentation:

Clears the ambient cache by erasing resources from the ambient cache.

FWIW I think the rest of the core documentation is good to copy and paste over to the platform side:

This operation can be potentially slow because it will trigger a VACUUM on SQLite, forcing the database to move pages on the filesystem.

Resources overlapping with offline regions will not be affected by this call.

[julianrex]

Core and Android use clearAmbientCache (unless I'm looking at the wrong thing).

[chloekraw]

Suggestion: "Clears the ambient cache by marking its resources as invalid. This method checks that the tiles in the ambient cache match those from the server. If the local tiles do not match, the tiles in the ambient cache are re-downloaded."

[julianrex]

This can move inside the if (completion) below.

[jmkiley]

If I move it inside if (completion), will it only happen if there is a completion handler?

[julianrex]

Yep, but you're only creating the error object, and that only gets used in the completion block. (Same thing in the methods below too.)

[friedbunny]

Thinking more about error codes, can we test that bad input/etc throw the expected errors?

[jmkiley]

Thanks @friedbunny, @chloekraw, @julianrex, and @zugaldia!

@friedbunny That would be good. Could I leave testing bad input as tailwork to do this week?

I think I addressed the feedback related to this specific PR. If there is anything else, please let me know. Maybe @chloekraw can ticket out questions about docs that are consistent cross-platform, so those can be addressed later this week?

[friedbunny]

Thanks for tackling this, it’s a long overdue API. 😁 I’m onboard with merging this now and then backfilling tests before the final picklejuice release.

[julianrex]

This looks good to merge. Just my minor comment, but that's small fry and can wait :)

[julianrex]

Yep, but you're only creating the error object, and that only gets used in the completion block. (Same thing in the methods below too.)