[core] OfflineDatabase pack API

- introduce `OfflineDatabase::pack()` standing for incremental vacuum
- make pack optional at offline region deletion
- update `OfflineDatabase.DeleteRegion` test accordingly to the items above

3 files changed, 52 insertions, 15 deletions

diff --git a/platform/default/include/mbgl/storage/offline_database.hpp b/platform/default/include/mbgl/storage/offline_database.hpp
index e599094a6d..8065dcaa49 100644
--- a/platform/default/include/mbgl/storage/offline_database.hpp
+++ b/platform/default/include/mbgl/storage/offline_database.hpp
@@ -74,7 +74,7 @@ public:
     expected<OfflineRegionMetadata, std::exception_ptr>
     updateMetadata(const int64_t regionID, const OfflineRegionMetadata&);
 
-    std::exception_ptr deleteRegion(OfflineRegion&&);
+    std::exception_ptr deleteRegion(OfflineRegion&&, bool skipPack = false);
     std::exception_ptr invalidateRegion(int64_t regionID);
 
     // Return value is (response, stored size)
@@ -93,6 +93,7 @@ public:
     uint64_t getOfflineMapboxTileCount();
     bool exceedsOfflineMapboxTileCountLimit(const Resource&);
     void markUsedResources(int64_t regionID, const std::list<Resource>&);
+    std::exception_ptr pack();
 
 private:
     void initialize();
diff --git a/platform/default/src/mbgl/storage/offline_database.cpp b/platform/default/src/mbgl/storage/offline_database.cpp
index 5aa5738f41..550e4c8427 100644
--- a/platform/default/src/mbgl/storage/offline_database.cpp
+++ b/platform/default/src/mbgl/storage/offline_database.cpp
@@ -183,6 +183,7 @@ void OfflineDatabase::migrateToVersion6() {
 }
 
 void OfflineDatabase::vacuum() {
+    assert(db);
     if (getPragma<int64_t>("PRAGMA auto_vacuum") != 2 /*INCREMENTAL*/) {
         db->exec("PRAGMA auto_vacuum = INCREMENTAL");
         db->exec("VACUUM");
@@ -872,7 +873,7 @@ OfflineDatabase::updateMetadata(const int64_t regionID, const OfflineRegionMetad
     return unexpected<std::exception_ptr>(std::current_exception());
 }
 
-std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region) try {
+std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region, bool skipPack) try {
     {
         mapbox::sqlite::Query query{ getStatement("DELETE FROM regions WHERE id = ?") };
         query.bind(1, region.getID());
@@ -881,10 +882,10 @@ std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region) try {
 
     evict(0);
     assert(db);
-    vacuum();
+    if (!skipPack) vacuum();
 
     // Ensure that the cached offlineTileCount value is recalculated.
-    offlineMapboxTileCount = {};
+    offlineMapboxTileCount = nullopt;
     return nullptr;
 } catch (const mapbox::sqlite::Exception& ex) {
     handleError(ex, "delete region");
@@ -1298,6 +1299,15 @@ void OfflineDatabase::markUsedResources(int64_t regionID, const std::list<Resour
     handleError(ex, "mark resources as used");
 }
 
+std::exception_ptr OfflineDatabase::pack() try {
+    if (!db) initialize();
+    vacuum();
+    return nullptr;
+} catch (const mapbox::sqlite::Exception& ex) {
+    handleError(ex, "pack storage");
+    return std::current_exception();
+}
+
 std::exception_ptr OfflineDatabase::resetDatabase() try {
     removeExisting();
     initialize();
diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp
index 7b5255c9f5..90b79ec612 100644
--- a/test/storage/offline_database.test.cpp
+++ b/test/storage/offline_database.test.cpp
@@ -658,6 +658,22 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DISABLED_MaximumAmbientCacheSize)) {
     EXPECT_EQ(initialSize, util::read_file(filename).size());
 }
 
+namespace {
+std::list<std::tuple<Resource, Response>> generateResources(const std::string& tilePrefix,
+                                                            const std::string& stylePrefix) {
+    static const auto responseData = randomString(.5 * 1024 * 1024);
+    Response response;
+    response.data = responseData;
+    std::list<std::tuple<Resource, Response>> resources;
+    for (unsigned i = 0; i < 50; ++i) {
+        resources.emplace_back(Resource::tile(tilePrefix + std::to_string(i), 1, 0, 0, 0, Tileset::Scheme::XYZ),
+                               response);
+        resources.emplace_back(Resource::style(stylePrefix + std::to_string(i)), response);
+    }
+    return resources;
+}
+} // namespace
+
 TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DeleteRegion)) {
     FixtureLog log;
     deleteDatabaseFiles();
@@ -677,25 +693,35 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DeleteRegion)) {
         OfflineTilePyramidRegionDefinition definition{ "mapbox://style", LatLngBounds::hull({1, 2}, {3, 4}), 5, 6, 2.0, true };
         OfflineRegionMetadata metadata{{ 1, 2, 3 }};
 
-        auto region = db.createRegion(definition, metadata);
+        auto region1 = db.createRegion(definition, metadata);
+        auto region2 = db.createRegion(definition, metadata);
 
-        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.putRegionResource(region->getID(), tile, response);
+        OfflineRegionStatus status;
+        db.putRegionResources(region1->getID(), generateResources("mapbox://tile_1", "mapbox://style_1"), status);
+        db.putRegionResources(region2->getID(), generateResources("mapbox://tile_2", "mapbox://style_2"), status);
+        const size_t sizeWithTwoRegions = util::read_file(filename).size();
 
-            const Resource style = Resource::style("mapbox://style_" + std::to_string(i));
-            db.putRegionResource(region->getID(), style, response);
-        }
+        db.deleteRegion(std::move(*region1), true /*skipPack*/);
 
-        db.deleteRegion(std::move(*region));
+        ASSERT_EQ(1u, db.listRegions().value().size());
+        // Region is removed but the size of the database is the same.
+        EXPECT_EQ(sizeWithTwoRegions, util::read_file(filename).size());
 
-        auto regions = db.listRegions().value();
-        ASSERT_EQ(0u, regions.size());
+        db.pack();
+        // The size of the database has shrunk after pack().
+        const size_t sizeWithOneRegion = util::read_file(filename).size();
+        EXPECT_LT(sizeWithOneRegion, sizeWithTwoRegions);
+
+        db.deleteRegion(std::move(*region2));
+        // The size of the database has shrunk right away.
+        const size_t sizeWithoutRegions = util::read_file(filename).size();
+        ASSERT_EQ(0u, db.listRegions().value().size());
+        EXPECT_LT(sizeWithoutRegions, sizeWithOneRegion);
 
         // The tiles from the offline region will migrate to the
         // ambient cache and shrink the database to the maximum
         // size defined by default.
-        EXPECT_LE(util::read_file(filename).size(), util::DEFAULT_MAX_CACHE_SIZE);
+        EXPECT_LE(sizeWithoutRegions, util::DEFAULT_MAX_CACHE_SIZE);
 
         // After clearing the cache, the size of the database
         // should get back to the original size.