From 573021d80952bb9b82af4bac6b03622c7231b936 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Thu, 2 Jun 2016 15:55:13 +0300 Subject: [PATCH 1/2] [index] Fixed massive descriptors leaks. --- coding/file_container.cpp | 4 +- indexer/index.cpp | 37 +++++++------------ indexer/index.hpp | 15 ++------ .../indexer_tests/features_vector_test.cpp | 2 +- indexer/mwm_set.hpp | 5 +-- search/mwm_context.cpp | 2 +- 6 files changed, 23 insertions(+), 42 deletions(-) diff --git a/coding/file_container.cpp b/coding/file_container.cpp index 35f5733323..ebe1fbaa51 100644 --- a/coding/file_container.cpp +++ b/coding/file_container.cpp @@ -3,6 +3,8 @@ #include "coding/write_to_sink.hpp" #include "coding/internal/file_data.hpp" +#include "std/cstring.hpp" + #ifndef OMIM_OS_WINDOWS #include #include @@ -122,7 +124,7 @@ 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)); + MYTHROW(Reader::OpenException, ("Can't open file:", fName, ", reason:", strerror(errno))); #endif } diff --git a/indexer/index.cpp b/indexer/index.cpp index 592e8d6cdb..516e460196 100644 --- a/indexer/index.cpp +++ b/indexer/index.cpp @@ -17,27 +17,17 @@ using platform::LocalCountryFile; ////////////////////////////////////////////////////////////////////////////////// MwmValue::MwmValue(LocalCountryFile const & localFile) - : m_cont(platform::GetCountryReader(localFile, MapOptions::Map)), - m_file(localFile), - m_table(0) + : 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) - return; - - if (!info.m_table) - { - if (version == version::Format::v5) - info.m_table = feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(m_file, m_cont); - else - info.m_table = feature::FeaturesOffsetsTable::Load(m_cont); - } - m_table = info.m_table.get(); + ; + else if (version == version::Format::v5) + m_table = feature::FeaturesOffsetsTable::CreateIfNotExistsAndLoad(m_file, m_cont); + else + m_table = feature::FeaturesOffsetsTable::Load(m_cont); } ////////////////////////////////////////////////////////////////////////////////// @@ -52,7 +42,7 @@ unique_ptr Index::CreateInfo(platform::LocalCountryFile const & localFi if (!h.IsMWMSuitable()) return nullptr; - unique_ptr info(new MwmInfoEx()); + auto info = make_unique(); info->m_limitRect = h.GetBounds(); pair const scaleR = h.GetScaleRange(); @@ -60,7 +50,7 @@ unique_ptr Index::CreateInfo(platform::LocalCountryFile const & localFi info->m_maxScale = static_cast(scaleR.second); info->m_version = value.GetMwmVersion(); - return unique_ptr(move(info)); + return info; } unique_ptr Index::CreateValue(MwmInfo & info) const @@ -68,7 +58,6 @@ 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)); } @@ -85,11 +74,11 @@ 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) + : 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()) { } diff --git a/indexer/index.hpp b/indexer/index.hpp index 7e4fa77153..09d61b9f1d 100644 --- a/indexer/index.hpp +++ b/indexer/index.hpp @@ -20,23 +20,15 @@ #include "std/utility.hpp" #include "std/vector.hpp" - -class MwmInfoEx : public MwmInfo -{ -public: - unique_ptr m_table; -}; - class MwmValue : public MwmSet::MwmValueBase { public: FilesContainerR const m_cont; IndexFactory m_factory; platform::LocalCountryFile const m_file; - feature::FeaturesOffsetsTable const * m_table; + unique_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(); } @@ -97,7 +89,7 @@ private: covering::IntervalsT const & interval = cov.Get(lastScale); // Prepare features reading. - FeaturesVector const fv(pValue->m_cont, header, pValue->m_table); + FeaturesVector const fv(pValue->m_cont, header, pValue->m_table.get()); ScaleIndex index(pValue->m_cont.GetReader(INDEX_FILE_TAG), pValue->m_factory); @@ -224,7 +216,8 @@ public: if (handle.IsAlive()) { MwmValue const * pValue = handle.GetValue(); - FeaturesVector const featureReader(pValue->m_cont, pValue->GetHeader(), pValue->m_table); + FeaturesVector const featureReader(pValue->m_cont, pValue->GetHeader(), + pValue->m_table.get()); do { osm::Editor::FeatureStatus const fts = editor.GetFeatureStatus(id, fidIter->m_index); diff --git a/indexer/indexer_tests/features_vector_test.cpp b/indexer/indexer_tests/features_vector_test.cpp index b991764899..975face9e0 100644 --- a/indexer/indexer_tests/features_vector_test.cpp +++ b/indexer/indexer_tests/features_vector_test.cpp @@ -59,7 +59,7 @@ UNIT_TEST(FeaturesVectorTest_ParseMetadata) TEST(handle.IsAlive(), ()); auto const * value = handle.GetValue(); - FeaturesVector fv(value->m_cont, value->GetHeader(), value->m_table); + FeaturesVector fv(value->m_cont, value->GetHeader(), value->m_table.get()); map actual; fv.ForEach([&](FeatureType & ft, uint32_t index) diff --git a/indexer/mwm_set.hpp b/indexer/mwm_set.hpp index f7ab054fa7..9423077689 100644 --- a/indexer/mwm_set.hpp +++ b/indexer/mwm_set.hpp @@ -112,10 +112,7 @@ public: }; public: - // Default value 32=2^5 was from the very begining. - // Later, we replaced my::Cache with the std::deque, but forgot to change - // logarithm constant 5 with actual size 32. Now it's fixed. - explicit MwmSet(size_t cacheSize = 32) : m_cacheSize(cacheSize) {} + explicit MwmSet(size_t cacheSize = 64) : m_cacheSize(cacheSize) {} virtual ~MwmSet() = default; class MwmValueBase diff --git a/search/mwm_context.cpp b/search/mwm_context.cpp index b89575d420..58e999994e 100644 --- a/search/mwm_context.cpp +++ b/search/mwm_context.cpp @@ -12,7 +12,7 @@ void CoverRect(m2::RectD const & rect, int scale, covering::IntervalsT & result) MwmContext::MwmContext(MwmSet::MwmHandle handle) : m_handle(move(handle)) , m_value(*m_handle.GetValue()) - , m_vector(m_value.m_cont, m_value.GetHeader(), m_value.m_table) + , m_vector(m_value.m_cont, m_value.GetHeader(), m_value.m_table.get()) , m_index(m_value.m_cont.GetReader(INDEX_FILE_TAG), m_value.m_factory) { } From 85ee45b48220988cb098e20268000d361434c4e2 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Fri, 3 Jun 2016 00:29:19 +0300 Subject: [PATCH 2/2] 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