[core] Eliminate maximumCacheEntrySize

Instead, the eviction policy accounts for the actual size needed for an incoming put.

7 files changed, 103 insertions, 99 deletions

diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp
index 0cee7de328..d55e12b778 100644
--- a/include/mbgl/storage/default_file_source.hpp
+++ b/include/mbgl/storage/default_file_source.hpp
@@ -17,8 +17,7 @@ class DefaultFileSource : public FileSource {
 public:
     DefaultFileSource(const std::string& cachePath,
                       const std::string& assetRoot,
-                      uint64_t maximumCacheSize      = util::DEFAULT_MAX_CACHE_SIZE,
-                      uint64_t maximumCacheEntrySize = util::DEFAULT_MAX_CACHE_ENTRY_SIZE);
+                      uint64_t maximumCacheSize = util::DEFAULT_MAX_CACHE_SIZE);
     ~DefaultFileSource() override;
 
     void setAccessToken(const std::string&);
diff --git a/include/mbgl/util/constants.hpp b/include/mbgl/util/constants.hpp
index 667e2f8497..9ddbcb2568 100644
--- a/include/mbgl/util/constants.hpp
+++ b/include/mbgl/util/constants.hpp
@@ -22,7 +22,6 @@ extern const double MIN_ZOOM;
 extern const double MAX_ZOOM;
 
 extern const uint64_t DEFAULT_MAX_CACHE_SIZE;
-extern const uint64_t DEFAULT_MAX_CACHE_ENTRY_SIZE;
 
 } // namespace util
 
diff --git a/platform/default/default_file_source.cpp b/platform/default/default_file_source.cpp
index db0f4ccac6..bdf02ee69a 100644
--- a/platform/default/default_file_source.cpp
+++ b/platform/default/default_file_source.cpp
@@ -50,8 +50,8 @@ public:
         std::unique_ptr<FileRequest> onlineRequest;
     };
 
-    Impl(const std::string& cachePath, uint64_t maximumCacheSize, uint64_t maximumCacheEntrySize)
-        : offlineDatabase(cachePath, maximumCacheSize, maximumCacheEntrySize) {
+    Impl(const std::string& cachePath, uint64_t maximumCacheSize)
+        : offlineDatabase(cachePath, maximumCacheSize) {
     }
 
     void setAccessToken(const std::string& accessToken) {
@@ -140,10 +140,9 @@ private:
 
 DefaultFileSource::DefaultFileSource(const std::string& cachePath,
                                      const std::string& assetRoot,
-                                     uint64_t maximumCacheSize,
-                                     uint64_t maximumCacheEntrySize)
+                                     uint64_t maximumCacheSize)
     : thread(std::make_unique<util::Thread<Impl>>(util::ThreadContext{"DefaultFileSource", util::ThreadType::Unknown, util::ThreadPriority::Low},
-            cachePath, maximumCacheSize, maximumCacheEntrySize)),
+            cachePath, maximumCacheSize)),
       assetFileSource(std::make_unique<AssetFileSource>(assetRoot)) {
 }
 
diff --git a/platform/default/mbgl/storage/offline_database.cpp b/platform/default/mbgl/storage/offline_database.cpp
index d775c40759..65955aa09c 100644
--- a/platform/default/mbgl/storage/offline_database.cpp
+++ b/platform/default/mbgl/storage/offline_database.cpp
@@ -22,10 +22,9 @@ OfflineDatabase::Statement::~Statement() {
     stmt.clearBindings();
 }
 
-OfflineDatabase::OfflineDatabase(const std::string& path_, uint64_t maximumCacheSize_, uint64_t maximumCacheEntrySize_)
+OfflineDatabase::OfflineDatabase(const std::string& path_, uint64_t maximumCacheSize_)
     : path(path_),
