diff --git a/platform/default/mbgl/storage/offline_database.cpp b/platform/default/mbgl/storage/offline_database.cpp index 65c20971827..fae7938501f 100644 --- a/platform/default/mbgl/storage/offline_database.cpp +++ b/platform/default/mbgl/storage/offline_database.cpp @@ -9,6 +9,109 @@ #include "sqlite3.hpp" namespace mbgl { + +// Manages transactions for (multiple) region downloads +// and ad-hoc caching +class TransactionManager { +public: + TransactionManager(mapbox::sqlite::Database& database_) + : database(database_) { + } + + ~TransactionManager() { + commitTransaction(); + } + + void startIsolated() { + commitTransaction(); + openTransaction(); + } + + void endIsolated() { + commitTransaction(); + if (runningBatches > 0) { + openTransaction(); + } + } + + void startBatch() { + runningBatches++; + openTransaction(); + }; + + void endBatch() { + runningBatches--; + if (runningBatches == 0 && pendingResources > 0) { + commitTransaction(); + } + }; + + void onBatchResourceAdded() { + assert (runningBatches > 0); + + pendingResources++; + if (pendingResources >= 128) { + commitTransaction(); + openTransaction(); + } + } + +private: + void openTransaction() { + if (!transaction) { + transaction = std::make_unique(database, mapbox::sqlite::Transaction::Immediate); + } + } + + void commitTransaction() { + if (!transaction) { + return; + } + + transaction->commit(); + transaction.reset(); + pendingResources = 0; + } + + mapbox::sqlite::Database& database; + std::unique_ptr transaction; + int runningBatches = 0; + int64_t pendingResources = 0; +}; + +// Enables reference counting on current batch +// transactions +class BatchTransaction { +public: + + BatchTransaction(TransactionManager& manager_) + : manager(manager_) { + manager.startBatch(); + } + + ~BatchTransaction() { + manager.endBatch(); + } + +private: + TransactionManager& manager; +}; + +// Ensures an isolated transaction. +// Any current transactions are committed first. +class IsolatedTransaction { +public: + IsolatedTransaction(TransactionManager& manager_) + : manager(manager_) { + manager.startIsolated(); + } + + ~IsolatedTransaction() { + manager.endIsolated(); + } +private: + TransactionManager& manager; +}; OfflineDatabase::Statement::~Statement() { stmt.reset(); @@ -25,6 +128,7 @@ OfflineDatabase::~OfflineDatabase() { // Deleting these SQLite objects may result in exceptions, but we're in a destructor, so we // can't throw anything. try { + transactionManager.reset(); statements.clear(); db.reset(); } catch (mapbox::sqlite::Exception& ex) { @@ -33,9 +137,11 @@ OfflineDatabase::~OfflineDatabase() { } void OfflineDatabase::connect(int flags) { + transactionManager.reset(); db = std::make_unique(path.c_str(), flags); db->setBusyTimeout(Milliseconds::max()); db->exec("PRAGMA foreign_keys = ON"); + transactionManager = std::make_unique(*db); } void OfflineDatabase::ensureSchema() { @@ -128,11 +234,10 @@ void OfflineDatabase::migrateToVersion5() { } void OfflineDatabase::migrateToVersion6() { - mapbox::sqlite::Transaction transaction(*db); + IsolatedTransaction transaction(*transactionManager); db->exec("ALTER TABLE resources ADD COLUMN must_revalidate INTEGER NOT NULL DEFAULT 0"); db->exec("ALTER TABLE tiles ADD COLUMN must_revalidate INTEGER NOT NULL DEFAULT 0"); db->exec("PRAGMA user_version = 6"); - transaction.commit(); } OfflineDatabase::Statement OfflineDatabase::getStatement(const char * sql) { @@ -169,6 +274,7 @@ optional OfflineDatabase::hasInternal(const Resource& resource) { } std::pair OfflineDatabase::put(const Resource& resource, const Response& response) { + IsolatedTransaction transaction(*transactionManager); return putInternal(resource, response, true); } @@ -291,10 +397,6 @@ bool OfflineDatabase::putResource(const Resource& resource, // We can't use REPLACE because it would change the id value. - // Begin an immediate-mode transaction to ensure that two writers do not attempt - // to INSERT a resource at the same moment. - mapbox::sqlite::Transaction transaction(*db, mapbox::sqlite::Transaction::Immediate); - // clang-format off Statement update = getStatement( "UPDATE resources " @@ -327,7 +429,6 @@ bool OfflineDatabase::putResource(const Resource& resource, update->run(); if (update->changes() != 0) { - transaction.commit(); return false; } @@ -354,7 +455,6 @@ bool OfflineDatabase::putResource(const Resource& resource, } insert->run(); - transaction.commit(); return true; } @@ -480,10 +580,6 @@ bool OfflineDatabase::putTile(const Resource::TileData& tile, // We can't use REPLACE because it would change the id value. - // Begin an immediate-mode transaction to ensure that two writers do not attempt - // to INSERT a resource at the same moment. - mapbox::sqlite::Transaction transaction(*db, mapbox::sqlite::Transaction::Immediate); - // clang-format off Statement update = getStatement( "UPDATE tiles " @@ -522,7 +618,6 @@ bool OfflineDatabase::putTile(const Resource::TileData& tile, update->run(); if (update->changes() != 0) { - transaction.commit(); return false; } @@ -551,12 +646,16 @@ bool OfflineDatabase::putTile(const Resource::TileData& tile, insert->bind(12, compressed); } + insert->run(); - transaction.commit(); return true; } +std::shared_ptr OfflineDatabase::beginRegionDownload() { + return std::make_shared(*transactionManager); +} + std::vector OfflineDatabase::listRegions() { // clang-format off Statement stmt = getStatement( @@ -577,6 +676,8 @@ std::vector OfflineDatabase::listRegions() { OfflineRegion OfflineDatabase::createRegion(const OfflineRegionDefinition& definition, const OfflineRegionMetadata& metadata) { + IsolatedTransaction transaction(*transactionManager); + // clang-format off Statement stmt = getStatement( "INSERT INTO regions (definition, description) " @@ -591,6 +692,8 @@ OfflineRegion OfflineDatabase::createRegion(const OfflineRegionDefinition& defin } OfflineRegionMetadata OfflineDatabase::updateMetadata(const int64_t regionID, const OfflineRegionMetadata& metadata) { + IsolatedTransaction transaction(*transactionManager); + // clang-format off Statement stmt = getStatement( "UPDATE regions SET description = ?1" @@ -604,6 +707,8 @@ OfflineRegionMetadata OfflineDatabase::updateMetadata(const int64_t regionID, co } void OfflineDatabase::deleteRegion(OfflineRegion&& region) { + IsolatedTransaction transaction(*transactionManager); + // clang-format off Statement stmt = getStatement( "DELETE FROM regions WHERE id = ?"); @@ -642,13 +747,15 @@ optional OfflineDatabase::hasRegionResource(int64_t regionID, const Res uint64_t OfflineDatabase::putRegionResource(int64_t regionID, const Resource& resource, const Response& response) { uint64_t size = putInternal(resource, response, false).second; bool previouslyUnused = markUsed(regionID, resource); + + transactionManager->onBatchResourceAdded(); if (offlineMapboxTileCount && resource.kind == Resource::Kind::Tile && util::mapbox::isMapboxURL(resource.url) && previouslyUnused) { *offlineMapboxTileCount += 1; - } + } return size; } diff --git a/platform/default/mbgl/storage/offline_database.hpp b/platform/default/mbgl/storage/offline_database.hpp index 91b544a9e01..927c0cd0be0 100644 --- a/platform/default/mbgl/storage/offline_database.hpp +++ b/platform/default/mbgl/storage/offline_database.hpp @@ -15,6 +15,7 @@ namespace mapbox { namespace sqlite { class Database; class Statement; +class Transaction; } // namespace sqlite } // namespace mapbox @@ -22,6 +23,8 @@ namespace mbgl { class Response; class TileID; +class BatchTransaction; +class TransactionManager; class OfflineDatabase : private util::noncopyable { public: @@ -35,6 +38,8 @@ class OfflineDatabase : private util::noncopyable { // Return value is (inserted, stored size) std::pair put(const Resource&, const Response&); + std::shared_ptr beginRegionDownload(); + std::vector listRegions(); OfflineRegion createRegion(const OfflineRegionDefinition&, @@ -104,6 +109,8 @@ class OfflineDatabase : private util::noncopyable { const std::string path; std::unique_ptr<::mapbox::sqlite::Database> db; std::unordered_map> statements; + + std::unique_ptr transactionManager; template T getPragma(const char *); diff --git a/platform/default/mbgl/storage/offline_download.cpp b/platform/default/mbgl/storage/offline_download.cpp index ba504c1f9b3..1e8176d281f 100644 --- a/platform/default/mbgl/storage/offline_download.cpp +++ b/platform/default/mbgl/storage/offline_download.cpp @@ -156,6 +156,7 @@ void OfflineDownload::activateDownload() { status = OfflineRegionStatus(); status.downloadState = OfflineRegionDownloadState::Active; status.requiredResourceCount++; + regionTransaction = offlineDatabase.beginRegionDownload(); ensureResource(Resource::style(definition.styleURL), [&](Response styleResponse) { status.requiredResourceCountIsPrecise = true; @@ -269,6 +270,7 @@ void OfflineDownload::activateDownload() { */ void OfflineDownload::continueDownload() { if (resourcesRemaining.empty() && status.complete()) { + regionTransaction.reset(); setState(OfflineRegionDownloadState::Inactive); return; } @@ -283,6 +285,7 @@ void OfflineDownload::deactivateDownload() { requiredSourceURLs.clear(); resourcesRemaining.clear(); requests.clear(); + regionTransaction.reset(); } void OfflineDownload::queueResource(Resource resource) { diff --git a/platform/default/mbgl/storage/offline_download.hpp b/platform/default/mbgl/storage/offline_download.hpp index 437f221c114..a4abeaef8f5 100644 --- a/platform/default/mbgl/storage/offline_download.hpp +++ b/platform/default/mbgl/storage/offline_download.hpp @@ -61,6 +61,8 @@ class OfflineDownload { void queueResource(Resource); void queueTiles(style::SourceType, uint16_t tileSize, const Tileset&); + + std::shared_ptr regionTransaction; }; } // namespace mbgl diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp index 94daf59c026..9563745b91d 100644 --- a/test/storage/offline_database.test.cpp +++ b/test/storage/offline_database.test.cpp @@ -292,9 +292,12 @@ TEST(OfflineDatabase, DeleteRegion) { Response response; response.noContent = true; - - db.putRegionResource(region.getID(), Resource::style("http://example.com/"), response); - db.putRegionResource(region.getID(), Resource::tile("http://example.com/", 1.0, 0, 0, 0, Tileset::Scheme::XYZ), response); + + { + auto transaction = db.beginRegionDownload(); + db.putRegionResource(region.getID(), Resource::style("http://example.com/"), response); + db.putRegionResource(region.getID(), Resource::tile("http://example.com/", 1.0, 0, 0, 0, Tileset::Scheme::XYZ), response); + } db.deleteRegion(std::move(region)); @@ -400,8 +403,11 @@ TEST(OfflineDatabase, PutRegionResourceDoesNotEvict) { Response response; response.data = randomString(1024); - for (uint32_t i = 1; i <= 100; i++) { - db.putRegionResource(region.getID(), Resource::style("http://example.com/"s + util::toString(i)), response); + { + auto transaction = db.beginRegionDownload(); + for (uint32_t i = 1; i <= 100; i++) { + db.putRegionResource(region.getID(), Resource::style("http://example.com/"s + util::toString(i)), response); + } } EXPECT_TRUE(bool(db.get(Resource::style("http://example.com/1")))); @@ -436,7 +442,8 @@ TEST(OfflineDatabase, GetRegionCompletedStatus) { Response response; response.data = std::make_shared("data"); - + + auto transaction = db.beginRegionDownload(); uint64_t styleSize = db.putRegionResource(region.getID(), Resource::style("http://example.com/"), response); OfflineRegionStatus status2 = db.getRegionCompletedStatus(region.getID()); @@ -467,8 +474,11 @@ TEST(OfflineDatabase, HasRegionResource) { Response response; response.data = randomString(1024); - for (uint32_t i = 1; i <= 100; i++) { - db.putRegionResource(region.getID(), Resource::style("http://example.com/"s + util::toString(i)), response); + { + auto transaction = db.beginRegionDownload(); + for (uint32_t i = 1; i <= 100; i++) { + 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")))); @@ -496,6 +506,7 @@ TEST(OfflineDatabase, HasRegionResourceTile) { response.data = std::make_shared("first"); EXPECT_FALSE(bool(db.hasRegionResource(region.getID(), resource))); + auto transaction = db.beginRegionDownload(); db.putRegionResource(region.getID(), resource, response); EXPECT_TRUE(bool(db.hasRegionResource(region.getID(), resource))); EXPECT_EQ(5, *(db.hasRegionResource(region.getID(), resource))); @@ -526,6 +537,8 @@ TEST(OfflineDatabase, OfflineMapboxTileCount) { // Count is initially zero. EXPECT_EQ(0u, db.getOfflineMapboxTileCount()); + + auto transaction = db.beginRegionDownload(); // Count stays the same after putting a non-tile resource. db.putRegionResource(region1.getID(), Resource::style("http://example.com/"), response); @@ -558,6 +571,8 @@ TEST(OfflineDatabase, OfflineMapboxTileCount) { // Count increases after putting a pre-existing, but non-offline Mapbox tile. db.putRegionResource(region1.getID(), mapboxTile2, response); EXPECT_EQ(2u, db.getOfflineMapboxTileCount()); + + transaction.reset(); // Count decreases after deleting a region when the tiles are not used by other regions. db.deleteRegion(std::move(region1)); diff --git a/test/storage/offline_download.test.cpp b/test/storage/offline_download.test.cpp index 57780eba400..cda37cf7468 100644 --- a/test/storage/offline_download.test.cpp +++ b/test/storage/offline_download.test.cpp @@ -295,6 +295,7 @@ TEST(OfflineDownload, GetStatusStyleComplete) { OfflineTilePyramidRegionDefinition("http://127.0.0.1:3000/style.json", LatLngBounds::world(), 0.0, 0.0, 1.0), test.db, test.fileSource); + auto transaction = test.db.beginRegionDownload(); test.db.putRegionResource(1, Resource::style("http://127.0.0.1:3000/style.json"), test.response("style.json")); @@ -317,6 +318,8 @@ TEST(OfflineDownload, GetStatusStyleAndSourceComplete) { OfflineTilePyramidRegionDefinition("http://127.0.0.1:3000/style.json", LatLngBounds::world(), 0.0, 0.0, 1.0), test.db, test.fileSource); + auto transaction = test.db.beginRegionDownload(); + test.db.putRegionResource(1, Resource::style("http://127.0.0.1:3000/style.json"), test.response("style.json"));