diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index 88f318581ba..2942a25a857 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -121,20 +121,18 @@ class DefaultFileSource : public FileSource { * Eviction works by removing the least-recently requested resources not also required * by other regions, until the database shrinks below a certain size. * - * If the |pack| argument is `false` the database file packing is skipped and the - * database file does not shrink after this operation completes. Database file - * packing can be done later with `packDatabase()`. It is a useful optimization - * e.g. when several regions should be deleted in a row. - * * Note that this method takes ownership of the input, reflecting the fact that once * region deletion is initiated, it is not legal to perform further actions with the * region. * + * Note that this operation can be potentially slow if packing the database occurs + * automatically (see runPackDatabaseAutomatically() and packDatabase()). + * * When the operation is complete or encounters an error, the given callback will be * executed on the database thread; it is the responsibility of the SDK bindings * to re-execute a user-provided callback on the main thread. */ - void deleteOfflineRegion(OfflineRegion&&, std::function, bool pack = true); + void deleteOfflineRegion(OfflineRegion&&, std::function); /* * Invalidate all the tiles from an offline region forcing Mapbox GL to revalidate @@ -189,12 +187,25 @@ class DefaultFileSource : public FileSource { /* * Packs the existing database file into a minimal amount of disk space. * + * This operation has a performance impact as it will vacuum the database, + * forcing it to move pages on the filesystem. + * * When the operation is complete or encounters an error, the given callback will be * executed on the database thread; it is the responsibility of the SDK bindings * to re-execute a user-provided callback on the main thread. */ void packDatabase(std::function callback); + /* + * Sets whether packing the database file occurs automatically after an offline + * region is deleted (deleteOfflineRegion()) or the ambient cache is cleared + * (clearAmbientCache()). + * + * By default, packing is enabled. If disabled, disk space will not be freed + * after resources are removed unless packDatabase() is explicitly called. + */ + void runPackDatabaseAutomatically(bool); + /* * Forces revalidation of the ambient cache. * @@ -212,9 +223,10 @@ class DefaultFileSource : public FileSource { /* * Erase resources from the ambient cache, freeing storage space. * - * Erases the ambient cache, freeing resources. This operation can be - * potentially slow because it will trigger a VACUUM on SQLite, - * forcing the database to move pages on the filesystem. + * Erases the ambient cache, freeing resources. + * + * Note that this operation can be potentially slow if packing the database + * occurs automatically (see runPackDatabaseAutomatically() and packDatabase()). * * Resources overlapping with offline regions will not be affected * by this call. diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java index 0051c17e030..a7bb9a99850 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java @@ -309,6 +309,9 @@ public void run() { /** * Packs the existing database file into a minimal amount of disk space. *

+ * This operation has a performance impact as it will vacuum the database, + * forcing it to move pages on the filesystem. + *

* When the operation is complete or encounters an error, the given callback will be * executed on the database thread; it is the responsibility of the SDK bindings * to re-execute a user-provided callback on the main thread. @@ -396,9 +399,10 @@ public void run() { /** * Erase resources from the ambient cache, freeing storage space. *

- * Erases the ambient cache, freeing resources. This operation can be - * potentially slow because it will trigger a VACUUM on SQLite, - * forcing the database to move pages on the filesystem. + * Erases the ambient cache, freeing resources. + *

+ * Note that this operation can be potentially slow if packing the database + * occurs automatically ({@link OfflineManager#runPackDatabaseAutomatically(boolean)}). *

*

* Resources overlapping with offline regions will not be affected @@ -661,7 +665,7 @@ private boolean isValidOfflineRegionDefinition(OfflineRegionDefinition definitio *

* Once this limit is reached, {@link OfflineRegion.OfflineRegionObserver#mapboxTileCountLimitExceeded(long)} * fires every additional attempt to download additional tiles until already downloaded tiles are removed - * by calling {@link OfflineRegion#deleteOfflineRegion(OfflineRegion.OfflineRegionDeleteCallback)}. + * by calling {@link OfflineRegion#delete(OfflineRegion.OfflineRegionDeleteCallback)}. *

* * @param limit the maximum number of tiles allowed to be downloaded @@ -669,6 +673,24 @@ private boolean isValidOfflineRegionDefinition(OfflineRegionDefinition definitio @Keep public native void setOfflineMapboxTileCountLimit(long limit); + /** + * Sets whether database file packing occurs automatically. + * By default, the automatic database file packing is enabled. + *

+ * If packing is enabled, database file packing occurs automatically + * after an offline region is deleted by calling + * {@link OfflineRegion#delete(OfflineRegion.OfflineRegionDeleteCallback)} + * or the ambient cache is cleared by calling {@link OfflineManager#clearAmbientCache()}. + * + * If packing is disabled, disk space will not be freed after + * resources are removed unless {@link OfflineManager#packDatabase()} is explicitly called. + *

+ * + * @param autopack flag setting the automatic database file packing. + */ + @Keep + public native void runPackDatabaseAutomatically(boolean autopack); + @Keep private native void initialize(FileSource fileSource); diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java index b188d60a9ec..d116535c5a6 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java @@ -388,52 +388,8 @@ public void run() { * by other regions, until the database shrinks below a certain size. *

*

- * When the operation is complete or encounters an error, the given callback will be - * executed on the main thread. - *

- *

- * After you call this method, you may not call any additional methods on this object. - *

- * - * @param callback the callback to be invoked - */ - public void delete(@NonNull final OfflineRegionDeleteCallback callback) { - if (!isDeleted) { - isDeleted = true; - fileSource.activate(); - deleteOfflineRegion(true /*pack*/, new OfflineRegionDeleteCallback() { - @Override - public void onDelete() { - handler.post(new Runnable() { - @Override - public void run() { - fileSource.deactivate(); - callback.onDelete(); - OfflineRegion.this.finalize(); - } - }); - } - - @Override - public void onError(final String error) { - handler.post(new Runnable() { - @Override - public void run() { - isDeleted = false; - fileSource.deactivate(); - callback.onError(error); - } - }); - } - }); - } - } - - /** - * Same as {@link OfflineRegion#delete} but skipping database file packing for performance reasons. - *

- * Database file packing can be done later with {@link OfflineManager#packDatabase}. - * This method is a useful optimization e.g. when several regions should be deleted in a row. + * Note that this operation can be potentially slow if packing the database + * occurs automatically ({@link OfflineManager#runPackDatabaseAutomatically(boolean)}). *

*

* When the operation is complete or encounters an error, the given callback will be @@ -445,11 +401,11 @@ public void run() { * * @param callback the callback to be invoked */ - public void deleteAndSkipPackDatabase(@NonNull final OfflineRegionDeleteCallback callback) { + public void delete(@NonNull final OfflineRegionDeleteCallback callback) { if (!isDeleted) { isDeleted = true; fileSource.activate(); - deleteOfflineRegion(false /*pack*/, new OfflineRegionDeleteCallback() { + deleteOfflineRegion(new OfflineRegionDeleteCallback() { @Override public void onDelete() { handler.post(new Runnable() { @@ -569,7 +525,7 @@ public void run() { private native void getOfflineRegionStatus(OfflineRegionStatusCallback callback); @Keep - private native void deleteOfflineRegion(boolean pack, OfflineRegionDeleteCallback callback); + private native void deleteOfflineRegion(OfflineRegionDeleteCallback callback); @Keep private native void updateOfflineRegionMetadata(byte[] metadata, OfflineRegionUpdateMetadataCallback callback); diff --git a/platform/android/src/offline/offline_manager.cpp b/platform/android/src/offline/offline_manager.cpp index c45386e3a78..be864e18aa7 100644 --- a/platform/android/src/offline/offline_manager.cpp +++ b/platform/android/src/offline/offline_manager.cpp @@ -168,6 +168,10 @@ void OfflineManager::setMaximumAmbientCacheSize(jni::JNIEnv& env_, const jni::jl std::exception_ptr exception) mutable { handleException(exception, *callback); }); } +void OfflineManager::runPackDatabaseAutomatically(jni::JNIEnv&, jboolean autopack) { + fileSource->runPackDatabaseAutomatically(autopack); +} + // FileSource::FileSourceCallback // void OfflineManager::FileSourceCallback::onSuccess(jni::JNIEnv& env, @@ -211,6 +215,7 @@ void OfflineManager::registerNative(jni::JNIEnv& env) { METHOD(&OfflineManager::invalidateAmbientCache, "nativeInvalidateAmbientCache"), METHOD(&OfflineManager::clearAmbientCache, "nativeClearAmbientCache"), METHOD(&OfflineManager::setMaximumAmbientCacheSize, "nativeSetMaximumAmbientCacheSize"), + METHOD(&OfflineManager::runPackDatabaseAutomatically, "runPackDatabaseAutomatically"), METHOD(&OfflineManager::putResourceWithUrl, "putResourceWithUrl")); } diff --git a/platform/android/src/offline/offline_manager.hpp b/platform/android/src/offline/offline_manager.hpp index 64e00f91fc2..0af92f81158 100644 --- a/platform/android/src/offline/offline_manager.hpp +++ b/platform/android/src/offline/offline_manager.hpp @@ -103,6 +103,8 @@ class OfflineManager { void setMaximumAmbientCacheSize(jni::JNIEnv&, const jni::jlong size, const jni::Object& callback_); + void runPackDatabaseAutomatically(jni::JNIEnv&, jboolean autopack); + private: std::shared_ptr fileSource; }; diff --git a/platform/android/src/offline/offline_region.cpp b/platform/android/src/offline/offline_region.cpp index 264b8216364..4276ce8f45f 100644 --- a/platform/android/src/offline/offline_region.cpp +++ b/platform/android/src/offline/offline_region.cpp @@ -101,31 +101,23 @@ void OfflineRegion::getOfflineRegionStatus(jni::JNIEnv& env_, const jni::Object< }); } -namespace { -// Reattach, the callback comes from a different thread -void handleException(std::exception_ptr exception, - const jni::Object& callback, - android::UniqueEnv env = android::AttachEnv()) { - if (exception) { - OfflineRegion::OfflineRegionDeleteCallback::onError(*env, callback, exception); - } else { - OfflineRegion::OfflineRegionDeleteCallback::onDelete(*env, callback); - } -} -} // namespace - -void OfflineRegion::deleteOfflineRegion(jni::JNIEnv& env_, - jni::jboolean pack, - const jni::Object& callback_) { +void OfflineRegion::deleteOfflineRegion(jni::JNIEnv& env_, const jni::Object& callback_) { auto globalCallback = jni::NewGlobal(env_, callback_); - fileSource->deleteOfflineRegion( - std::move(*region), - [ - // Ensure the object is not gc'd in the meanwhile - callback = std::make_shared(std::move(globalCallback))]( - std::exception_ptr error) mutable { handleException(error, *callback); }, - pack); + fileSource->deleteOfflineRegion(std::move(*region), + [ + // Ensure the object is not gc'd in the meanwhile + callback = std::make_shared( + std::move(globalCallback))](std::exception_ptr error) mutable { + // Reattach, the callback comes from a different thread + android::UniqueEnv env = android::AttachEnv(); + + if (error) { + OfflineRegionDeleteCallback::onError(*env, *callback, error); + } else { + OfflineRegionDeleteCallback::onDelete(*env, *callback); + } + }); } void OfflineRegion::invalidateOfflineRegion(jni::JNIEnv& env_, diff --git a/platform/android/src/offline/offline_region.hpp b/platform/android/src/offline/offline_region.hpp index 2c92951f696..dda253469e0 100644 --- a/platform/android/src/offline/offline_region.hpp +++ b/platform/android/src/offline/offline_region.hpp @@ -69,7 +69,7 @@ class OfflineRegion { void getOfflineRegionStatus(jni::JNIEnv&, const jni::Object&); - void deleteOfflineRegion(jni::JNIEnv&, jni::jboolean pack, const jni::Object&); + void deleteOfflineRegion(jni::JNIEnv&, const jni::Object&); void invalidateOfflineRegion(jni::JNIEnv&, const jni::Object&); diff --git a/platform/default/include/mbgl/storage/offline_database.hpp b/platform/default/include/mbgl/storage/offline_database.hpp index ac997bf6adb..67a19fcf265 100644 --- a/platform/default/include/mbgl/storage/offline_database.hpp +++ b/platform/default/include/mbgl/storage/offline_database.hpp @@ -74,7 +74,7 @@ class OfflineDatabase : private util::noncopyable { expected updateMetadata(const int64_t regionID, const OfflineRegionMetadata&); - std::exception_ptr deleteRegion(OfflineRegion&&, bool pack = true); + std::exception_ptr deleteRegion(OfflineRegion&&); std::exception_ptr invalidateRegion(int64_t regionID); // Return value is (response, stored size) @@ -94,6 +94,7 @@ class OfflineDatabase : private util::noncopyable { bool exceedsOfflineMapboxTileCountLimit(const Resource&); void markUsedResources(int64_t regionID, const std::list&); std::exception_ptr pack(); + void runPackDatabaseAutomatically(bool autopack_) { autopack = autopack_; } private: void initialize(); @@ -149,6 +150,7 @@ class OfflineDatabase : private util::noncopyable { optional offlineMapboxTileCount; bool evict(uint64_t neededFreeSize); + bool autopack = true; }; } // namespace mbgl diff --git a/platform/default/src/mbgl/storage/default_file_source.cpp b/platform/default/src/mbgl/storage/default_file_source.cpp index 36acb748de0..e6cdb411bb3 100644 --- a/platform/default/src/mbgl/storage/default_file_source.cpp +++ b/platform/default/src/mbgl/storage/default_file_source.cpp @@ -82,9 +82,9 @@ class DefaultFileSource::Impl { } } - void deleteRegion(OfflineRegion&& region, std::function callback, bool pack) { + void deleteRegion(OfflineRegion&& region, std::function callback) { downloads.erase(region.getID()); - callback(offlineDatabase->deleteRegion(std::move(region), pack)); + callback(offlineDatabase->deleteRegion(std::move(region))); } void invalidateRegion(int64_t regionID, std::function callback) { @@ -208,6 +208,8 @@ class DefaultFileSource::Impl { void packDatabase(std::function callback) { callback(offlineDatabase->pack()); } + void runPackDatabaseAutomatically(bool autopack) { offlineDatabase->runPackDatabaseAutomatically(autopack); } + private: expected getDownload(int64_t regionID) { auto it = downloads.find(regionID); @@ -316,10 +318,8 @@ void DefaultFileSource::updateOfflineMetadata(const int64_t regionID, impl->actor().invoke(&Impl::updateMetadata, regionID, metadata, callback); } -void DefaultFileSource::deleteOfflineRegion(OfflineRegion&& region, - std::function callback, - bool pack) { - impl->actor().invoke(&Impl::deleteRegion, std::move(region), callback, pack); +void DefaultFileSource::deleteOfflineRegion(OfflineRegion&& region, std::function callback) { + impl->actor().invoke(&Impl::deleteRegion, std::move(region), callback); } void DefaultFileSource::invalidateOfflineRegion(OfflineRegion& region, @@ -363,6 +363,10 @@ void DefaultFileSource::packDatabase(std::function cal impl->actor().invoke(&Impl::packDatabase, std::move(callback)); } +void DefaultFileSource::runPackDatabaseAutomatically(bool autopack) { + impl->actor().invoke(&Impl::runPackDatabaseAutomatically, autopack); +} + void DefaultFileSource::invalidateAmbientCache(std::function callback) { impl->actor().invoke(&Impl::invalidateAmbientCache, std::move(callback)); } diff --git a/platform/default/src/mbgl/storage/offline_database.cpp b/platform/default/src/mbgl/storage/offline_database.cpp index fef524ce570..974815191b0 100644 --- a/platform/default/src/mbgl/storage/offline_database.cpp +++ b/platform/default/src/mbgl/storage/offline_database.cpp @@ -154,7 +154,7 @@ void OfflineDatabase::removeExisting() { void OfflineDatabase::removeOldCacheTable() { assert(db); db->exec("DROP TABLE IF EXISTS http_cache"); - vacuum(); + if (autopack) vacuum(); } void OfflineDatabase::createSchema() { @@ -705,7 +705,7 @@ std::exception_ptr OfflineDatabase::clearAmbientCache() try { resourceQuery.run(); - vacuum(); + if (autopack) vacuum(); return nullptr; } catch (...) { @@ -884,7 +884,7 @@ OfflineDatabase::updateMetadata(const int64_t regionID, const OfflineRegionMetad return unexpected(std::current_exception()); } -std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region, bool pack) try { +std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region) try { { mapbox::sqlite::Query query{ getStatement("DELETE FROM regions WHERE id = ?") }; query.bind(1, region.getID()); @@ -893,7 +893,7 @@ std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region, bool pa evict(0); assert(db); - if (pack) vacuum(); + if (autopack) vacuum(); // Ensure that the cached offlineTileCount value is recalculated. offlineMapboxTileCount = nullopt; @@ -1235,7 +1235,7 @@ std::exception_ptr OfflineDatabase::setMaximumAmbientCacheSize(uint64_t size) { if (databaseSize > maximumAmbientCacheSize) { evict(0); - vacuum(); + if (autopack) vacuum(); } return nullptr; diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp index 9d798d0a627..ef7ad257d74 100644 --- a/test/storage/offline_database.test.cpp +++ b/test/storage/offline_database.test.cpp @@ -701,7 +701,8 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DeleteRegion)) { db.putRegionResources(region2->getID(), generateResources("mapbox://tile_2", "mapbox://style_2"), status); const size_t sizeWithTwoRegions = util::read_file(filename).size(); - db.deleteRegion(std::move(*region1), false /*pack*/); + db.runPackDatabaseAutomatically(false); + db.deleteRegion(std::move(*region1)); ASSERT_EQ(1u, db.listRegions().value().size()); // Region is removed but the size of the database is the same. @@ -711,7 +712,7 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DeleteRegion)) { // The size of the database has shrunk after pack(). const size_t sizeWithOneRegion = util::read_file(filename).size(); EXPECT_LT(sizeWithOneRegion, sizeWithTwoRegions); - + db.runPackDatabaseAutomatically(true); db.deleteRegion(std::move(*region2)); // The size of the database has shrunk right away. const size_t sizeWithoutRegions = util::read_file(filename).size(); @@ -732,6 +733,36 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DeleteRegion)) { EXPECT_EQ(0u, log.uncheckedCount()); } +TEST(OfflineDatabase, TEST_REQUIRES_WRITE(Pack)) { + FixtureLog log; + deleteDatabaseFiles(); + + OfflineDatabase db(filename); + size_t initialSize = util::read_file(filename).size(); + db.runPackDatabaseAutomatically(false); + + Response response; + response.data = randomString(.5 * 1024 * 1024); + + for (unsigned i = 0; i < 50; ++i) { + const Resource tile = Resource::tile("mapbox://tile_" + std::to_string(i), 1, 0, 0, 0, Tileset::Scheme::XYZ); + db.put(tile, response); + + const Resource style = Resource::style("mapbox://style_" + std::to_string(i)); + db.put(style, response); + } + size_t populatedSize = util::read_file(filename).size(); + ASSERT_GT(populatedSize, initialSize); + + db.clearAmbientCache(); + EXPECT_EQ(populatedSize, util::read_file(filename).size()); + EXPECT_EQ(0u, log.uncheckedCount()); + + db.pack(); + EXPECT_EQ(initialSize, util::read_file(filename).size()); + EXPECT_EQ(0u, log.uncheckedCount()); +} + TEST(OfflineDatabase, MapboxTileLimitExceeded) { FixtureLog log;