From f84e46ad2690e3914efe830682b41dd55dc945f3 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Thu, 29 Aug 2019 14:39:57 +0300 Subject: [core] Mark offline region resources in batches --- .../include/mbgl/storage/offline_database.hpp | 4 +-- .../include/mbgl/storage/offline_download.hpp | 2 ++ .../default/src/mbgl/storage/offline_database.cpp | 20 +++--------- .../default/src/mbgl/storage/offline_download.cpp | 31 +++++++++++++----- test/storage/offline_database.test.cpp | 38 ++++++++++------------ 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> getRegionResource(int64_t regionID, const Resource&); - optional hasRegionResource(int64_t regionID, const Resource&); + optional> getRegionResource(const Resource&); + optional hasRegionResource(const Resource&); uint64_t putRegionResource(int64_t regionID, const Resource&, const Response&); void putRegionResources(int64_t regionID, const std::list>&, 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> requests; std::unordered_set requiredSourceURLs; std::deque resourcesRemaining; + std::list resourcesToBeMarkedAsUsed; std::list> 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> OfflineDatabase::getRegionResource(int64_t regionID, const Resource& resource) try { - auto response = getInternal(resource); - - if (response) { - markUsed(regionID, resource); - } - - return response; +optional> OfflineDatabase::getRegionResource(const Resource& resource) try { + return getInternal(resource); } catch (const mapbox::sqlite::Exception& ex) { handleError(ex, "read region resource"); return nullopt; } -optional OfflineDatabase::hasRegionResource(int64_t regionID, const Resource& resource) try { - auto response = hasInternal(resource); - - if (response) { - markUsed(regionID, resource); - } - - return response; +optional 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 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 { + optional result; if (!callback) { - return offlineDatabase.hasRegionResource(id, resource); - } - optional> response = offlineDatabase.getRegionResource(id, resource); - if (!response) { - return {}; + result = offlineDatabase.hasRegionResource(resource); + } else { + optional> 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 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("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) { -- cgit v1.2.1