[core] Mark offline region resources in batchesupstream/mikhail_offline_download

5 files changed, 48 insertions, 47 deletions

diff --git a/platform/default/include/mbgl/storage/offline_database.hpp b/platform/default/include/mbgl/storage/offline_database.hpp
index fe286575a4..e19dcfade9 100644
--- a/platform/default/include/mbgl/storage/offline_database.hpp
+++ b/platform/default/include/mbgl/storage/offline_database.hpp
@@ -78,8 +78,8 @@ public:
     std::exception_ptr invalidateRegion(int64_t regionID);
 
     // Return value is (response, stored size)
-    optional<std::pair<Response, uint64_t>> getRegionResource(int64_t regionID, const Resource&);
-    optional<int64_t> hasRegionResource(int64_t regionID, const Resource&);
+    optional<std::pair<Response, uint64_t>> getRegionResource(const Resource&);
+    optional<int64_t> hasRegionResource(const Resource&);
     uint64_t putRegionResource(int64_t regionID, const Resource&, const Response&);
     void putRegionResources(int64_t regionID, const std::list<std::tuple<Resource, Response>>&, OfflineRegionStatus&);
 
diff --git a/platform/default/include/mbgl/storage/offline_download.hpp b/platform/default/include/mbgl/storage/offline_download.hpp
index fe5d26f9eb..53b42ae9d1 100644
--- a/platform/default/include/mbgl/storage/offline_download.hpp
+++ b/platform/default/include/mbgl/storage/offline_download.hpp
@@ -60,10 +60,12 @@ private:
     std::list<std::unique_ptr<AsyncRequest>> requests;
     std::unordered_set<std::string> requiredSourceURLs;
     std::deque<Resource> resourcesRemaining;
+    std::list<Resource> resourcesToBeMarkedAsUsed;
     std::list<std::tuple<Resource, Response>> buffer;
 
     void queueResource(Resource&&);
     void queueTiles(style::SourceType, uint16_t tileSize, const Tileset&);
+    void markPendingUsedResources();
 };
 
 } // namespace mbgl
diff --git a/platform/default/src/mbgl/storage/offline_database.cpp b/platform/default/src/mbgl/storage/offline_database.cpp
index 31442c1bde..83eea7bcc4 100644
--- a/platform/default/src/mbgl/storage/offline_database.cpp
+++ b/platform/default/src/mbgl/storage/offline_database.cpp
@@ -877,27 +877,15 @@ std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region) try {
     return std::current_exception();
 }
 
