From 15f3da89458518cbc2e27eb90279ffff0b26d71e Mon Sep 17 00:00:00 2001 From: vng Date: Mon, 25 Jan 2016 16:47:18 +0300 Subject: [PATCH] Review fixes. --- search/v2/features_layer_matcher.cpp | 60 +++++++++++++++------------- search/v2/features_layer_matcher.hpp | 10 ++--- search/v2/geocoder.cpp | 24 ++++++----- search/v2/stats_cache.hpp | 39 ++++++++++++------ search/v2/street_vicinity_loader.cpp | 10 +++-- search/v2/street_vicinity_loader.hpp | 6 +-- 6 files changed, 86 insertions(+), 63 deletions(-) diff --git a/search/v2/features_layer_matcher.cpp b/search/v2/features_layer_matcher.cpp index daad1192a2..25102c56b1 100644 --- a/search/v2/features_layer_matcher.cpp +++ b/search/v2/features_layer_matcher.cpp @@ -16,73 +16,77 @@ double const FeaturesLayerMatcher::kBuildingRadiusMeters = 50; FeaturesLayerMatcher::FeaturesLayerMatcher(Index & index, my::Cancellable const & cancellable) : m_context(nullptr) , m_reverseGeocoder(index) - , m_nearbyStreetsCache("F2NS") - , m_matchingStreetsCache("B2S") + , m_nearbyStreetsCache("FeatureToNearbyStreets") + , m_matchingStreetsCache("BuildingToStreet") , m_loader(scales::GetUpperScale(), ReverseGeocoder::kLookupRadiusM) , m_cancellable(cancellable) { } -void FeaturesLayerMatcher::InitContext(MwmContext * context) +void FeaturesLayerMatcher::SetContext(MwmContext * context) { + ASSERT(context, ()); + if (m_context == context) + return; + m_context = context; m_houseToStreetTable = HouseToStreetTable::Load(m_context->m_value); ASSERT(m_houseToStreetTable, ()); - m_loader.InitContext(context); + m_loader.SetContext(context); } -void FeaturesLayerMatcher::FinishQuery() +void FeaturesLayerMatcher::OnQueryFinished() { - m_nearbyStreetsCache.FinishQuery(); - m_matchingStreetsCache.FinishQuery(); - m_loader.FinishQuery(); + m_nearbyStreetsCache.ClearIfNeeded(); + m_matchingStreetsCache.ClearIfNeeded(); + m_loader.OnQueryFinished(); } uint32_t FeaturesLayerMatcher::GetMatchingStreet(uint32_t houseId) { - auto r = m_matchingStreetsCache.Get(houseId); - if (!r.second) - return r.first; + auto entry = m_matchingStreetsCache.Get(houseId); + if (!entry.second) + return entry.first; FeatureType houseFeature; GetByIndex(houseId, houseFeature); - r.first = GetMatchingStreetImpl(houseId, houseFeature); - return r.first; + entry.first = GetMatchingStreetImpl(houseId, houseFeature); + return entry.first; } uint32_t FeaturesLayerMatcher::GetMatchingStreet(uint32_t houseId, FeatureType & houseFeature) { - auto r = m_matchingStreetsCache.Get(houseId); - if (!r.second) - return r.first; + auto entry = m_matchingStreetsCache.Get(houseId); + if (!entry.second) + return entry.first; - r.first = GetMatchingStreetImpl(houseId, houseFeature); - return r.first; + entry.first = GetMatchingStreetImpl(houseId, houseFeature); + return entry.first; } vector const & FeaturesLayerMatcher::GetNearbyStreets(uint32_t featureId) { - auto r = m_nearbyStreetsCache.Get(featureId); - if (!r.second) - return r.first; + auto entry = m_nearbyStreetsCache.Get(featureId); + if (!entry.second) + return entry.first; FeatureType feature; GetByIndex(featureId, feature); - m_reverseGeocoder.GetNearbyStreets(feature::GetCenter(feature), r.first); - return r.first; + m_reverseGeocoder.GetNearbyStreets(feature::GetCenter(feature), entry.first); + return entry.first; } vector const & FeaturesLayerMatcher::GetNearbyStreets( uint32_t featureId, FeatureType & feature) { - auto r = m_nearbyStreetsCache.Get(featureId); - if (!r.second) - return r.first; + auto entry = m_nearbyStreetsCache.Get(featureId); + if (!entry.second) + return entry.first; - m_reverseGeocoder.GetNearbyStreets(feature::GetCenter(feature), r.first); - return r.first; + m_reverseGeocoder.GetNearbyStreets(feature::GetCenter(feature), entry.first); + return entry.first; } uint32_t FeaturesLayerMatcher::GetMatchingStreetImpl(uint32_t houseId, FeatureType & houseFeature) diff --git a/search/v2/features_layer_matcher.hpp b/search/v2/features_layer_matcher.hpp index f1ba85e0ca..62536c2526 100644 --- a/search/v2/features_layer_matcher.hpp +++ b/search/v2/features_layer_matcher.hpp @@ -59,7 +59,7 @@ public: static double const kBuildingRadiusMeters; FeaturesLayerMatcher(Index & index, my::Cancellable const & cancellable); - void InitContext(MwmContext * context); + void SetContext(MwmContext * context); template void Match(FeaturesLayer const & child, FeaturesLayer const & parent, TFn && fn) @@ -91,7 +91,7 @@ public: } } - void FinishQuery(); + void OnQueryFinished(); private: template @@ -333,7 +333,7 @@ private: inline void GetByIndex(uint32_t id, FeatureType & ft) const { - /// @todo Add StatsCache for feature id -> (point, name / house number). + /// @todo Add Cache for feature id -> (point, name / house number). m_context->m_vector.GetByIndex(id, ft); } @@ -343,12 +343,12 @@ private: // Cache of streets in a feature's vicinity. All lists in the cache // are ordered by distance from the corresponding feature. - StatsCache> m_nearbyStreetsCache; + Cache> m_nearbyStreetsCache; // Cache of correct streets for buildings. Current search algorithm // supports only one street for a building, whereas buildings can be // located on multiple streets. - StatsCache m_matchingStreetsCache; + Cache m_matchingStreetsCache; unique_ptr m_houseToStreetTable; diff --git a/search/v2/geocoder.cpp b/search/v2/geocoder.cpp index 3527d184d4..aeb07ab353 100644 --- a/search/v2/geocoder.cpp +++ b/search/v2/geocoder.cpp @@ -294,6 +294,7 @@ Geocoder::Geocoder(Index & index, storage::CountryInfoGetter const & infoGetter) , m_numTokens(0) , m_model(SearchModel::Instance()) , m_streets(nullptr) + , m_matcher(nullptr) , m_finder(static_cast(*this)) , m_results(nullptr) { @@ -426,34 +427,35 @@ void Geocoder::GoImpl(vector> & infos, bool inViewport) // MatchViewportAndPosition() should always be matched in mwms // intersecting with position and viewport. + auto const & cancellable = static_cast(*this); auto processCountry = [&](size_t index, unique_ptr context) { ASSERT(context, ()); m_context = move(context); MY_SCOPE_GUARD(cleanup, [&]() { - LOG(LDEBUG, ("Search results for", m_context->GetMwmName())); - m_matcher->FinishQuery(); + LOG(LDEBUG, (m_context->GetMwmName(), "processing complete.")); + m_matcher->OnQueryFinished(); m_matcher = nullptr; m_context.reset(); m_addressFeatures.clear(); m_streets = nullptr; }); - auto iMatcher = m_matchersCache.find(m_context->m_id); - if (iMatcher == m_matchersCache.end()) + auto it = m_matchersCache.find(m_context->m_id); + if (it == m_matchersCache.end()) { - iMatcher = m_matchersCache.insert(make_pair(m_context->m_id, make_unique( - m_index, *this /* cancellable */))).first; + it = m_matchersCache.insert(make_pair(m_context->m_id, make_unique( + m_index, cancellable))).first; } - m_matcher = iMatcher->second.get(); - m_matcher->InitContext(m_context.get()); + m_matcher = it->second.get(); + m_matcher->SetContext(m_context.get()); unique_ptr viewportCBV; if (inViewport) { viewportCBV = Retrieval::RetrieveGeometryFeatures( - m_context->m_value, static_cast(*this), + m_context->m_value, cancellable, m_params.m_viewport, m_params.m_scale); } @@ -464,7 +466,7 @@ void Geocoder::GoImpl(vector> & infos, bool inViewport) PrepareRetrievalParams(i, i + 1); m_addressFeatures[i] = Retrieval::RetrieveAddressFeatures( - m_context->m_value, *this /* cancellable */, m_retrievalParams); + m_context->m_value, cancellable, m_retrievalParams); ASSERT(m_addressFeatures[i], ()); if (viewportCBV) @@ -726,7 +728,7 @@ void Geocoder::MatchRegions(RegionType type) auto const & regions = m_regions[type]; - auto const & fileName = m_context->m_handle.GetId().GetInfo()->GetCountryName(); + auto const & fileName = m_context->GetMwmName(); // Try to match regions. for (auto const & p : regions) diff --git a/search/v2/stats_cache.hpp b/search/v2/stats_cache.hpp index ff875a539d..42cfee0e0e 100644 --- a/search/v2/stats_cache.hpp +++ b/search/v2/stats_cache.hpp @@ -2,6 +2,7 @@ #include "base/logging.hpp" #include "std/unordered_map.hpp" +#include "std/utility.hpp" namespace search @@ -10,22 +11,30 @@ namespace v2 { template -class StatsCache +class Cache { unordered_map m_map; - size_t m_count, m_new, m_emptyCount; - char const * m_name; + + /// query statistics + size_t m_accesses; + size_t m_misses; + + size_t m_emptyQueriesCount; /// empty queries count at a row + string m_name; /// cache name for print functions public: - explicit StatsCache(char const * name) : m_count(0), m_new(0), m_emptyCount(0), m_name(name) {} + explicit Cache(string const & name) + : m_accesses(0), m_misses(0), m_emptyQueriesCount(0), m_name(name) + { + } pair Get(TKey const & key) { auto r = m_map.insert(make_pair(key, TValue())); - ++m_count; + ++m_accesses; if (r.second) - ++m_new; + ++m_misses; return pair(r.first->second, r.second); } @@ -33,20 +42,24 @@ public: void Clear() { m_map.clear(); - m_count = m_new = 0; + m_accesses = m_misses = 0; + m_emptyQueriesCount = 0; } - void FinishQuery() + /// Called at the end of every search query. + void ClearIfNeeded() { - if (m_count != 0) + if (m_accesses != 0) { - LOG(LDEBUG, ("Cache", m_name, "Queries =", m_count, "From cache =", m_count - m_new, "Added =", m_new)); - m_count = m_new = 0; + LOG(LDEBUG, ("Cache", m_name, "Queries =", m_accesses, + "From cache =", m_accesses - m_misses, "Added =", m_misses)); + m_accesses = m_misses = 0; + m_emptyQueriesCount = 0; } - else if (++m_emptyCount > 5) + else if (++m_emptyQueriesCount > 5) { + LOG(LDEBUG, ("Clearing cache", m_name)); Clear(); - m_emptyCount = 0; } } }; diff --git a/search/v2/street_vicinity_loader.cpp b/search/v2/street_vicinity_loader.cpp index f2fdf6e78f..b89aa6b0e5 100644 --- a/search/v2/street_vicinity_loader.cpp +++ b/search/v2/street_vicinity_loader.cpp @@ -20,16 +20,20 @@ StreetVicinityLoader::StreetVicinityLoader(int scale, double offsetMeters) { } -void StreetVicinityLoader::InitContext(MwmContext * context) +void StreetVicinityLoader::SetContext(MwmContext * context) { + ASSERT(context, ()); + if (m_context == context) + return; + m_context = context; auto const scaleRange = m_context->m_value.GetHeader().GetScaleRange(); m_scale = my::clamp(m_scale, scaleRange.first, scaleRange.second); } -void StreetVicinityLoader::FinishQuery() +void StreetVicinityLoader::OnQueryFinished() { - m_cache.FinishQuery(); + m_cache.ClearIfNeeded(); } StreetVicinityLoader::Street const & StreetVicinityLoader::GetStreet(uint32_t featureId) diff --git a/search/v2/street_vicinity_loader.hpp b/search/v2/street_vicinity_loader.hpp index a2bbb0cdc2..f59698babd 100644 --- a/search/v2/street_vicinity_loader.hpp +++ b/search/v2/street_vicinity_loader.hpp @@ -41,7 +41,7 @@ public: }; StreetVicinityLoader(int scale, double offsetMeters); - void InitContext(MwmContext * context); + void SetContext(MwmContext * context); // Calls |fn| on each index in |sortedIds| where sortedIds[index] // belongs to the street's vicinity. @@ -69,7 +69,7 @@ public: } } - void FinishQuery(); + void OnQueryFinished(); Street const & GetStreet(uint32_t featureId); @@ -80,7 +80,7 @@ private: int m_scale; double const m_offsetMeters; - StatsCache m_cache; + Cache m_cache; DISALLOW_COPY_AND_MOVE(StreetVicinityLoader); };