-      maximumCacheSize(maximumCacheSize_),
-      maximumCacheEntrySize(maximumCacheEntrySize_) {
+      maximumCacheSize(maximumCacheSize_) {
     ensureSchema();
 }
 
@@ -112,29 +111,41 @@ optional<Response> OfflineDatabase::get(const Resource& resource) {
 }
 
 uint64_t OfflineDatabase::put(const Resource& resource, const Response& response) {
-    if (response.data && response.data->size() > maximumCacheEntrySize) {
-        Log::Warning(Event::Database, "Entry too big for caching");
-        return 0;
-    } else if (!evict()) {
-        Log::Warning(Event::Database, "Unable to make space for new entries");
-        return 0;
-    } else {
-        return putInternal(resource, response);
-    }
+    return putInternal(resource, response, true);
 }
 
-uint64_t OfflineDatabase::putInternal(const Resource& resource, const Response& response) {
-    // Don't store errors in the cache.
+uint64_t OfflineDatabase::putInternal(const Resource& resource, const Response& response, bool evict_) {
     if (response.error) {
         return 0;
     }
 
+    std::string compressedData;
+    bool compressed = false;
+    uint64_t size = 0;
+
+    if (response.data) {
+        compressedData = util::compress(*response.data);
+        compressed = compressedData.size() < response.data->size();
+        size = compressed ? compressedData.size() : response.data->size();
+    }
+
+    if (evict_ && !evict(size)) {
+        Log::Warning(Event::Database, "Unable to make space for entry");
+        return 0;
+    }
+
     if (resource.kind == Resource::Kind::Tile) {
         assert(resource.tileData);
-        return putTile(*resource.tileData, response);
+        putTile(*resource.tileData, response,
+                compressed ? compressedData : *response.data,
+                compressed);
     } else {
-        return putResource(resource, response);
+        putResource(resource, response,
+                compressed ? compressedData : *response.data,
+                compressed);
     }
+
+    return size;
 }
 
 optional<Response> OfflineDatabase::getResource(const Resource& resource) {
@@ -175,7 +186,10 @@ optional<Response> OfflineDatabase::getResource(const Resource& resource) {
     return response;
 }
 
-uint64_t OfflineDatabase::putResource(const Resource& resource, const Response& response) {
+void OfflineDatabase::putResource(const Resource& resource,
+                                  const Response& response,
+                                  const std::string& data,
+                                  bool compressed) {
     if (response.notModified) {
         Statement stmt = getStatement(
             //                               1            2             3
@@ -185,7 +199,6 @@ uint64_t OfflineDatabase::putResource(const Resource& resource, const Response&
         stmt->bind(2, response.expires);
         stmt->bind(3, resource.url);
         stmt->run();
-        return 0;
     } else {
         Statement stmt = getStatement(
             //                        1     2    3      4        5         6        7       8
@@ -199,27 +212,15 @@ uint64_t OfflineDatabase::putResource(const Resource& resource, const Response&
         stmt->bind(5 /* modified */, response.modified);
         stmt->bind(6 /* accessed */, SystemClock::now());
 
-        std::string data;
-        uint64_t size = 0;
-
         if (response.noContent) {
             stmt->bind(7 /* data */, nullptr);
             stmt->bind(8 /* compressed */, false);
         } else {
-            data = util::compress(*response.data);
-            if (data.size() < response.data->size()) {
-                size = data.size();
-                stmt->bindBlob(7 /* data */, data.data(), size, false);
-                stmt->bind(8 /* compressed */, true);
-            } else {
-                size = response.data->size();
-                stmt->bindBlob(7 /* data */, response.data->data(), size, false);
-                stmt->bind(8 /* compressed */, false);
-            }
+            stmt->bindBlob(7 /* data */, data.data(), data.size(), false);
+            stmt->bind(8 /* compressed */, compressed);
         }
 
         stmt->run();
-        return size;
     }
 }
 
@@ -281,7 +282,10 @@ optional<Response> OfflineDatabase::getTile(const Resource::TileData& tile) {
     return response;
 }
 
-uint64_t OfflineDatabase::putTile(const Resource::TileData& tile, const Response& response) {
+void OfflineDatabase::putTile(const Resource::TileData& tile,
+                              const Response& response,
+                              const std::string& data,
+                              bool compressed) {
     if (response.notModified) {
         Statement stmt = getStatement(
             "UPDATE tiles SET accessed = ?1, expires = ?2 "
@@ -301,7 +305,6 @@ uint64_t OfflineDatabase::putTile(const Resource::TileData& tile, const Response
         stmt->bind(6, tile.y);
         stmt->bind(7, tile.z);
         stmt->run();
-        return 0;
     } else {
         Statement stmt1 = getStatement(
             "REPLACE INTO tilesets (url_template, pixel_ratio) "
@@ -328,27 +331,15 @@ uint64_t OfflineDatabase::putTile(const Resource::TileData& tile, const Response
         stmt2->bind(8 /* expires */, response.expires);
         stmt2->bind(9 /* accessed */, SystemClock::now());
 
-        std::string data;
-        uint64_t size = 0;
-
         if (response.noContent) {
             stmt2->bind(10 /* data */, nullptr);
             stmt2->bind(11 /* compressed */, false);
         } else {
-            data = util::compress(*response.data);
-            if (data.size() < response.data->size()) {
-                size = data.size();
-                stmt2->bindBlob(10 /* data */, data.data(), size, false);
-                stmt2->bind(11 /* compressed */, true);
-            } else {
-                size = response.data->size();
-                stmt2->bindBlob(10 /* data */, response.data->data(), size, false);
-                stmt2->bind(11 /* compressed */, false);
-            }
+            stmt2->bindBlob(10 /* data */, data.data(), data.size(), false);
+            stmt2->bind(11 /* compressed */, compressed);
         }
 
         stmt2->run();
-        return size;
     }
 }
 
@@ -388,7 +379,7 @@ void OfflineDatabase::deleteRegion(OfflineRegion&& region) {
     stmt->bind(1, region.getID());
     stmt->run();
 
-    evict();
+    evict(0);
 }
 
 optional<Response> OfflineDatabase::getRegionResource(int64_t regionID, const Resource& resource) {
@@ -402,7 +393,7 @@ optional<Response> OfflineDatabase::getRegionResource(int64_t regionID, const Re
 }
 
 uint64_t OfflineDatabase::putRegionResource(int64_t regionID, const Resource& resource, const Response& response) {
-    uint64_t result = putInternal(resource, response);
+    uint64_t result = putInternal(resource, response, false);
     markUsed(regionID, resource);
     return result;
 }
@@ -487,12 +478,9 @@ T OfflineDatabase::getPragma(const char * sql) {
 // SQLite database never shrinks in size unless we call VACCUM. We here
 // are monitoring the soft limit (i.e. number of free pages in the file)
 // and as it approaches to the hard limit (i.e. the actual file size) we
-// delete an arbitrary number of old cache entries.
-//
-// The free pages approach saves us from calling VACCUM or keeping a
-// running total, which can be costly. We need a buffer because pages can
-// get fragmented on the database.
-bool OfflineDatabase::evict() {
+// delete an arbitrary number of old cache entries. The free pages approach saves
+// us from calling VACCUM or keeping a running total, which can be costly.
+bool OfflineDatabase::evict(uint64_t neededFreeSize) {
     uint64_t pageSize = getPragma<int64_t>("PRAGMA page_size");
     uint64_t pageCount = getPragma<int64_t>("PRAGMA page_count");
 
@@ -504,7 +492,9 @@ bool OfflineDatabase::evict() {
         return pageSize * (pageCount - getPragma<int64_t>("PRAGMA freelist_count"));
     };
 
-    while (usedSize() > maximumCacheSize - 5 * pageSize) {
+    // The addition of pageSize is a fudge factor to account for non `data` column
+    // size, and because pages can get fragmented on the database.
+    while (usedSize() + neededFreeSize + pageSize > maximumCacheSize) {
         Statement stmt1 = getStatement(
             "DELETE FROM resources "
             "WHERE ROWID IN ( "
diff --git a/platform/default/mbgl/storage/offline_database.hpp b/platform/default/mbgl/storage/offline_database.hpp
index 650cf6a16c..854ebdb47d 100644
--- a/platform/default/mbgl/storage/offline_database.hpp
+++ b/platform/default/mbgl/storage/offline_database.hpp
@@ -28,8 +28,7 @@ public:
     // Limits affect ambient caching (put) only; resources required by offline
     // regions are exempt.
     OfflineDatabase(const std::string& path,
-                    uint64_t maximumCacheSize      = util::DEFAULT_MAX_CACHE_SIZE,
-                    uint64_t maximumCacheEntrySize = util::DEFAULT_MAX_CACHE_ENTRY_SIZE);
+                    uint64_t maximumCacheSize = util::DEFAULT_MAX_CACHE_SIZE);
     ~OfflineDatabase();
 
     optional<Response> get(const Resource&);
@@ -66,14 +65,16 @@ private:
     };
 
     Statement getStatement(const char *);
-    uint64_t putInternal(const Resource&, const Response&);
 
     optional<Response> getTile(const Resource::TileData&);
-    uint64_t putTile(const Resource::TileData&, const Response&);
+    void putTile(const Resource::TileData&, const Response&,
+                 const std::string&, bool compressed);
 
     optional<Response> getResource(const Resource&);
-    uint64_t putResource(const Resource&, const Response&);
+    void putResource(const Resource&, const Response&,
+                     const std::string&, bool compressed);
 
+    uint64_t putInternal(const Resource&, const Response&, bool evict);
     void markUsed(int64_t regionID, const Resource&);
 
     const std::string path;
@@ -84,9 +85,8 @@ private:
     T getPragma(const char *);
 
     uint64_t maximumCacheSize;
-    uint64_t maximumCacheEntrySize;
 
-    bool evict();
+    bool evict(uint64_t neededFreeSize);
 };
 
 } // namespace mbgl
diff --git a/src/mbgl/util/constants.cpp b/src/mbgl/util/constants.cpp
index 047d7f3bd6..90a4d28c2f 100644
--- a/src/mbgl/util/constants.cpp
+++ b/src/mbgl/util/constants.cpp
@@ -30,7 +30,6 @@ const double MIN_ZOOM = 0.0;
 const double MAX_ZOOM = 25.5;
 
 const uint64_t DEFAULT_MAX_CACHE_SIZE = 50 * 1024 * 1024;
-const uint64_t DEFAULT_MAX_CACHE_ENTRY_SIZE = std::numeric_limits<uint64_t>::max();
 
 } // namespace util
 
diff --git a/test/storage/offline_database.cpp b/test/storage/offline_database.cpp
index 680a14bb4f..e4a5381d21 100644
--- a/test/storage/offline_database.cpp
+++ b/test/storage/offline_database.cpp
@@ -9,6 +9,7 @@
 #include <gtest/gtest.h>
 #include <sqlite3.h>
 #include <thread>
+#include <random>
 
 using namespace std::literals::string_literals;
 
@@ -484,38 +485,33 @@ TEST(OfflineDatabase, ConcurrentUse) {
     thread2.join();
 }
 
-TEST(OfflineDatabase, PutIgnoresOversizedResources) {
-    using namespace mbgl;
-
-    Log::setObserver(std::make_unique<FixtureLogObserver>());
-    OfflineDatabase db(":memory:", 1000, 1);
-
-    Resource resource = Resource::style("http://example.com/");
-    Response response;
-    response.data = std::make_shared<std::string>("data");
+static std::shared_ptr<std::string> randomString(size_t size) {
+    auto result = std::make_shared<std::string>(size, 0);
+    std::mt19937 random;
 
-    db.put(resource, response);
-    EXPECT_FALSE(bool(db.get(resource)));
+    for (size_t i = 0; i < size; i++) {
+        (*result)[i] = random();
+    }
 
-    auto observer = Log::removeObserver();
-    auto flo = dynamic_cast<FixtureLogObserver*>(observer.get());
-    EXPECT_EQ(1ul, flo->count({ EventSeverity::Warning, Event::Database, -1, "Entry too big for caching" }));
+    return result;
 }
 
-TEST(OfflineDatabase, PutRegionResourceDoesNotIgnoreOversizedResources) {
+TEST(OfflineDatabase, PutReturnsSize) {
     using namespace mbgl;
 
-    OfflineDatabase db(":memory:", 1000, 1);
+    OfflineDatabase db(":memory:");
 
-    OfflineRegionDefinition definition { "", LatLngBounds::world(), 0, INFINITY, 1.0 };
-    OfflineRegion region = db.createRegion(definition, OfflineRegionMetadata());
+    Response compressible;
+    compressible.data = std::make_shared<std::string>(1024, 0);
+    EXPECT_EQ(17, db.put(Resource::style("http://example.com/compressible"), compressible));
 
-    Resource resource = Resource::style("http://example.com/");
-    Response response;
-    response.data = std::make_shared<std::string>("data");
+    Response incompressible;
+    incompressible.data = randomString(1024);
+    EXPECT_EQ(1024, db.put(Resource::style("http://example.com/incompressible"), incompressible));
 
-    db.putRegionResource(region.getID(), resource, response);
-    EXPECT_TRUE(bool(db.get(resource)));
+    Response noContent;
+    noContent.noContent = true;
+    EXPECT_EQ(0, db.put(Resource::style("http://example.com/noContent"), noContent));
 }
 
 TEST(OfflineDatabase, PutEvictsLeastRecentlyUsedResources) {
@@ -524,7 +520,7 @@ TEST(OfflineDatabase, PutEvictsLeastRecentlyUsedResources) {
     OfflineDatabase db(":memory:", 1024 * 20);
 
     Response response;
-    response.data = std::make_shared<std::string>(1024, '0');
+    response.data = randomString(1024);
 
     for (uint32_t i = 1; i <= 20; i++) {
         db.put(Resource::style("http://example.com/"s + util::toString(i)), response);
@@ -538,12 +534,11 @@ TEST(OfflineDatabase, PutRegionResourceDoesNotEvict) {
     using namespace mbgl;
 
     OfflineDatabase db(":memory:", 1024 * 20);
-
     OfflineRegionDefinition definition { "", LatLngBounds::world(), 0, INFINITY, 1.0 };
     OfflineRegion region = db.createRegion(definition, OfflineRegionMetadata());
 
     Response response;
-    response.data = std::make_shared<std::string>(1024, '0');
+    response.data = randomString(1024);
 
     for (uint32_t i = 1; i <= 20; i++) {
         db.putRegionResource(region.getID(), Resource::style("http://example.com/"s + util::toString(i)), response);
@@ -552,3 +547,26 @@ TEST(OfflineDatabase, PutRegionResourceDoesNotEvict) {
     EXPECT_TRUE(bool(db.get(Resource::style("http://example.com/1"))));
     EXPECT_TRUE(bool(db.get(Resource::style("http://example.com/20"))));
 }
+
+TEST(OfflineDatabase, PutFailsWhenEvictionInsuffices) {
+    using namespace mbgl;
+
+    Log::setObserver(std::make_unique<FixtureLogObserver>());
+    OfflineDatabase db(":memory:", 1024 * 20);
+
+    Response small;
+    small.data = randomString(1024);
+
+    for (uint32_t i = 1; i <= 10; i++) {
+        db.put(Resource::style("http://example.com/"s + util::toString(i)), small);
+    }
+
+    Response big;
+    big.data = randomString(1024 * 15);
+    db.put(Resource::style("http://example.com/big"), big);
+    EXPECT_FALSE(bool(db.get(Resource::style("http://example.com/big"))));
+
+    auto observer = Log::removeObserver();
+    auto flo = dynamic_cast<FixtureLogObserver*>(observer.get());
+    EXPECT_EQ(1ul, flo->count({ EventSeverity::Warning, Event::Database, -1, "Unable to make space for entry" }));
+}