From 8150a4c98d9e54589df6c4b2ba46e26c22b6f487 Mon Sep 17 00:00:00 2001 From: Denis Koronchik Date: Fri, 19 Sep 2014 12:54:38 +0300 Subject: [PATCH] [core] Refactoring of FilesMappingContainer Handle: using move semantics to control ownership. --- coding/coding_tests/file_container_test.cpp | 39 +++++++++++++++++---- coding/file_container.cpp | 38 +++++++++++++++----- coding/file_container.hpp | 27 +++++++++++--- routing/osrm2feature_map.cpp | 23 ++++-------- routing/osrm2feature_map.hpp | 4 +-- routing/osrm_data_facade.hpp | 24 +++++-------- 6 files changed, 101 insertions(+), 54 deletions(-) diff --git a/coding/coding_tests/file_container_test.cpp b/coding/coding_tests/file_container_test.cpp index 40360981b0..5f8d77e830 100644 --- a/coding/coding_tests/file_container_test.cpp +++ b/coding/coding_tests/file_container_test.cpp @@ -205,6 +205,34 @@ UNIT_TEST(FilesContainer_RewriteExisting) FileWriter::DeleteFileX(fName); } +UNIT_TEST(FilesMappingContainer_Handle) +{ + string const fName = "file_container.tmp"; + string const tag = "dummy"; + + { + FilesContainerW writer(fName); + FileWriter w = writer.GetWriter(tag); + w.Write(tag.c_str(), tag.size()); + } + + { + FilesMappingContainer cont(fName); + + FilesMappingContainer::Handle h1 = cont.Map(tag); + TEST(h1.IsValid(), ()); + + FilesMappingContainer::Handle h2; + TEST(!h2.IsValid(), ()); + + h2.Assign(std::move(h1)); + TEST(!h1.IsValid(), ()); + TEST(h2.IsValid(), ()); + } + + FileWriter::DeleteFileX(fName); +} + UNIT_TEST(FilesMappingContainer_Smoke) { string const fName = "file_container.tmp"; @@ -228,10 +256,12 @@ UNIT_TEST(FilesMappingContainer_Smoke) { FilesMappingContainer reader(fName); + for (size_t i = 0; i < ARRAY_SIZE(key); ++i) { FilesMappingContainer::Handle h = reader.Map(key[i]); - uint32_t const * data = reinterpret_cast(h.GetData()); + uint32_t const * data = h.GetData(); + for (uint32_t j = 0; j < count; ++j) { TEST_EQUAL(j + i, *data, ()); @@ -274,19 +304,16 @@ UNIT_TEST(FilesMappingContainer_PageSize) for (size_t i = 0; i < sz; ++i) { - handle[i] = reader.Map(key[i]); + handle[i].Assign(reader.Map(key[i])); TEST_EQUAL(handle[i].GetSize(), count[i], ()); } for (size_t i = 0; i < sz; ++i) { - char const * data = handle[i].GetData(); + char const * data = handle[i].GetData(); for (size_t j = 0; j < count[i]; ++j) TEST_EQUAL(*data++, byte[j % ARRAY_SIZE(byte)], ()); } - - for (size_t i = 0; i < sz; ++i) - handle[i].Unmap(); } FileWriter::DeleteFileX(fName); diff --git a/coding/file_container.cpp b/coding/file_container.cpp index 40c64f7e64..39a9af5583 100644 --- a/coding/file_container.cpp +++ b/coding/file_container.cpp @@ -126,11 +126,11 @@ FilesMappingContainer::Handle FilesMappingContainer::Map(Tag const & tag) const uint64_t const length = p->m_size + (p->m_offset - offset); ASSERT_GREATER_OR_EQUAL(length, p->m_size, ()); - char const * data = reinterpret_cast(mmap(0, length, PROT_READ, MAP_SHARED, m_fd, offset)); - - if (data == reinterpret_cast(-1)) + void * pMap = mmap(0, length, PROT_READ, MAP_SHARED, m_fd, offset); + if (pMap == MAP_FAILED) MYTHROW(Reader::OpenException, ("Can't map section:", tag, "with [offset, size]:", *p)); + char const * data = reinterpret_cast(pMap); char const * d = data + (p->m_offset - offset); return Handle(d, data, p->m_size, length); } @@ -140,18 +140,40 @@ FilesMappingContainer::Handle FilesMappingContainer::Map(Tag const & tag) const return Handle(); } +///////////////////////////////////////////////////////////////////////////// +// FilesMappingContainer::Handle +///////////////////////////////////////////////////////////////////////////// + FilesMappingContainer::Handle::~Handle() { -// CHECK(!IsValid(), ()); + Unmap(); +} + +void FilesMappingContainer::Handle::Assign(Handle && h) +{ + Unmap(); + + m_base = h.m_base; + m_origBase = h.m_origBase; + m_size = h.m_size; + m_origSize = h.m_origSize; + + h.Reset(); } void FilesMappingContainer::Handle::Unmap() { - ASSERT(IsValid(), ()); - VERIFY(0 == munmap((void*)m_origBase, m_origSize), ()); + if (IsValid()) + { + VERIFY(0 == munmap((void*)m_origBase, m_origSize), ()); + Reset(); + } +} - m_origBase = m_base = 0; - m_origSize = m_size = 0; +void FilesMappingContainer::Handle::Reset() +{ + m_base = m_origBase = 0; + m_size = m_origSize = 0; } ///////////////////////////////////////////////////////////////////////////// diff --git a/coding/file_container.hpp b/coding/file_container.hpp index a9d8011098..96f1dfe518 100644 --- a/coding/file_container.hpp +++ b/coding/file_container.hpp @@ -4,6 +4,7 @@ #include "../std/vector.hpp" #include "../std/string.hpp" +#include "../std/noncopyable.hpp" class FilesContainerBase @@ -119,26 +120,42 @@ public: explicit FilesMappingContainer(string const & fName); ~FilesMappingContainer(); - class Handle + class Handle : private noncopyable { + void Reset(); + public: Handle() : m_base(0), m_origBase(0), m_size(0), m_origSize(0) { } - Handle(char const * base, char const * alignBase, uint64_t size, uint64_t origSize) : m_base(base), m_origBase(alignBase), m_size(size), m_origSize(origSize) { } - + Handle(Handle && h) + { + Assign(std::move(h)); + } ~Handle(); + void Assign(Handle && h); + void Unmap(); - bool IsValid() const { return (m_base != 0 && m_size > 0); } + bool IsValid() const { return (m_base != 0); } uint64_t GetSize() const { return m_size; } - char const * GetData() const { return m_base; } + + template T const * GetData() const + { + ASSERT_EQUAL(m_size % sizeof(T), 0, ()); + return reinterpret_cast(m_base); + } + template size_t GetDataCount() const + { + ASSERT_EQUAL(m_size % sizeof(T), 0, ()); + return (m_size / sizeof(T)); + } private: char const * m_base; diff --git a/routing/osrm2feature_map.cpp b/routing/osrm2feature_map.cpp index 05e98c97b2..1cb0cd807a 100644 --- a/routing/osrm2feature_map.cpp +++ b/routing/osrm2feature_map.cpp @@ -73,40 +73,31 @@ string DebugPrint(OsrmFtSegMapping::FtSeg const & seg) } -OsrmFtSegMapping::~OsrmFtSegMapping() -{ - if (m_handle.IsValid()) - m_handle.Unmap(); -} - void OsrmFtSegMapping::Clear() { m_offsets.clear(); - - if (m_handle.IsValid()) - m_handle.Unmap(); + m_handle.Unmap(); } void OsrmFtSegMapping::Load(FilesMappingContainer & cont) { Clear(); + /// @todo To reduce memory pressure we can use regular Reader to load m_offsets. + /// Also we can try to do mapping here. FilesMappingContainer::Handle h = cont.Map(ROUTING_NODEIND_TO_FTSEGIND_FILE_TAG); - SegOffset const * p = reinterpret_cast(h.GetData()); - - ASSERT_EQUAL(h.GetSize() % sizeof(SegOffset), 0, ()); - m_offsets.assign(p, p + h.GetSize() / sizeof(SegOffset)); + SegOffset const * p = h.GetData(); + m_offsets.assign(p, p + h.GetDataCount()); h.Unmap(); - m_handle = cont.Map(ROUTING_FTSEG_FILE_TAG); + m_handle.Assign(cont.Map(ROUTING_FTSEG_FILE_TAG)); } size_t OsrmFtSegMapping::GetSegmentsCount() const { - ASSERT_EQUAL(m_handle.GetSize() % sizeof(FtSeg), 0, ()); - return m_handle.GetSize() / sizeof(FtSeg); + return m_handle.GetDataCount(); } pair OsrmFtSegMapping::GetSegVector(OsrmNodeIdT nodeId) const diff --git a/routing/osrm2feature_map.hpp b/routing/osrm2feature_map.hpp index 0ddf3240f6..755707ddab 100644 --- a/routing/osrm2feature_map.hpp +++ b/routing/osrm2feature_map.hpp @@ -61,8 +61,6 @@ public: }; #pragma pack (pop) - ~OsrmFtSegMapping(); - void Clear(); void Load(FilesMappingContainer & cont); @@ -80,7 +78,7 @@ public: pair GetSegmentsRange(uint32_t nodeId) const; OsrmNodeIdT GetNodeId(size_t segInd) const; - FtSeg const * GetSegments() const { return reinterpret_cast(m_handle.GetData()); } + FtSeg const * GetSegments() const { return m_handle.GetData(); } size_t GetSegmentsCount() const; //@} diff --git a/routing/osrm_data_facade.hpp b/routing/osrm_data_facade.hpp index 065a437934..86af7927f3 100644 --- a/routing/osrm_data_facade.hpp +++ b/routing/osrm_data_facade.hpp @@ -40,33 +40,25 @@ public: OsrmDataFacade(FilesMappingContainer const & container) : m_container(container) { - m_handleEdgeData = m_container.Map(ROUTING_EDGEDATA_FILE_TAG); + m_handleEdgeData.Assign(m_container.Map(ROUTING_EDGEDATA_FILE_TAG)); ASSERT(m_handleEdgeData.IsValid(), ()); - succinct::mapper::map(m_edgeData, m_handleEdgeData.GetData()); + succinct::mapper::map(m_edgeData, m_handleEdgeData.GetData()); - m_handleEdgeId = m_container.Map(ROUTING_EDGEID_FILE_TAG); + m_handleEdgeId.Assign(m_container.Map(ROUTING_EDGEID_FILE_TAG)); ASSERT(m_handleEdgeId.IsValid(), ()); - succinct::mapper::map(m_edgeId, m_handleEdgeId.GetData()); + succinct::mapper::map(m_edgeId, m_handleEdgeId.GetData()); - m_handleShortcuts = m_container.Map(ROUTING_SHORTCUTS_FILE_TAG); + m_handleShortcuts.Assign(m_container.Map(ROUTING_SHORTCUTS_FILE_TAG)); ASSERT(m_handleShortcuts.IsValid(), ()); - succinct::mapper::map(m_shortcuts, m_handleShortcuts.GetData()); + succinct::mapper::map(m_shortcuts, m_handleShortcuts.GetData()); - m_handleFanoMatrix = m_container.Map(ROUTING_MATRIX_FILE_TAG); + m_handleFanoMatrix.Assign(m_container.Map(ROUTING_MATRIX_FILE_TAG)); ASSERT(m_handleFanoMatrix.IsValid(), ()); - succinct::mapper::map(m_fanoMatrix, m_handleFanoMatrix.GetData()); + succinct::mapper::map(m_fanoMatrix, m_handleFanoMatrix.GetData()); m_numberOfNodes = (unsigned)sqrt(m_fanoMatrix.size() / 2) + 1; } - ~OsrmDataFacade() - { - m_handleEdgeData.Unmap(); - m_handleEdgeId.Unmap(); - m_handleFanoMatrix.Unmap(); - m_handleShortcuts.Unmap(); - } - unsigned GetNumberOfNodes() const {