[core] commit ad-hoc cached resources and region (meta-data) immediatelyupstream/offline-download-transactions

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<mapbox::sqlite::Transaction>(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<mapbox::sqlite::Transaction> 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<RegionTransaction>(*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<mapbox::sqlite::Database>(path.c_str(), flags);
     db->setBusyTimeout(Milliseconds::max());
     db->exec("PRAGMA foreign_keys = ON");
+    transactionManager = std::make_unique<TransactionManager>(*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<int64_t> OfflineDatabase::hasInternal(const Resource& resource) {
 }
 
 std::pair<bool, uint64_t> 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<TransactionToken> OfflineDatabase::beginRegionDownload() {
-    return std::make_shared<TransactionToken>(*regionTransaction);
+std::shared_ptr<BatchTransaction> OfflineDatabase::beginRegionDownload() {
+    return std::make_shared<BatchTransaction>(*transactionManager);
 }
 
 std::vector<OfflineRegion> OfflineDatabase::listRegions() {
@@ -645,6 +676,8 @@ std::vector<OfflineRegion> 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<bool, uint64_t> put(const Resource&, const Response&);
 
-    std::shared_ptr<TransactionToken> beginRegionDownload();
+    std::shared_ptr<BatchTransaction> beginRegionDownload();
 
     std::vector<OfflineRegion> listRegions();
 
@@ -110,7 +110,7 @@ private:
     std::unique_ptr<::mapbox::sqlite::Database> db;
     std::unordered_map<const char *, std::unique_ptr<::mapbox::sqlite::Statement>> statements;
     
-    std::unique_ptr<RegionTransaction> regionTransaction;
+    std::unique_ptr<TransactionManager> transactionManager;
 
     template <class T>
     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<TransactionToken> regionTransaction;
+    std::shared_ptr<BatchTransaction> 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<std::string>("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<std::string>("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"));