From 8d7d8e6533c9c863a9617d0bfeec904e62002cc6 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Fri, 10 Dec 2021 14:29:48 -0800 Subject: [PATCH] Cleanup archive structure. --- impeller/archivist/BUILD.gn | 6 +- impeller/archivist/archive.cc | 11 +- impeller/archivist/archive.h | 8 +- .../archivist/archive_class_registration.cc | 22 ++- impeller/archivist/archive_database.cc | 10 +- impeller/archivist/archive_database.h | 2 +- impeller/archivist/archive_vector.cc | 2 +- impeller/archivist/archive_vector.h | 2 +- impeller/archivist/archivist_fixture.cc | 38 +++++ impeller/archivist/archivist_fixture.h | 36 ++++ impeller/archivist/archivist_unittests.cc | 156 ++++++++---------- 11 files changed, 170 insertions(+), 123 deletions(-) create mode 100644 impeller/archivist/archivist_fixture.cc create mode 100644 impeller/archivist/archivist_fixture.h diff --git a/impeller/archivist/BUILD.gn b/impeller/archivist/BUILD.gn index 67667865232a9..3a409fb2e3a96 100644 --- a/impeller/archivist/BUILD.gn +++ b/impeller/archivist/BUILD.gn @@ -34,7 +34,11 @@ impeller_component("archivist") { impeller_component("archivist_unittests") { testonly = true - sources = [ "archivist_unittests.cc" ] + sources = [ + "archivist_fixture.cc", + "archivist_fixture.h", + "archivist_unittests.cc", + ] deps = [ ":archivist", "//flutter/testing", diff --git a/impeller/archivist/archive.cc b/impeller/archivist/archive.cc index 0289b648a0c1c..25e3d1d25e99b 100644 --- a/impeller/archivist/archive.cc +++ b/impeller/archivist/archive.cc @@ -14,8 +14,8 @@ namespace impeller { -Archive::Archive(const std::string& path, bool recreate) - : database_(std::make_unique(path, recreate)) {} +Archive::Archive(const std::string& path) + : database_(std::make_unique(path)) {} Archive::~Archive() { FML_DCHECK(transaction_count_ == 0) @@ -64,7 +64,7 @@ bool Archive::ArchiveInstance(const ArchiveDef& definition, /* * We need to bind the primary key only if the item does not provide its own */ - if (!definition.autoAssignName && + if (!definition.auto_key && !statement.WriteValue(ArchiveClassRegistration::NameIndex, itemName)) { return false; } @@ -79,8 +79,7 @@ bool Archive::ArchiveInstance(const ArchiveDef& definition, int64_t lastInsert = database_->GetLastInsertRowID(); - if (!definition.autoAssignName && - lastInsert != static_cast(itemName)) { + if (!definition.auto_key && lastInsert != static_cast(itemName)) { return false; } @@ -256,7 +255,7 @@ bool ArchiveLocation::ReadVectorKeys(Archivable::ArchiveName name, return false; } - const auto& keys = vector.keys(); + const auto& keys = vector.GetKeys(); std::move(keys.begin(), keys.end(), std::back_inserter(members)); diff --git a/impeller/archivist/archive.h b/impeller/archivist/archive.h index 97ebd6593cb42..930813e03c01c 100644 --- a/impeller/archivist/archive.h +++ b/impeller/archivist/archive.h @@ -24,9 +24,9 @@ struct ArchiveDef { using Member = uint64_t; using Members = std::vector; - const ArchiveDef* superClass; - const std::string className; - const bool autoAssignName; + const ArchiveDef* isa = nullptr; + const std::string table_name; + const bool auto_key = true; const Members members; }; @@ -34,7 +34,7 @@ static const Archivable::ArchiveName ArchiveNameAuto = 0; class Archive { public: - Archive(const std::string& path, bool recreate); + Archive(const std::string& path); ~Archive(); diff --git a/impeller/archivist/archive_class_registration.cc b/impeller/archivist/archive_class_registration.cc index e67275904201e..36f16bfa251f9 100644 --- a/impeller/archivist/archive_class_registration.cc +++ b/impeller/archivist/archive_class_registration.cc @@ -11,13 +11,12 @@ namespace impeller { -static const char* const ArchiveColumnPrefix = "item"; -static const char* const ArchivePrimaryKeyColumnName = "name"; -static const char* const ArchiveTablePrefix = "RL_"; +static const char* const ArchiveColumnPrefix = "col_"; +static const char* const ArchivePrimaryKeyColumnName = "primary_key"; ArchiveClassRegistration::ArchiveClassRegistration(ArchiveDatabase& database, ArchiveDef definition) - : database_(database), class_name_(definition.className) { + : database_(database), class_name_(definition.table_name) { /* * Each class in the archive class hierarchy is assigned an entry in the * class map. @@ -31,11 +30,11 @@ ArchiveClassRegistration::ArchiveClassRegistration(ArchiveDatabase& database, for (const auto& member : current->members) { map[member] = currentMember++; } - class_map_[current->className] = map; - current = current->superClass; + class_map_[current->table_name] = map; + current = current->isa; } - is_ready_ = CreateTable(definition.autoAssignName); + is_ready_ = CreateTable(definition.auto_key); } const std::string& ArchiveClassRegistration::GetClassName() const { @@ -81,8 +80,8 @@ bool ArchiveClassRegistration::CreateTable(bool autoIncrement) { * Table names cannot participate in parameter substitution, so we prepare * a statement and check its validity before running. */ - stream << "CREATE TABLE IF NOT EXISTS " << ArchiveTablePrefix - << class_name_.c_str() << " (" << ArchivePrimaryKeyColumnName; + stream << "CREATE TABLE IF NOT EXISTS " << class_name_.c_str() << " (" + << ArchivePrimaryKeyColumnName; if (autoIncrement) { stream << " INTEGER PRIMARY KEY AUTOINCREMENT, "; @@ -120,7 +119,7 @@ ArchiveStatement ArchiveClassRegistration::GetQueryStatement( stream << ","; } } - stream << " FROM " << ArchiveTablePrefix << class_name_; + stream << " FROM " << class_name_; if (single) { stream << " WHERE " << ArchivePrimaryKeyColumnName << " = ?"; @@ -135,8 +134,7 @@ ArchiveStatement ArchiveClassRegistration::GetQueryStatement( ArchiveStatement ArchiveClassRegistration::GetInsertStatement() const { std::stringstream stream; - stream << "INSERT OR REPLACE INTO " << ArchiveTablePrefix << class_name_ - << " VALUES ( ?, "; + stream << "INSERT OR REPLACE INTO " << class_name_ << " VALUES ( ?, "; for (size_t i = 0; i < member_count_; i++) { stream << "?"; if (i != member_count_ - 1) { diff --git a/impeller/archivist/archive_database.cc b/impeller/archivist/archive_database.cc index 75b13d7e55603..df174a508128d 100644 --- a/impeller/archivist/archive_database.cc +++ b/impeller/archivist/archive_database.cc @@ -18,11 +18,7 @@ namespace impeller { #define DB_HANDLE reinterpret_cast(database_) -ArchiveDatabase::ArchiveDatabase(const std::string& filename, bool recreate) { - if (recreate) { - ::remove(filename.c_str()); - } - +ArchiveDatabase::ArchiveDatabase(const std::string& filename) { if (::sqlite3_initialize() != SQLITE_OK) { VALIDATION_LOG << "Could not initialize sqlite."; return; @@ -82,7 +78,7 @@ static inline const ArchiveClassRegistration* RegistrationIfReady( const ArchiveClassRegistration* ArchiveDatabase::GetRegistrationForDefinition( const ArchiveDef& definition) { - auto found = registrations_.find(definition.className); + auto found = registrations_.find(definition.table_name); if (found != registrations_.end()) { /* * This class has already been registered. @@ -96,7 +92,7 @@ const ArchiveClassRegistration* ArchiveDatabase::GetRegistrationForDefinition( auto registration = std::unique_ptr( new ArchiveClassRegistration(*this, definition)); auto res = - registrations_.emplace(definition.className, std::move(registration)); + registrations_.emplace(definition.table_name, std::move(registration)); /* * If the new class registation is ready, return it to the caller. diff --git a/impeller/archivist/archive_database.h b/impeller/archivist/archive_database.h index 6092c3e231e23..3f66d8414ee32 100644 --- a/impeller/archivist/archive_database.h +++ b/impeller/archivist/archive_database.h @@ -18,7 +18,7 @@ struct ArchiveDef; class ArchiveDatabase { public: - ArchiveDatabase(const std::string& filename, bool recreate); + ArchiveDatabase(const std::string& filename); ~ArchiveDatabase(); diff --git a/impeller/archivist/archive_vector.cc b/impeller/archivist/archive_vector.cc index e642db06a78f7..2589cc3dd73fc 100644 --- a/impeller/archivist/archive_vector.cc +++ b/impeller/archivist/archive_vector.cc @@ -24,7 +24,7 @@ Archivable::ArchiveName ArchiveVector::GetArchiveName() const { return ArchiveNameAuto; } -const std::vector ArchiveVector::keys() const { +const std::vector ArchiveVector::GetKeys() const { return keys_; } diff --git a/impeller/archivist/archive_vector.h b/impeller/archivist/archive_vector.h index e186d6f9d3123..feceafed144d4 100644 --- a/impeller/archivist/archive_vector.h +++ b/impeller/archivist/archive_vector.h @@ -15,7 +15,7 @@ class ArchiveVector : public Archivable { ArchiveName GetArchiveName() const override; - const std::vector keys() const; + const std::vector GetKeys() const; bool Write(ArchiveLocation& item) const override; diff --git a/impeller/archivist/archivist_fixture.cc b/impeller/archivist/archivist_fixture.cc new file mode 100644 index 0000000000000..302bb7470a466 --- /dev/null +++ b/impeller/archivist/archivist_fixture.cc @@ -0,0 +1,38 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "impeller/archivist/archivist_fixture.h" + +#include "flutter/fml/paths.h" + +namespace impeller { +namespace testing { + +ArchivistFixture::ArchivistFixture() { + std::stringstream stream; + stream << flutter::testing::GetCurrentTestName() << ".db"; + archive_file_name_ = fml::paths::JoinPaths( + {flutter::testing::GetFixturesPath(), stream.str()}); +} + +ArchivistFixture::~ArchivistFixture() = default; + +const std::string ArchivistFixture::GetArchiveFileName() const { + return archive_file_name_; +} + +void ArchivistFixture::SetUp() { + DeleteArchiveFile(); +} + +void ArchivistFixture::TearDown() { + // DeleteArchiveFile(); +} + +void ArchivistFixture::DeleteArchiveFile() const { + fml::UnlinkFile(archive_file_name_.c_str()); +} + +} // namespace testing +} // namespace impeller diff --git a/impeller/archivist/archivist_fixture.h b/impeller/archivist/archivist_fixture.h new file mode 100644 index 0000000000000..2f444add6bdd2 --- /dev/null +++ b/impeller/archivist/archivist_fixture.h @@ -0,0 +1,36 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#pragma once + +#include "flutter/fml/macros.h" +#include "flutter/testing/testing.h" + +namespace impeller { +namespace testing { + +class ArchivistFixture : public ::testing::Test { + public: + ArchivistFixture(); + + ~ArchivistFixture(); + + // |::testing::Test| + void SetUp() override; + + // |::testing::Test| + void TearDown() override; + + const std::string GetArchiveFileName() const; + + private: + std::string archive_file_name_; + + void DeleteArchiveFile() const; + + FML_DISALLOW_COPY_AND_ASSIGN(ArchivistFixture); +}; + +} // namespace testing +} // namespace impeller diff --git a/impeller/archivist/archivist_unittests.cc b/impeller/archivist/archivist_unittests.cc index 96d47d6467119..eb247a98a64e8 100644 --- a/impeller/archivist/archivist_unittests.cc +++ b/impeller/archivist/archivist_unittests.cc @@ -8,6 +8,7 @@ #include "flutter/fml/macros.h" #include "flutter/testing/testing.h" #include "impeller/archivist/archive.h" +#include "impeller/archivist/archivist_fixture.h" namespace impeller { namespace testing { @@ -44,114 +45,89 @@ class Sample : public Archivable { }; const ArchiveDef Sample::ArchiveDefinition = { - .superClass = nullptr, - .className = "Sample", - .autoAssignName = false, + .isa = nullptr, + .table_name = "Sample", + .auto_key = false, .members = {999}, }; -TEST(ArchiveTest, SimpleInitialization) { - auto name = "/tmp/sample.db"; - { - Archive archive(name, true); - ASSERT_TRUE(archive.IsReady()); - } - ASSERT_EQ(::remove(name), 0); +using ArchiveTest = ArchivistFixture; + +TEST_F(ArchiveTest, SimpleInitialization) { + Archive archive(GetArchiveFileName().c_str()); + ASSERT_TRUE(archive.IsReady()); } -TEST(ArchiveTest, AddStorageClass) { - auto name = "/tmp/sample2.db"; - { - Archive archive(name, true); - ASSERT_TRUE(archive.IsReady()); - } - ASSERT_EQ(::remove(name), 0); +TEST_F(ArchiveTest, AddStorageClass) { + Archive archive(GetArchiveFileName().c_str()); + ASSERT_TRUE(archive.IsReady()); } -TEST(ArchiveTest, AddData) { - auto name = "/tmp/sample3.db"; - { - Archive archive(name, true); - ASSERT_TRUE(archive.IsReady()); - Sample sample; +TEST_F(ArchiveTest, AddData) { + Archive archive(GetArchiveFileName().c_str()); + ASSERT_TRUE(archive.IsReady()); + Sample sample; + ASSERT_TRUE(archive.Write(sample)); +} + +TEST_F(ArchiveTest, AddDataMultiple) { + Archive archive(GetArchiveFileName().c_str()); + ASSERT_TRUE(archive.IsReady()); + + for (size_t i = 0; i < 100; i++) { + Sample sample(i + 1); ASSERT_TRUE(archive.Write(sample)); } - ASSERT_EQ(::remove(name), 0); } -TEST(ArchiveTest, AddDataMultiple) { - auto name = "/tmp/sample4.db"; - { - Archive archive(name, true); - ASSERT_TRUE(archive.IsReady()); +TEST_F(ArchiveTest, ReadData) { + Archive archive(GetArchiveFileName().c_str()); + ASSERT_TRUE(archive.IsReady()); + + size_t count = 50; + + std::vector keys; + std::vector values; + + for (size_t i = 0; i < count; i++) { + Sample sample(i + 1); + keys.push_back(sample.GetArchiveName()); + values.push_back(sample.GetSomeData()); + ASSERT_TRUE(archive.Write(sample)); + } - for (size_t i = 0; i < 100; i++) { - Sample sample(i + 1); - ASSERT_TRUE(archive.Write(sample)); - } + for (size_t i = 0; i < count; i++) { + Sample sample; + ASSERT_TRUE(archive.Read(keys[i], sample)); + ASSERT_EQ(values[i], sample.GetSomeData()); } - ASSERT_EQ(::remove(name), 0); } -TEST(ArchiveTest, ReadData) { - auto name = "/tmp/sample5.db"; - { - Archive archive(name, true); - ASSERT_TRUE(archive.IsReady()); - - size_t count = 50; - - std::vector keys; - std::vector values; - - for (size_t i = 0; i < count; i++) { - Sample sample(i + 1); - keys.push_back(sample.GetArchiveName()); - values.push_back(sample.GetSomeData()); - ASSERT_TRUE(archive.Write(sample)); - } - - for (size_t i = 0; i < count; i++) { - Sample sample; - ASSERT_TRUE(archive.Read(keys[i], sample)); - ASSERT_EQ(values[i], sample.GetSomeData()); - } +TEST_F(ArchiveTest, ReadDataWithNames) { + Archive archive(GetArchiveFileName().c_str()); + ASSERT_TRUE(archive.IsReady()); + + size_t count = 8; + + std::vector keys; + std::vector values; + + keys.reserve(count); + values.reserve(count); + + for (size_t i = 0; i < count; i++) { + Sample sample(i + 1); + keys.push_back(sample.GetArchiveName()); + values.push_back(sample.GetSomeData()); + ASSERT_TRUE(archive.Write(sample)); } - ASSERT_EQ(::remove(name), 0); -} -/* - * This shouldn't be slow. Need to cache compiled statements. - */ -TEST(ArchiveTest, ReadDataWithNames) { - auto name = "/tmp/sample6.db"; - { - Archive archive(name, true); - ASSERT_TRUE(archive.IsReady()); - - size_t count = 8; - - std::vector keys; - std::vector values; - - keys.reserve(count); - values.reserve(count); - - for (size_t i = 0; i < count; i++) { - Sample sample(i + 1); - keys.push_back(sample.GetArchiveName()); - values.push_back(sample.GetSomeData()); - ASSERT_TRUE(archive.Write(sample)); - } - - for (size_t i = 0; i < count; i++) { - Sample sample; - ASSERT_TRUE(archive.Read(keys[i], sample)); - ASSERT_EQ(values[i], sample.GetSomeData()); - ASSERT_EQ(keys[i], sample.GetArchiveName()); - } + for (size_t i = 0; i < count; i++) { + Sample sample; + ASSERT_TRUE(archive.Read(keys[i], sample)); + ASSERT_EQ(values[i], sample.GetSomeData()); + ASSERT_EQ(keys[i], sample.GetArchiveName()); } - ASSERT_EQ(::remove(name), 0); } } // namespace testing