Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Implement a vacuum strategy for the offline database #4342

Merged
merged 1 commit into from
Mar 17, 2016
Merged

[core] Implement a vacuum strategy for the offline database #4342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ offline.db
/test/fixtures/api/1.png
/test/fixtures/api/2.png
/test/fixtures/database/*.db
/test/fixtures/offline/v3.db
/test/fixtures/**/actual.png
/test/fixtures/**/diff.png
/test/output
Expand Down
39 changes: 22 additions & 17 deletions platform/default/mbgl/storage/offline_database.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ namespace mbgl {

using namespace mapbox::sqlite;

// If you change the schema you must write a migration from the previous version.
static const uint32_t schemaVersion = 2;

OfflineDatabase::Statement::~Statement() {
stmt.reset();
stmt.clearBindings();
Expand Down Expand Up @@ -50,15 +47,12 @@ void OfflineDatabase::ensureSchema() {
try {
connect(ReadWrite);

{
auto userVersionStmt = db->prepare("PRAGMA user_version");
userVersionStmt.run();
switch (userVersionStmt.get<int>(0)) {
case 0: break; // cache-only database; ok to delete
case 1: break; // cache-only database; ok to delete
case 2: return;
default: throw std::runtime_error("unknown schema version");
}
switch (userVersion()) {
case 0: break; // cache-only database; ok to delete
case 1: break; // cache-only database; ok to delete
case 2: migrateToVersion3(); // fall through
case 3: return;
default: throw std::runtime_error("unknown schema version");
}

removeExisting();
Expand All @@ -76,8 +70,17 @@ void OfflineDatabase::ensureSchema() {
#include "offline_schema.cpp.include"

connect(ReadWrite | Create);

// If you change the schema you must write a migration from the previous version.
db->exec("PRAGMA auto_vacuum = INCREMENTAL");
db->exec(schema);
db->exec("PRAGMA user_version = " + util::toString(schemaVersion));
db->exec("PRAGMA user_version = 3");
}

int OfflineDatabase::userVersion() {
auto stmt = db->prepare("PRAGMA user_version");
stmt.run();
return stmt.get<int>(0);
}

void OfflineDatabase::removeExisting() {
Expand All @@ -92,6 +95,11 @@ void OfflineDatabase::removeExisting() {
}
}

void OfflineDatabase::migrateToVersion3() {
db->exec("PRAGMA auto_vacuum = INCREMENTAL");
db->exec("VACUUM");
}

OfflineDatabase::Statement OfflineDatabase::getStatement(const char * sql) {
auto it = statements.find(sql);

Expand Down Expand Up @@ -458,6 +466,7 @@ void OfflineDatabase::deleteRegion(OfflineRegion&& region) {
stmt->run();

evict(0);
db->exec("PRAGMA incremental_vacuum");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think calling vacuum should be a separated API and not called automatically. I can think of 2 use cases for that:

  • In your app you want to remove several regions. Each deletion would be super slow because the database would be recreated on each VACUUM. Instead, the user interface would be designed in a way you delete all regions first and at the end call OffilineDatabase::vacuum(), committing to the operation.
  • This API could be used for vacuum the dangling pages from the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that PRAGMA incremental_vacuum is not the same as VACUUM. In particular, my interpretation of the documentation is that PRAGMA incremental_vacuum is much cheaper: it works with a page structure that has already been prepared for incremental vacuum via PRAGMA auto_vacuum, and it doesn't compact partially filled pages of the database nor rewrite the entire database file like VACUUM does. I am not sure that a single PRAGMA incremental_vacuum after deleting several regions would be significantly cheaper than calling PRAGMA incremental_vacuum after each deletion. In fact, it might block other database commands for a longer continuous period of time.

Regarding the second point, we expect the number of free pages for the ambient cache to be low. The expected behavior is that the ambient cache fills up to the maximum allotted size, and then stays at that size. Vacuuming free pages from it would in fact be a deoptimization, because those pages would just need to be reallocated as soon as the cache filled back up to its maximum size.

I'd prefer to keep the vacuum strategy a private implementation detail for now. There are certainly some unknowns about how it will perform, but it's not clear to me that providing API control will help.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying.


// Ensure that the cached offlineTileCount value is recalculated.
offlineMapboxTileCount = {};
Expand Down Expand Up @@ -614,10 +623,6 @@ bool OfflineDatabase::evict(uint64_t neededFreeSize) {
uint64_t pageSize = getPragma<int64_t>("PRAGMA page_size");
uint64_t pageCount = getPragma<int64_t>("PRAGMA page_count");

if (pageSize * pageCount > maximumCacheSize) {
Log::Warning(mbgl::Event::Database, "Current size is larger than the maximum size. Database won't get truncated.");
}

auto usedSize = [&] {
return pageSize * (pageCount - getPragma<int64_t>("PRAGMA freelist_count"));
};
Expand Down
2 changes: 2 additions & 0 deletions platform/default/mbgl/storage/offline_database.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,10 @@ class OfflineDatabase : private util::noncopyable {

private:
void connect(int flags);
int userVersion();
void ensureSchema();
void removeExisting();
void migrateToVersion3();

class Statement {
public:
Expand Down
Binary file added test/fixtures/offline/v2.db
View file
Open in desktop
Binary file not shown.
28 changes: 28 additions & 0 deletions test/storage/offline_database.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <mbgl/util/string.hpp>

#include <gtest/gtest.h>
#include <sqlite3.hpp>
#include <sqlite3.h>
#include <thread>
#include <random>
Expand Down Expand Up @@ -660,3 +661,30 @@ TEST(OfflineDatabase, OfflineMapboxTileCount) {
db.deleteRegion(std::move(region1));
EXPECT_EQ(0, db.getOfflineMapboxTileCount());
}

static int databasePageCount(const std::string& path) {
mapbox::sqlite::Database db(path, mapbox::sqlite::ReadOnly);
mapbox::sqlite::Statement stmt = db.prepare("pragma page_count");
stmt.run();
return stmt.get<int>(0);
}

TEST(OfflineDatabase, MigrateFromV2Schema) {
using namespace mbgl;

// v2.db is a v2 database containing a single offline region with a small number of resources.

deleteFile("test/fixtures/offline/v3.db");
writeFile("test/fixtures/offline/v3.db", util::read_file("test/fixtures/offline/v2.db"));

{
OfflineDatabase db("test/fixtures/offline/v3.db", 0);
auto regions = db.listRegions();
for (auto& region : regions) {
db.deleteRegion(std::move(region));
}
}

EXPECT_LT(databasePageCount("test/fixtures/offline/v3.db"),
databasePageCount("test/fixtures/offline/v2.db"));
}