From 070a14d8accf1f23330ee743b3bf10593ff42563 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Mon, 7 Sep 2015 16:08:07 +0300 Subject: [PATCH] Review fixes. --- coding/simple_dense_coding.cpp | 7 +- coding/simple_dense_coding.hpp | 8 -- indexer/indexer.pro | 1 - indexer/indexer_tests/rank_table_test.cpp | 69 ++++++++--- indexer/rank_table.cpp | 132 +++++++++++++--------- indexer/rank_table.hpp | 19 ++-- 6 files changed, 148 insertions(+), 88 deletions(-) diff --git a/coding/simple_dense_coding.cpp b/coding/simple_dense_coding.cpp index c75034b5be..4fd359a6f5 100644 --- a/coding/simple_dense_coding.cpp +++ b/coding/simple_dense_coding.cpp @@ -106,7 +106,12 @@ SimpleDenseCoding::SimpleDenseCoding(vector const & data) m_symbols.assign(symbols); } -SimpleDenseCoding::SimpleDenseCoding(SimpleDenseCoding && rhs) { Swap(move(rhs)); } +SimpleDenseCoding::SimpleDenseCoding(SimpleDenseCoding && rhs) +{ + m_bits.swap(rhs.m_bits); + m_index.swap(rhs.m_index); + m_symbols.swap(rhs.m_symbols); +} uint8_t SimpleDenseCoding::Get(uint64_t i) const { diff --git a/coding/simple_dense_coding.hpp b/coding/simple_dense_coding.hpp index cd647991a9..025767e722 100644 --- a/coding/simple_dense_coding.hpp +++ b/coding/simple_dense_coding.hpp @@ -52,14 +52,6 @@ public: visitor(m_symbols, "m_symbols"); } - template - void Swap(TSDC && rhs) - { - m_bits.swap(rhs.m_bits); - m_index.swap(rhs.m_index); - m_symbols.swap(rhs.m_symbols); - } - private: succinct::bit_vector m_bits; succinct::rs_bit_vector m_index; diff --git a/indexer/indexer.pro b/indexer/indexer.pro index e7fe5e6a5b..2c3012aa3f 100644 --- a/indexer/indexer.pro +++ b/indexer/indexer.pro @@ -75,7 +75,6 @@ HEADERS += \ feature_loader_base.hpp \ feature_meta.hpp \ feature_processor.hpp \ - feature_rank_table.cpp \ feature_utils.hpp \ feature_visibility.hpp \ features_offsets_table.hpp \ diff --git a/indexer/indexer_tests/rank_table_test.cpp b/indexer/indexer_tests/rank_table_test.cpp index 9430394000..8a496ce2d5 100644 --- a/indexer/indexer_tests/rank_table_test.cpp +++ b/indexer/indexer_tests/rank_table_test.cpp @@ -1,15 +1,22 @@ #include "testing/testing.hpp" +#include "indexer/classificator_loader.hpp" +#include "indexer/index.hpp" +#include "indexer/mwm_set.hpp" #include "indexer/rank_table.hpp" #include "platform/country_defines.hpp" #include "platform/local_country_file.hpp" +#include "platform/platform.hpp" #include "coding/file_container.hpp" +#include "coding/file_name_utils.hpp" #include "coding/file_writer.hpp" +#include "coding/internal/file_data.hpp" #include "base/scope_guard.hpp" +#include "std/string.hpp" #include "std/vector.hpp" namespace @@ -17,9 +24,28 @@ namespace void TestTable(vector const & ranks, search::RankTable const & table) { TEST_EQUAL(ranks.size(), table.Size(), ()); - TEST_EQUAL(table.GetVersion(), search::RankTable::V1, ()); + TEST_EQUAL(table.GetVersion(), search::RankTable::V0, ()); for (size_t i = 0; i < ranks.size(); ++i) - TEST_EQUAL(i, table.Get(i), ()); + TEST_EQUAL(ranks[i], table.Get(i), ()); +} + +void TestTable(vector const & ranks, string const & path) +{ + // Tries to load table via file read. + { + FilesContainerR rcont(path); + auto table = search::RankTable::Load(rcont); + TEST(table, ()); + TestTable(ranks, *table); + } + + // Tries to load table via file mapping. + { + FilesMappingContainer mcont(path); + auto table = search::RankTable::Load(mcont); + TEST(table, ()); + TestTable(ranks, *table); + } } } // namespace @@ -40,19 +66,34 @@ UNIT_TEST(FeatureRankTableBuilder_Smoke) search::RankTableBuilder::Create(ranks, wcont); } - // Tries to load table via file read. + TestTable(ranks, kTestCont); +} + +UNIT_TEST(FeatureRankTableBuilder_EndToEnd) +{ + classificator::Load(); + + string const originalMapPath = + my::JoinFoldersToPath(GetPlatform().WritableDir(), "minsk-pass.mwm"); + string const mapPath = my::JoinFoldersToPath(GetPlatform().WritableDir(), "minsk-pass-copy.mwm"); + my::CopyFileX(originalMapPath, mapPath); + MY_SCOPE_GUARD(cleanup, bind(&FileWriter::DeleteFileX, mapPath)); + + platform::LocalCountryFile localFile = + platform::LocalCountryFile::MakeForTesting("minsk-pass-copy"); + TEST(localFile.OnDisk(MapOptions::Map), ()); + + vector ranks; { - FilesContainerR rcont(kTestCont); - auto table = search::RankTable::Load(rcont); - TEST(table, ()); - TestTable(ranks, *table); + FilesContainerR rcont(mapPath); + search::RankTableBuilder::CalcSearchRanks(rcont, ranks); } - // Tries to load table via file mapping. - { - FilesMappingContainer mcont(kTestCont); - auto table = search::RankTable::Load(mcont); - TEST(table, ()); - TestTable(ranks, *table); - } + search::RankTableBuilder::Create(localFile); + + Index index; + auto regResult = index.RegisterMap(localFile); + TEST_EQUAL(regResult.second, MwmSet::RegResult::Success, ()); + + TestTable(ranks, mapPath); } diff --git a/indexer/rank_table.cpp b/indexer/rank_table.cpp index b7a500b19e..34484f7e84 100644 --- a/indexer/rank_table.cpp +++ b/indexer/rank_table.cpp @@ -3,7 +3,6 @@ #include "indexer/data_header.hpp" #include "indexer/feature_algo.hpp" #include "indexer/feature_utils.hpp" -#include "indexer/features_offsets_table.hpp" #include "indexer/features_vector.hpp" #include "indexer/types_skipper.hpp" @@ -11,6 +10,8 @@ #include "coding/endianness.hpp" #include "coding/file_container.hpp" +#include "coding/file_writer.hpp" +#include "coding/reader.hpp" #include "coding/simple_dense_coding.hpp" #include "coding/succinct_mapper.hpp" #include "coding/writer.hpp" @@ -25,19 +26,31 @@ namespace search { +namespace +{ uint64_t const kVersionOffset = 0; uint64_t const kFlagsOffset = 1; uint64_t const kHeaderSize = 8; -namespace +enum class CheckResult { -// Returns true when flags claim that the serialized data has the same -// endianness as a host. -bool SameEndianness(uint8_t flags) + CorruptedHeader, + EndiannessMismatch, + EndiannessMatch +}; + +template +CheckResult CheckEndianness(TReader && reader) { + if (reader.Size() < kHeaderSize) + return CheckResult::CorruptedHeader; + uint8_t flags; + reader.Read(kFlagsOffset, &flags, sizeof(flags)); bool const isHostBigEndian = IsBigEndian(); bool const isDataBigEndian = flags & 1; - return isHostBigEndian == isDataBigEndian; + if (isHostBigEndian != isDataBigEndian) + return CheckResult::EndiannessMismatch; + return CheckResult::EndiannessMatch; } class MemoryRegion @@ -101,17 +114,19 @@ unique_ptr GetMemoryRegionForTag(FilesMappingContainer & mco return make_unique(move(handle)); } -class RankTableV1 : public RankTable +// RankTable version 1, uses simple dense coding to store and access +// array of ranks. +class RankTableV0 : public RankTable { public: - RankTableV1() = default; + RankTableV0() = default; - RankTableV1(vector const & ranks) : m_coding(ranks) {} + RankTableV0(vector const & ranks) : m_coding(ranks) {} // RankTable overrides: uint8_t Get(uint64_t i) const override { return m_coding.Get(i); } uint64_t Size() const override { return m_coding.Size(); } - RankTable::Version GetVersion() const override { return V1; } + RankTable::Version GetVersion() const override { return V0; } void Serialize(Writer & writer) override { static uint64_t const padding = 0; @@ -124,36 +139,46 @@ public: Freeze(m_coding, writer, "SimpleDenseCoding"); } - // Loads rank table v1 from a raw memory region. - static unique_ptr Load(unique_ptr && region) + // Loads RankTableV0 from a raw memory region. + static unique_ptr Load(unique_ptr && region) { - if (!region.get() || region->Size() < kHeaderSize) - return unique_ptr(); + if (!region.get()) + return unique_ptr(); - uint8_t const flags = region->ImmutableData()[kFlagsOffset]; - if (!SameEndianness(flags)) - return unique_ptr(); + auto const result = CheckEndianness(MemReader(region->ImmutableData(), region->Size())); + if (result != CheckResult::EndiannessMatch) + return unique_ptr(); - unique_ptr table(new RankTableV1()); + unique_ptr table(new RankTableV0()); coding::Map(table->m_coding, region->ImmutableData() + kHeaderSize, "SimpleDenseCoding"); table->m_region = move(region); return table; } - // Loads rank table v1 from a raw memory region. Modifies region in + // Loads RankTableV0 from a raw memory region. Modifies region in // the case of endianness mismatch. - static unique_ptr Load(unique_ptr && region) + static unique_ptr Load(unique_ptr && region) { - if (!region.get() || region->Size() < kHeaderSize) - return unique_ptr(); + if (!region.get()) + return unique_ptr(); - unique_ptr table(new RankTableV1()); - uint8_t const flags = region->ImmutableData()[kFlagsOffset]; - if (SameEndianness(flags)) - coding::Map(table->m_coding, region->ImmutableData() + kHeaderSize, "SimpleDenseCoding"); - else - coding::ReverseMap(table->m_coding, region->MutableData() + kHeaderSize, "SimpleDenseCoding"); - table->m_region = move(region); + unique_ptr table; + switch (CheckEndianness(MemReader(region->ImmutableData(), region->Size()))) + { + case CheckResult::CorruptedHeader: + break; + case CheckResult::EndiannessMismatch: + table.reset(new RankTableV0()); + coding::ReverseMap(table->m_coding, region->MutableData() + kHeaderSize, + "SimpleDenseCoding"); + table->m_region = move(region); + break; + case CheckResult::EndiannessMatch: + table.reset(new RankTableV0()); + coding::Map(table->m_coding, region->ImmutableData() + kHeaderSize, "SimpleDenseCoding"); + table->m_region = move(region); + break; + } return table; } @@ -181,11 +206,11 @@ void SerializeRankTable(RankTable & table, FilesContainerW & wcont) // Deserializes rank table from a rank section. Returns null when it's // not possible to load a rank table (no rank section, corrupted -// header, endianness mismatch for a mapped mwm).. +// header, endianness mismatch for a mapped mwm). template unique_ptr LoadRankTable(unique_ptr && region) { - if (!region || !region->ImmutableData() || region->Size() < 8) + if (!region || !region->ImmutableData() || region->Size() < kHeaderSize) { LOG(LERROR, ("Invalid RankTable format.")); return unique_ptr(); @@ -195,8 +220,8 @@ unique_ptr LoadRankTable(unique_ptr && region) static_cast(region->ImmutableData()[kVersionOffset]); switch (version) { - case RankTable::V1: - return RankTableV1::Load(move(region)); + case RankTable::V0: + return RankTableV0::Load(move(region)); } return unique_ptr(); } @@ -216,8 +241,6 @@ uint8_t CalcSearchRank(FeatureType const & ft) } } // namespace -RankTable::~RankTable() {} - // static unique_ptr RankTable::Load(FilesContainerR & rcont) { @@ -234,10 +257,7 @@ unique_ptr RankTable::Load(FilesMappingContainer & mcont) void RankTableBuilder::CalcSearchRanks(FilesContainerR & rcont, vector & ranks) { feature::DataHeader header(rcont); - unique_ptr offsetsTable = - feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(rcont); - ASSERT(offsetsTable.get(), ()); - FeaturesVector featuresVector(rcont, header, offsetsTable.get()); + FeaturesVector featuresVector(rcont, header, nullptr /* features offsets table */); featuresVector.ForEach([&ranks](FeatureType const & ft, uint32_t /* index */) { @@ -255,23 +275,25 @@ void RankTableBuilder::Create(platform::LocalCountryFile const & localFile) FilesContainerR rcont(mapPath); if (rcont.IsExist(RANKS_FILE_TAG)) { - auto reader = rcont.GetReader(RANKS_FILE_TAG); - if (reader.Size() >= kHeaderSize) + switch (CheckEndianness(rcont.GetReader(RANKS_FILE_TAG))) { - uint8_t flags; - reader.Read(kFlagsOffset, &flags, sizeof(flags)); - - if (SameEndianness(flags)) + case CheckResult::CorruptedHeader: { - // Feature rank table already exists and has correct - // endianess. Nothing to do here. + // Worst case - we need to create rank table from scratch. + break; + } + case CheckResult::EndiannessMismatch: + { + // Try to copy whole serialized data and instantiate table via reverse mapping. + auto region = GetMemoryRegionForTag(rcont, RANKS_FILE_TAG); + table = LoadRankTable(move(region)); + break; + } + case CheckResult::EndiannessMatch: + { + // Table exists and has proper format. Nothing to do here. return; } - - // Copy whole serialized table and try to deserialize it via - // reverse mapping. - auto region = GetMemoryRegionForTag(rcont, RANKS_FILE_TAG); - table = LoadRankTable(move(region)); } } @@ -281,19 +303,19 @@ void RankTableBuilder::Create(platform::LocalCountryFile const & localFile) { vector ranks; CalcSearchRanks(rcont, ranks); - table = make_unique(ranks); + table = make_unique(ranks); } } ASSERT(table.get(), ()); - FilesContainerW wcont(mapPath); + FilesContainerW wcont(mapPath, FileWriter::OP_WRITE_EXISTING); SerializeRankTable(*table, wcont); } // static void RankTableBuilder::Create(vector const & ranks, FilesContainerW & wcont) { - RankTableV1 table(ranks); + RankTableV0 table(ranks); SerializeRankTable(table, wcont); } } // namespace search diff --git a/indexer/rank_table.hpp b/indexer/rank_table.hpp index 8f9b94b486..113675bedb 100644 --- a/indexer/rank_table.hpp +++ b/indexer/rank_table.hpp @@ -27,29 +27,29 @@ namespace search // File offset (bytes) Field name Field size (bytes) // 0 version 1 // 1 flags 1 -// 2 data * +// 8 data * // // Flags bits: // 0 - endianess of the stored table, 1 if BigEndian, 0 otherwise. // [1, 8) - currently not used. // Data size and contents depend on the version, but note that data -// should always be 8-bytes aligned. Therefore, there're 6-bytes empty +// should always be 8-bytes aligned. Therefore, there is 6-bytes empty // area between flags and data. Feel free to use it if you need it. class RankTable { public: enum Version { - V1 = 0 + V0 = 0 }; - virtual ~RankTable(); + virtual ~RankTable() = default; // Returns rank of the i-th feature. virtual uint8_t Get(uint64_t i) const = 0; - // Returns total number of ranks (or features, as there're 1-1 correspondence). + // Returns total number of ranks (or features, as there is a 1-1 correspondence). virtual uint64_t Size() const = 0; // Returns underlying data format version. @@ -62,7 +62,7 @@ public: // deserializes it. Returns nullptr if there're no ranks section or // rank table's header is damaged. // - // *NOTE* Return value can outlive |rcont|. Also note that there're + // *NOTE* Return value can outlive |rcont|. Also note that there is // undefined behaviour if ranks section exists but internally // damaged. static unique_ptr Load(FilesContainerR & rcont); @@ -90,12 +90,13 @@ public: // * When rank table already exists and has proper endianness, does nothing. // * When rank table already exists but has improper endianness, re-creates it by // reverse mapping. - // * When rank table does not exists or exists but damaged, calculates all - // features's ranks and creates rank table. + // * When rank table does not exist or exists but is damaged, calculates all + // features' ranks and creates rank table. static void Create(platform::LocalCountryFile const & localFile); // Force creation of a rank table from array of ranks. Existing rank - // table is removed (if any). + // table is removed (if any). Note that |wcont| must be instantiated + // as FileWriter::OP_WRITE_EXISTING. static void Create(vector const & ranks, FilesContainerW & wcont); }; } // namespace search