-optional<std::pair<Response, uint64_t>> OfflineDatabase::getRegionResource(int64_t regionID, const Resource& resource) try {
-    auto response = getInternal(resource);
-
-    if (response) {
-        markUsed(regionID, resource);
-    }
-
-    return response;
+optional<std::pair<Response, uint64_t>> OfflineDatabase::getRegionResource(const Resource& resource) try {
+    return getInternal(resource);
 } catch (const mapbox::sqlite::Exception& ex) {
     handleError(ex, "read region resource");
     return nullopt;
 }
 
-optional<int64_t> OfflineDatabase::hasRegionResource(int64_t regionID, const Resource& resource) try {
-    auto response = hasInternal(resource);
-
-    if (response) {
-        markUsed(regionID, resource);
-    }
-
-    return response;
+optional<int64_t> OfflineDatabase::hasRegionResource(const Resource& resource) try {
+    return hasInternal(resource);
 } catch (const mapbox::sqlite::Exception& ex) {
     handleError(ex, "query region resource");
     return nullopt;
diff --git a/platform/default/src/mbgl/storage/offline_download.cpp b/platform/default/src/mbgl/storage/offline_download.cpp
index e27e645a13..d28c849541 100644
--- a/platform/default/src/mbgl/storage/offline_download.cpp
+++ b/platform/default/src/mbgl/storage/offline_download.cpp
@@ -353,10 +353,13 @@ void OfflineDownload::activateDownload() {
 */
 void OfflineDownload::continueDownload() {
     if (resourcesRemaining.empty() && status.complete()) {
+        markPendingUsedResources();
         setState(OfflineRegionDownloadState::Inactive);
         return;
     }
 
+    if (resourcesToBeMarkedAsUsed.size() >= 200) markPendingUsedResources();
+
     while (!resourcesRemaining.empty() && requests.size() < onlineFileSource.getMaximumConcurrentRequests()) {
         ensureResource(std::move(resourcesRemaining.front()));
         resourcesRemaining.pop_front();
@@ -391,6 +394,11 @@ void OfflineDownload::queueTiles(SourceType type, uint16_t tileSize, const Tiles
     });
 }
 
+void OfflineDownload::markPendingUsedResources() {
+    offlineDatabase.markUsedResources(id, resourcesToBeMarkedAsUsed);
+    resourcesToBeMarkedAsUsed.clear();
+}
+
 void OfflineDownload::ensureResource(Resource&& resource,
                                      std::function<void(Response)> callback) {
     assert(resource.priority == Resource::Priority::Low);
@@ -399,24 +407,29 @@ void OfflineDownload::ensureResource(Resource&& resource,
     auto workRequestsIt = requests.insert(requests.begin(), nullptr);
     *workRequestsIt = util::RunLoop::Get()->invokeCancellable([=]() {
         requests.erase(workRequestsIt);
-
+        const auto resourceKind = resource.kind;
         auto getResourceSizeInDatabase = [&] () -> optional<int64_t> {
+            optional<int64_t> result;
             if (!callback) {
-                return offlineDatabase.hasRegionResource(id, resource);
-            }
-            optional<std::pair<Response, uint64_t>> response = offlineDatabase.getRegionResource(id, resource);
-            if (!response) {
-                return {};
+                result = offlineDatabase.hasRegionResource(resource);
+            } else {
+                optional<std::pair<Response, uint64_t>> response = offlineDatabase.getRegionResource(resource);
+                if (response) {
+                    callback(response->first);
+                    result = response->second;
+                }
             }
-            callback(response->first);
-            return response->second;
+
+            if (result) resourcesToBeMarkedAsUsed.emplace_back(std::move(resource));
+            return result;
         };
 
         optional<int64_t> offlineResponse = getResourceSizeInDatabase();
         if (offlineResponse) {
+            assert(!resourcesToBeMarkedAsUsed.empty());
             status.completedResourceCount++;
             status.completedResourceSize += *offlineResponse;
-            if (resource.kind == Resource::Kind::Tile) {
+            if (resourceKind == Resource::Kind::Tile) {
                 status.completedTileCount += 1;
                 status.completedTileSize += *offlineResponse;
             }
diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp
index 3f6db527d4..24234b0624 100644
--- a/test/storage/offline_database.test.cpp
+++ b/test/storage/offline_database.test.cpp
@@ -1093,8 +1093,8 @@ TEST(OfflineDatabase, HasRegionResource) {
     auto region = db.createRegion(definition, OfflineRegionMetadata());
     ASSERT_TRUE(region);
 
-    EXPECT_FALSE(bool(db.hasRegionResource(region->getID(), Resource::style("http://example.com/1"))));
-    EXPECT_FALSE(bool(db.hasRegionResource(region->getID(), Resource::style("http://example.com/20"))));
+    EXPECT_FALSE(bool(db.hasRegionResource(Resource::style("http://example.com/1"))));
+    EXPECT_FALSE(bool(db.hasRegionResource(Resource::style("http://example.com/20"))));
 
     Response response;
     response.data = randomString(1024);
@@ -1103,9 +1103,9 @@ TEST(OfflineDatabase, HasRegionResource) {
         db.putRegionResource(region->getID(), Resource::style("http://example.com/"s + util::toString(i)), response);
     }
 
-    EXPECT_TRUE(bool(db.hasRegionResource(region->getID(), Resource::style("http://example.com/1"))));
-    EXPECT_TRUE(bool(db.hasRegionResource(region->getID(), Resource::style("http://example.com/20"))));
-    EXPECT_EQ(1024, *(db.hasRegionResource(region->getID(), Resource::style("http://example.com/20"))));
+    EXPECT_TRUE(bool(db.hasRegionResource(Resource::style("http://example.com/1"))));
+    EXPECT_TRUE(bool(db.hasRegionResource(Resource::style("http://example.com/20"))));
+    EXPECT_EQ(1024, *(db.hasRegionResource(Resource::style("http://example.com/20"))));
 
     EXPECT_EQ(0u, log.uncheckedCount());
 }
@@ -1131,16 +1131,16 @@ TEST(OfflineDatabase, HasRegionResourceTile) {
 
     response.data = std::make_shared<std::string>("first");
 
-    EXPECT_FALSE(bool(db.hasRegionResource(region->getID(), resource)));
+    EXPECT_FALSE(bool(db.hasRegionResource(resource)));
     db.putRegionResource(region->getID(), resource, response);
-    EXPECT_TRUE(bool(db.hasRegionResource(region->getID(), resource)));
-    EXPECT_EQ(5, *(db.hasRegionResource(region->getID(), resource)));
+    EXPECT_TRUE(bool(db.hasRegionResource(resource)));
+    EXPECT_EQ(5, *(db.hasRegionResource(resource)));
 
     auto anotherRegion = db.createRegion(definition, OfflineRegionMetadata());
     ASSERT_TRUE(anotherRegion);
     EXPECT_LT(region->getID(), anotherRegion->getID());
-    EXPECT_TRUE(bool(db.hasRegionResource(anotherRegion->getID(), resource)));
-    EXPECT_EQ(5, *(db.hasRegionResource(anotherRegion->getID(), resource)));
+    EXPECT_TRUE(bool(db.hasRegionResource(resource)));
+    EXPECT_EQ(5, *(db.hasRegionResource(resource)));
 
     EXPECT_EQ(0u, log.uncheckedCount());
 
@@ -1488,12 +1488,12 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(DisallowedIO)) {
     EXPECT_EQ(1u, log.count(warning(ResultCode::Auth, "Can't update region metadata: authorization denied")));
     EXPECT_EQ(0u, log.uncheckedCount());
 
-    EXPECT_EQ(nullopt, db.getRegionResource(region->getID(), fixture::resource));
+    EXPECT_EQ(nullopt, db.getRegionResource(fixture::resource));
     EXPECT_EQ(1u, log.count(warning(ResultCode::Auth, "Can't update timestamp: authorization denied")));
     EXPECT_EQ(1u, log.count(warning(ResultCode::Auth, "Can't read region resource: authorization denied")));
     EXPECT_EQ(0u, log.uncheckedCount());
 
-    EXPECT_EQ(nullopt, db.hasRegionResource(region->getID(), fixture::resource));
+    EXPECT_EQ(nullopt, db.hasRegionResource(fixture::resource));
     EXPECT_EQ(1u, log.count(warning(ResultCode::Auth, "Can't query region resource: authorization denied")));
     EXPECT_EQ(0u, log.uncheckedCount());
 
@@ -1566,7 +1566,7 @@ TEST(OfflineDatabase, TEST_REQUIRES_WRITE(MergeDatabaseWithSingleRegion_Update))
         EXPECT_EQ(1u, status->completedTileCount);
 
         //Verify the modified timestamp matches the tile in the sideloaded db.
-        auto updatedTile = db.getRegionResource(regionId,
+        auto updatedTile = db.getRegionResource(
             Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.webp",
                 1, 0, 0, 1, Tileset::Scheme::XYZ));
         EXPECT_EQ(Timestamp{ Seconds(1520409600) }, *(updatedTile->first.modified));
@@ -1586,8 +1586,7 @@ TEST(OfflineDatabase, MergeDatabaseWithSingleRegion_NoUpdate) {
     EXPECT_EQ(1u, result->size());
     EXPECT_EQ(1u, db.listRegions()->size());
 
-    auto regionId = result->front().getID();
-    auto updatedTile = db.getRegionResource(regionId,
+    auto updatedTile = db.getRegionResource(
         Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.webp",
             1, 0, 0, 1, Tileset::Scheme::XYZ));
 
@@ -1601,14 +1600,13 @@ TEST(OfflineDatabase, MergeDatabaseWithSingleRegion_AmbientTiles) {
 
     OfflineDatabase db(":memory:");
     auto result = db.mergeDatabase(filename_sideload);
-    auto regionId = result->front().getID();
 
-    EXPECT_TRUE(bool(db.hasRegionResource(regionId, Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", 1, 0, 0, 1, Tileset::Scheme::XYZ))));
+    EXPECT_TRUE(bool(db.hasRegionResource(Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", 1, 0, 0, 1, Tileset::Scheme::XYZ))));
 
     //Ambient resources should not be copied
-    EXPECT_FALSE(bool(db.hasRegionResource(regionId, Resource::style("mapbox://styles/mapbox/streets-v9"))));
-    EXPECT_FALSE(bool(db.hasRegionResource(regionId, Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", 1, 0, 1, 2, Tileset::Scheme::XYZ))));
-    EXPECT_FALSE(bool(db.hasRegionResource(regionId, Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", 1, 1, 1, 2, Tileset::Scheme::XYZ))));
+    EXPECT_FALSE(bool(db.hasRegionResource(Resource::style("mapbox://styles/mapbox/streets-v9"))));
+    EXPECT_FALSE(bool(db.hasRegionResource(Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", 1, 0, 1, 2, Tileset::Scheme::XYZ))));
+    EXPECT_FALSE(bool(db.hasRegionResource(Resource::tile("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", 1, 1, 1, 2, Tileset::Scheme::XYZ))));
 }
 
 TEST(OfflineDatabase, MergeDatabaseWithMultipleRegions_New) {