From 85ee45b48220988cb098e20268000d361434c4e2 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Fri, 3 Jun 2016 00:29:19 +0300 Subject: [PATCH] Review fixes. --- coding/file_container.cpp | 14 ++++++++++++-- coding/reader.hpp | 1 + indexer/index.cpp | 30 +++++++++++++++++++----------- indexer/index.hpp | 23 ++++++++++++++++++++++- indexer/mwm_set.cpp | 17 ++++++++++++++++- indexer/mwm_set.hpp | 6 +++--- 6 files changed, 73 insertions(+), 18 deletions(-) diff --git a/coding/file_container.cpp b/coding/file_container.cpp index ebe1fbaa51..e37dc86731 100644 --- a/coding/file_container.cpp +++ b/coding/file_container.cpp @@ -6,7 +6,6 @@ #include "std/cstring.hpp" #ifndef OMIM_OS_WINDOWS - #include #include #include #include @@ -20,6 +19,7 @@ #include #endif +#include template void Read(TSource & src, InfoT & i) { @@ -124,7 +124,17 @@ void MappedFile::Open(string const & fName) #else m_fd = open(fName.c_str(), O_RDONLY | O_NONBLOCK); if (m_fd == -1) - MYTHROW(Reader::OpenException, ("Can't open file:", fName, ", reason:", strerror(errno))); + { + if (errno == EMFILE || errno == ENFILE) + { + MYTHROW(Reader::TooManyFilesException, + ("Can't open file:", fName, ", reason:", strerror(errno))); + } + else + { + MYTHROW(Reader::OpenException, ("Can't open file:", fName, ", reason:", strerror(errno))); + } + } #endif } diff --git a/coding/reader.hpp b/coding/reader.hpp index bfc03cc797..df70e03143 100644 --- a/coding/reader.hpp +++ b/coding/reader.hpp @@ -19,6 +19,7 @@ public: DECLARE_EXCEPTION(OpenException, Exception); DECLARE_EXCEPTION(SizeException, Exception); DECLARE_EXCEPTION(ReadException, Exception); + DECLARE_EXCEPTION(TooManyFilesException, Exception); virtual ~Reader() {} virtual uint64_t Size() const = 0; diff --git a/indexer/index.cpp b/indexer/index.cpp index 516e460196..830faa1adf 100644 --- a/indexer/index.cpp +++ b/indexer/index.cpp @@ -20,14 +20,23 @@ MwmValue::MwmValue(LocalCountryFile const & localFile) : m_cont(platform::GetCountryReader(localFile, MapOptions::Map)), m_file(localFile) { m_factory.Load(m_cont); +} +void MwmValue::SetTable(MwmInfoEx & info) +{ auto const version = GetHeader().GetFormat(); if (version < version::Format::v5) - ; - else if (version == version::Format::v5) - m_table = feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(m_file, m_cont); - else - m_table = feature::FeaturesOffsetsTable::Load(m_cont); + return; + + m_table = info.m_table.lock(); + if (!m_table) + { + if (version == version::Format::v5) + m_table = feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(m_file, m_cont); + else + m_table = feature::FeaturesOffsetsTable::Load(m_cont); + info.m_table = m_table; + } } ////////////////////////////////////////////////////////////////////////////////// @@ -42,7 +51,7 @@ unique_ptr Index::CreateInfo(platform::LocalCountryFile const & localFi if (!h.IsMWMSuitable()) return nullptr; - auto info = make_unique(); + auto info = make_unique(); info->m_limitRect = h.GetBounds(); pair const scaleR = h.GetScaleRange(); @@ -50,7 +59,7 @@ unique_ptr Index::CreateInfo(platform::LocalCountryFile const & localFi info->m_maxScale = static_cast(scaleR.second); info->m_version = value.GetMwmVersion(); - return info; + return unique_ptr(move(info)); } unique_ptr Index::CreateValue(MwmInfo & info) const @@ -58,6 +67,7 @@ unique_ptr Index::CreateValue(MwmInfo & info) const // Create a section with rank table if it does not exist. platform::LocalCountryFile const & localFile = info.GetLocalFile(); unique_ptr p(new MwmValue(localFile)); + p->SetTable(dynamic_cast(info)); ASSERT(p->GetHeader().IsMWMSuitable(), ()); return unique_ptr(move(p)); } @@ -75,10 +85,8 @@ bool Index::DeregisterMap(CountryFile const & countryFile) { return Deregister(c Index::FeaturesLoaderGuard::FeaturesLoaderGuard(Index const & parent, MwmId const & id) : m_handle(parent.GetMwmHandleById(id)) - , - /// @note This guard is suitable when mwm is loaded - m_vector(m_handle.GetValue()->m_cont, m_handle.GetValue()->GetHeader(), - m_handle.GetValue()->m_table.get()) + , m_vector(m_handle.GetValue()->m_cont, m_handle.GetValue()->GetHeader(), + m_handle.GetValue()->m_table.get()) { } diff --git a/indexer/index.hpp b/indexer/index.hpp index 09d61b9f1d..c0c997a7c0 100644 --- a/indexer/index.hpp +++ b/indexer/index.hpp @@ -19,6 +19,24 @@ #include "std/limits.hpp" #include "std/utility.hpp" #include "std/vector.hpp" +#include "std/weak_ptr.hpp" + +class MwmInfoEx : public MwmInfo +{ +private: + friend class Index; + friend class MwmValue; + + // weak_ptr is needed here to access offsets table in already + // instantiated MwmValue-s for the MWM, including MwmValues in the + // MwmSet's cache. We can't use shared_ptr because of offsets table + // must be removed as soon as the last corresponding MwmValue is + // destroyed. Also, note that this value must be used and modified + // only in MwmValue::SetTable() method, which, in turn, is called + // only in the MwmSet critical section, protected by a lock. So, + // there's an implicit synchronization on this field. + weak_ptr m_table; +}; class MwmValue : public MwmSet::MwmValueBase { @@ -26,9 +44,11 @@ public: FilesContainerR const m_cont; IndexFactory m_factory; platform::LocalCountryFile const m_file; - unique_ptr m_table; + + shared_ptr m_table; explicit MwmValue(platform::LocalCountryFile const & localFile); + void SetTable(MwmInfoEx & info); inline feature::DataHeader const & GetHeader() const { return m_factory.GetHeader(); } inline version::MwmVersion const & GetMwmVersion() const { return m_factory.GetMwmVersion(); } @@ -246,6 +266,7 @@ public: } /// Guard for loading features from particular MWM by demand. + /// @note This guard is suitable when mwm is loaded. class FeaturesLoaderGuard { public: diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index ba35b57051..a3e446dd55 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -1,7 +1,7 @@ #include "indexer/mwm_set.hpp" #include "indexer/scales.hpp" -#include "defines.hpp" +#include "coding/reader.hpp" #include "base/assert.hpp" #include "base/exception.hpp" @@ -11,6 +11,7 @@ #include "std/algorithm.hpp" #include "std/sstream.hpp" +#include "defines.hpp" using platform::CountryFile; using platform::LocalCountryFile; @@ -162,6 +163,14 @@ bool MwmSet::DeregisterImpl(MwmId const & id, EventList & events) SetStatus(*info, MwmInfo::STATUS_DEREGISTERED, events); vector> & infos = m_info[info->GetCountryName()]; infos.erase(remove(infos.begin(), infos.end(), info), infos.end()); + for (auto it = m_cache.begin(); it != m_cache.end(); ++it) + { + if (it->first == id) + { + m_cache.erase(it); + break; + } + } return true; } @@ -284,6 +293,12 @@ unique_ptr MwmSet::LockValueImpl(MwmId const & id, EventLi { return CreateValue(*info); } + catch (Reader::TooManyFilesException const & ex) + { + LOG(LERROR, ("Too many open files, can't open:", info->GetCountryName())); + --info->m_numRefs; + return nullptr; + } catch (exception const & ex) { LOG(LERROR, ("Can't create MWMValue for", info->GetCountryName(), "Reason", ex.what())); diff --git a/indexer/mwm_set.hpp b/indexer/mwm_set.hpp index 9423077689..c924f5dd97 100644 --- a/indexer/mwm_set.hpp +++ b/indexer/mwm_set.hpp @@ -24,8 +24,8 @@ class MwmInfo { public: - friend class MwmSet; friend class Index; + friend class MwmSet; enum MwmTypeT { @@ -69,7 +69,7 @@ public: /// Returns the lock counter value for test needs. uint8_t GetNumRefs() const { return m_numRefs; } -private: +protected: inline Status SetStatus(Status status) { Status result = m_status; @@ -79,7 +79,7 @@ private: platform::LocalCountryFile m_file; ///< Path to the mwm file. atomic m_status; ///< Current country status. - uint8_t m_numRefs; ///< Number of active handles. + uint32_t m_numRefs; ///< Number of active handles. }; class MwmSet