From 9e7c31f9946b486111371b20772e25dc6f7a388c Mon Sep 17 00:00:00 2001 From: Ivo van Dongen Date: Sun, 18 Feb 2018 16:23:01 +0200 Subject: [core] commit ad-hoc cached resources and region (meta-data) immediately --- platform/default/mbgl/storage/offline_database.cpp | 101 ++++++++++++++------- platform/default/mbgl/storage/offline_database.hpp | 8 +- platform/default/mbgl/storage/offline_download.hpp | 2 +- test/storage/offline_database.test.cpp | 31 +++++-- test/storage/offline_download.test.cpp | 3 + 5 files changed, 100 insertions(+), 45 deletions(-) diff --git a/platform/default/mbgl/storage/offline_database.cpp b/platform/default/mbgl/storage/offline_database.cpp index 4a72b3f371..fae7938501 100644 --- a/platform/default/mbgl/storage/offline_database.cpp +++ b/platform/default/mbgl/storage/offline_database.cpp @@ -11,39 +11,54 @@ namespace mbgl { // Manages transactions for (multiple) region downloads -class RegionTransaction { +// and ad-hoc caching +class TransactionManager { public: - RegionTransaction(mapbox::sqlite::Database& database_) + TransactionManager(mapbox::sqlite::Database& database_) : database(database_) { } - ~RegionTransaction() { + ~TransactionManager() { commitTransaction(); } - void start() { - runningTransactions++; + void startIsolated() { + commitTransaction(); + openTransaction(); + } + + void endIsolated() { + commitTransaction(); + if (runningBatches > 0) { + openTransaction(); + } + } + + void startBatch() { + runningBatches++; openTransaction(); }; - void end() { - runningTransactions--; - if (runningTransactions == 0 && pendingRegionResources > 0) { + void endBatch() { + runningBatches--; + if (runningBatches == 0 && pendingResources > 0) { commitTransaction(); } }; - void onResourceAdded() { - pendingRegionResources++; - if (pendingRegionResources >= 128) { + void onBatchResourceAdded() { + assert (runningBatches > 0); + + pendingResources++; + if (pendingResources >= 128) { commitTransaction(); openTransaction(); } } + private: void openTransaction() { if (!transaction) { - Log::Info(Event::Database, "Starting region transaction"); transaction = std::make_unique(database, mapbox::sqlite::Transaction::Immediate); } } @@ -52,35 +67,50 @@ private: if (!transaction) { return; } - - Log::Info(Event::Database, "Committing region transaction"); + transaction->commit(); transaction.reset(); - pendingRegionResources = 0; + pendingResources = 0; } mapbox::sqlite::Database& database; std::unique_ptr transaction; - int runningTransactions = 0; - int64_t pendingRegionResources = 0; + int runningBatches = 0; + int64_t pendingResources = 0; }; -// Enables reference counting on current (region) +// Enables reference counting on current batch // transactions -class TransactionToken { +class BatchTransaction { public: - TransactionToken(RegionTransaction& transaction_) - : transaction(transaction_) { - transaction.start(); + BatchTransaction(TransactionManager& manager_) + : manager(manager_) { + manager.startBatch(); } - ~TransactionToken() { - transaction.end(); + ~BatchTransaction() { + manager.endBatch(); } private: - RegionTransaction& transaction; + 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() { @@ -92,14 +122,13 @@ OfflineDatabase::OfflineDatabase(std::string path_, uint64_t maximumCacheSize_) : path(std::move(path_)), maximumCacheSize(maximumCacheSize_) { ensureSchema(); - regionTransaction = std::make_unique(*db); } OfflineDatabase::~OfflineDatabase() { // Deleting these SQLite objects may result in exceptions, but we're in a destructor, so we // can't throw anything. try { - regionTransaction.reset(); + transactionManager.reset(); statements.clear(); db.reset(); } catch (mapbox::sqlite::Exception& ex) { @@ -108,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() { @@ -203,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) { @@ -244,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); } @@ -621,8 +652,8 @@ bool OfflineDatabase::putTile(const Resource::TileData& tile, return true; } -std::shared_ptr OfflineDatabase::beginRegionDownload() { - return std::make_shared(*regionTransaction); +std::shared_ptr OfflineDatabase::beginRegionDownload() { + return std::make_shared(*transactionManager); } std::vector OfflineDatabase::listRegions() { @@ -645,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) " @@ -659,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" @@ -672,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 = ?"); @@ -711,7 +748,7 @@ uint64_t OfflineDatabase::putRegionResource(int64_t regionID, const Resource& re uint64_t size = putInternal(resource, response, false).second; bool previouslyUnused = markUsed(regionID, resource); - regionTransaction->onResourceAdded(); + transactionManager->onBatchResourceAdded(); if (offlineMapboxTileCount && resource.kind == Resource::Kind::Tile diff --git a/platform/default/mbgl/storage/offline_database.hpp b/platform/default/mbgl/storage/offline_database.hpp index aeb360bf81..927c0cd0be 100644 --- a/platform/default/mbgl/storage/offline_database.hpp +++ b/platform/default/mbgl/storage/offline_database.hpp @@ -23,8 +23,8 @@ namespace mbgl { class Response; class TileID; -class TransactionToken; -class RegionTransaction; +class BatchTransaction; +class TransactionManager; class OfflineDatabase : private util::noncopyable { public: @@ -38,7 +38,7 @@ public: // Return value is (inserted, stored size) std::pair put(const Resource&, const Response&); - std::shared_ptr beginRegionDownload(); + std::shared_ptr beginRegionDownload(); std::vector listRegions(); @@ -110,7 +110,7 @@ private: std::unique_ptr<::mapbox::sqlite::Database> db; std::unordered_map> statements; - std::unique_ptr regionTransaction; + std::unique_ptr transactionManager; template T getPragma(const char *); diff --git a/platform/default/mbgl/storage/offline_download.hpp b/platform/default/mbgl/storage/offline_download.hpp index c104927b35..a4abeaef8f 100644 --- a/platform/default/mbgl/storage/offline_download.hpp +++ b/platform/default/mbgl/storage/offline_download.hpp @@ -62,7 +62,7 @@ private: void queueResource(Resource); void queueTiles(style::SourceType, uint16_t tileSize, const Tileset&); - std::shared_ptr regionTransaction; + std::shared_ptr regionTransaction; }; } // namespace mbgl diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp index 94daf59c02..9563745b91 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 57780eba40..cda37cf746 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")); -- cgit v1.2.1