From fd503ac72e67c886b21879760373478345889597 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Wed, 23 Sep 2015 14:24:59 +0300 Subject: [PATCH] [search] Fixed SearchEngine threading model. --- indexer/index.cpp | 18 +- indexer/index.hpp | 3 + .../Search/MWMSearchManager.mm | 1 - map/framework.cpp | 177 ++++++----- map/framework.hpp | 22 +- mapshot/mapshot.pro | 3 +- qt/mainwindow.cpp | 6 - search/intermediate_result.cpp | 8 +- search/intermediate_result.hpp | 5 +- search/result.cpp | 15 +- search/result.hpp | 3 +- search/search_engine.cpp | 279 ++++++++---------- search/search_engine.hpp | 95 ++++-- .../retrieval_test.cpp | 9 +- search/search_query.cpp | 6 +- search/search_query.hpp | 11 +- .../test_search_engine.cpp | 3 +- .../test_search_engine.hpp | 4 +- .../test_search_request.cpp | 2 +- 19 files changed, 377 insertions(+), 293 deletions(-) diff --git a/indexer/index.cpp b/indexer/index.cpp index e9b5524dbb..dfce9dce8e 100644 --- a/indexer/index.cpp +++ b/indexer/index.cpp @@ -65,15 +65,21 @@ unique_ptr Index::CreateInfo(platform::LocalCountryFile const & localFi unique_ptr Index::CreateValue(MwmInfo & info) const { + MwmInfoEx & infoEx = dynamic_cast(info); + // Create a section with rank table if it does not exist. platform::LocalCountryFile const & localFile = info.GetLocalFile(); - try + if (!infoEx.m_rankTableCtorCalled) { - search::RankTableBuilder::CreateIfNotExists(localFile); - } - catch (RootException const & e) - { - LOG(LWARNING, ("Can't create rank table for:", localFile, ":", e.Msg())); + infoEx.m_rankTableCtorCalled = true; + try + { + search::RankTableBuilder::CreateIfNotExists(localFile); + } + catch (RootException const & e) + { + LOG(LWARNING, ("Can't create rank table for:", localFile, ":", e.Msg())); + } } unique_ptr p(new MwmValue(localFile)); diff --git a/indexer/index.hpp b/indexer/index.hpp index e48ffad279..7cdf80daf1 100644 --- a/indexer/index.hpp +++ b/indexer/index.hpp @@ -24,7 +24,10 @@ class MwmInfoEx : public MwmInfo { public: + MwmInfoEx() : m_rankTableCtorCalled(false) {} + unique_ptr m_table; + bool m_rankTableCtorCalled; }; class MwmValue : public MwmSet::MwmValueBase diff --git a/iphone/Maps/Classes/CustomViews/MapViewControls/Search/MWMSearchManager.mm b/iphone/Maps/Classes/CustomViews/MapViewControls/Search/MWMSearchManager.mm index 43aa6dfacd..2069233b46 100644 --- a/iphone/Maps/Classes/CustomViews/MapViewControls/Search/MWMSearchManager.mm +++ b/iphone/Maps/Classes/CustomViews/MapViewControls/Search/MWMSearchManager.mm @@ -272,7 +272,6 @@ extern NSString * const kSearchStateKey = @"SearchStateKey"; - (void)changeToDefaultState { self.view.alpha = 1.; - GetFramework().PrepareSearch(); [self updateTopController]; [self.navigationController popToRootViewControllerAnimated:NO]; [self.parentView addSubview:self.rootView]; diff --git a/map/framework.cpp b/map/framework.cpp index cf56112e4e..649da39940 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -13,6 +13,7 @@ #include "routing/route.hpp" #include "routing/routing_algorithm.hpp" +#include "search/geometry_utils.hpp" #include "search/intermediate_result.hpp" #include "search/result.hpp" #include "search/search_engine.hpp" @@ -92,40 +93,48 @@ Framework::FixedPosition::FixedPosition() namespace { - static const int BM_TOUCH_PIXEL_INCREASE = 20; - static const int kKeepPedestrianDistanceMeters = 10000; +static const int BM_TOUCH_PIXEL_INCREASE = 20; +static const int kKeepPedestrianDistanceMeters = 10000; +char const kRouterTypeKey[] = "router"; +char const kMapStyleKey[] = "MapStyleKeyV1"; +char const kAllow3dKey[] = "Allow3d"; +char const kAllow3dBuildingsKey[] = "Buildings3d"; - char const kRouterTypeKey[] = "router"; - char const kMapStyleKey[] = "MapStyleKeyV1"; +double const kDistEqualQuery = 100.0; - char const kAllow3dKey[] = "Allow3d"; - char const kAllow3dBuildingsKey[] = "Buildings3d"; +double const kRotationAngle = math::pi4; +double const kAngleFOV = math::pi / 3.0; - double const kRotationAngle = math::pi4; - double const kAngleFOV = math::pi / 3.0; - - // TODO! - // To adjust GpsTrackFilter was added secret command "?gpstrackaccuracy:xxx;" - // where xxx is a new value for horizontal accuracy. - // This is temporary solution while we don't have a good filter. - void ParseSetGpsTrackMinAccuracyCommand(string const & query) +// TODO! +// To adjust GpsTrackFilter was added secret command "?gpstrackaccuracy:xxx;" +// where xxx is a new value for horizontal accuracy. +// This is temporary solution while we don't have a good filter. +void ParseSetGpsTrackMinAccuracyCommand(string const & query) +{ + const char kGpsAccuracy[] = "?gpstrackaccuracy:"; + if (strings::StartsWith(query, kGpsAccuracy)) { - const char kGpsAccuracy[] = "?gpstrackaccuracy:"; - if (strings::StartsWith(query, kGpsAccuracy)) + size_t const end = query.find(';', sizeof(kGpsAccuracy) - 1); + if (end != string::npos) { - size_t const end = query.find(';', sizeof(kGpsAccuracy) - 1); - if (end != string::npos) + string s(query.begin() + sizeof(kGpsAccuracy) - 1, query.begin() + end); + double value; + if (strings::to_double(s, value)) { - string s(query.begin() + sizeof(kGpsAccuracy) - 1, query.begin() + end); - double value; - if (strings::to_double(s, value)) - { - GpsTrackFilter::StoreMinHorizontalAccuracy(value); - } + GpsTrackFilter::StoreMinHorizontalAccuracy(value); } } } +} +// Cancels search query by |handle|. +void CancelQuery(weak_ptr & handle) +{ + auto queryHandle = handle.lock(); + if (queryHandle) + queryHandle->Cancel(); + handle.reset(); +} } // namespace pair Framework::RegisterMap( @@ -465,7 +474,7 @@ void Framework::UpdateLatestCountryFile(LocalCountryFile const & localFile) if (id.IsAlive()) InvalidateRect(id.GetInfo()->m_limitRect); - m_searchEngine->ClearViewportsCache(); + m_searchEngine->ClearCaches(); } void Framework::OnMapDeregistered(platform::LocalCountryFile const & localFile) @@ -498,7 +507,7 @@ void Framework::RegisterAllMaps() m_activeMaps->Init(maps); - m_searchEngine->SupportOldFormat(minFormat < version::v3); + m_searchEngine->SetSupportOldFormat(minFormat < version::v3); } void Framework::DeregisterAllMaps() @@ -821,10 +830,10 @@ void Framework::StartInteractiveSearch(search::SearchParams const & params) { using namespace search; - m_lastSearch = params; - m_lastSearch.SetForceSearch(false); - m_lastSearch.SetSearchMode(SearchParams::IN_VIEWPORT_ONLY | SearchParams::SEARCH_WORLD); - m_lastSearch.m_callback = [this](Results const & results) + m_lastISParams = params; + m_lastISParams.SetForceSearch(false); + m_lastISParams.SetSearchMode(SearchParams::IN_VIEWPORT_ONLY | SearchParams::SEARCH_WORLD); + m_lastISParams.m_callback = [this](Results const & results) { if (!results.IsEndMarker()) { @@ -841,9 +850,15 @@ void Framework::UpdateUserViewportChanged() { if (IsISActive()) { - (void)GetCurrentPosition(m_lastSearch.m_lat, m_lastSearch.m_lon); + (void)GetCurrentPosition(m_lastISParams.m_lat, m_lastISParams.m_lon); - m_searchEngine->Search(m_lastSearch, GetCurrentViewport()); + m_searchEngine->Search(m_lastISParams, GetCurrentViewport()); + + (void)GetCurrentPosition(m_lastISParams.m_lat, m_lastISParams.m_lon); + m_lastISParams.SetSearchMode(search::SearchParams::IN_VIEWPORT_ONLY); + m_lastISParams.SetForceSearch(false); + + Search(m_lastISParams); } } @@ -851,7 +866,7 @@ void Framework::ClearAllCaches() { m_model.ClearCaches(); m_infoGetter->ClearCaches(); - m_searchEngine->ClearAllCaches(); + m_searchEngine->ClearCaches(); } void Framework::SetDownloadCountryListener(TDownloadCountryListener const & listener) @@ -926,6 +941,33 @@ void Framework::UpdateCountryInfo(storage::TIndex const & countryIndex, bool isC m_drapeEngine->SetCountryInfo(countryInfo, isCurrentCountry); } +bool Framework::Search(search::SearchParams const & params) +{ +#ifdef FIXED_LOCATION + search::SearchParams rParams(params); + if (params.IsValidPosition()) + { + m_fixedPos.GetLat(rParams.m_lat); + m_fixedPos.GetLon(rParams.m_lon); + } +#else + search::SearchParams const & rParams = params; +#endif + + m2::RectD const viewport = GetCurrentViewport(); + + if (QueryCouldBeSkipped(rParams, viewport)) + return false; + + m_lastQueryParams = rParams; + m_lastQueryViewport = viewport; + + // Cancels previous search request (if any) and initiates new search request. + CancelQuery(m_lastQueryHandle); + m_lastQueryHandle = m_searchEngine->Search(m_lastQueryParams, m_lastQueryViewport); + return true; +} + void Framework::MemoryWarning() { LOG(LINFO, ("MemoryWarning")); @@ -960,6 +1002,20 @@ void Framework::EnterForeground() m_drapeEngine->SetRenderingEnabled(true); } +bool Framework::GetCurrentPosition(double & lat, double & lon) const +{ + m2::PointD pos; + MyPositionMarkPoint * myPosMark = UserMarkContainer::UserMarkForMyPostion(); + if (!myPosMark->HasPosition()) + return false; + + pos = myPosMark->GetPivot(); + + lat = MercatorBounds::YToLat(pos.y); + lon = MercatorBounds::XToLon(pos.x); + return true; +} + void Framework::InitCountryInfoGetter() { ASSERT(!m_infoGetter.get(), ("InitCountryInfoGetter() must be called only once.")); @@ -1012,40 +1068,24 @@ string Framework::GetCountryName(string const & id) const return info.m_name; } -void Framework::PrepareSearch() +bool Framework::QueryCouldBeSkipped(search::SearchParams const & params, + m2::RectD const & viewport) const { - m_searchEngine->PrepareSearch(GetCurrentViewport()); -} - -bool Framework::Search(search::SearchParams const & params) -{ -#ifdef FIXED_LOCATION - search::SearchParams rParams(params); - if (params.IsValidPosition()) - { - m_fixedPos.GetLat(rParams.m_lat); - m_fixedPos.GetLon(rParams.m_lon); - } -#else - search::SearchParams const & rParams = params; -#endif - - ParseSetGpsTrackMinAccuracyCommand(params.m_query); - - return m_searchEngine->Search(rParams, GetCurrentViewport()); -} - -bool Framework::GetCurrentPosition(double & lat, double & lon) const -{ - m2::PointD pos; - MyPositionMarkPoint * myPosMark = UserMarkContainer::UserMarkForMyPostion(); - if (!myPosMark->HasPosition()) + if (params.IsForceSearch()) return false; - - pos = myPosMark->GetPivot(); - - lat = MercatorBounds::YToLat(pos.y); - lon = MercatorBounds::XToLon(pos.x); + if (!m_lastQueryParams.IsEqualCommon(params)) + return false; + if (!m_lastQueryViewport.IsValid() || + !search::IsEqualMercator(m_lastQueryViewport, viewport, kDistEqualQuery)) + { + return false; + } + if (!m_lastQueryParams.IsSearchAroundPosition() || + ms::DistanceOnEarth(m_lastQueryParams.m_lat, m_lastQueryParams.m_lon, params.m_lat, + params.m_lon) <= kDistEqualQuery) + { + return false; + } return true; } @@ -1221,8 +1261,13 @@ void Framework::FillSearchResultsMarks(search::Results const & results) void Framework::CancelInteractiveSearch() { - m_lastSearch.Clear(); UserMarkControllerGuard(m_bmManager, UserMarkType::SEARCH_MARK).m_controller.Clear(); + if (IsISActive()) + { + m_lastISParams.Clear(); + CancelQuery(m_lastQueryHandle); + } + m_fixedSearchResults = 0; } diff --git a/map/framework.hpp b/map/framework.hpp index be87f99eb2..1cfe403152 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -42,6 +42,7 @@ #include "std/target_os.hpp" #include "std/unique_ptr.hpp" #include "std/vector.hpp" +#include "std/weak_ptr.hpp" namespace search { @@ -336,7 +337,8 @@ private: void InitCountryInfoGetter(); void InitSearchEngine(); - search::SearchParams m_lastSearch; + // Last search query params for the interactive search. + search::SearchParams m_lastISParams; uint8_t m_fixedSearchResults; bool m_connectToGpsTrack; // need to connect to tracker when Drape is being constructed @@ -350,6 +352,21 @@ private: void OnUpdateCountryIndex(storage::TIndex const & currentIndex, m2::PointF const & pt); void UpdateCountryInfo(storage::TIndex const & countryIndex, bool isCurrentCountry); + // Search query params and viewport for the latest search + // query. These fields are used to check whether a new search query + // can be skipped. Note that these fields are not guarded by a mutex + // because we're assuming that they will be accessed only from the + // UI thread. + search::SearchParams m_lastQueryParams; + m2::RectD m_lastQueryViewport; + + // A handle for the latest search query. + weak_ptr m_lastQueryHandle; + + // Returns true when |params| and |viewport| are almost the same as + // the latest search query's params and viewport. + bool QueryCouldBeSkipped(search::SearchParams const & params, m2::RectD const & viewport) const; + void OnUpdateGpsTrackPointsCallback(vector> && toAdd, pair const & toRemove); @@ -360,7 +377,6 @@ public: /// Call this function before entering search GUI. /// While it's loading, we can cache features in viewport. - void PrepareSearch(); bool Search(search::SearchParams const & params); bool GetCurrentPosition(double & lat, double & lon) const; @@ -369,7 +385,7 @@ public: size_t ShowAllSearchResults(search::Results const & results); void StartInteractiveSearch(search::SearchParams const & params); - bool IsISActive() const { return !m_lastSearch.m_query.empty(); } + bool IsISActive() const { return !m_lastISParams.m_query.empty(); } void CancelInteractiveSearch(); list const & GetLastSearchQueries() const { return m_searchQuerySaver.Get(); } diff --git a/mapshot/mapshot.pro b/mapshot/mapshot.pro index cc21f97edd..31cd6c05aa 100644 --- a/mapshot/mapshot.pro +++ b/mapshot/mapshot.pro @@ -2,7 +2,8 @@ ROOT_DIR = .. DEPENDENCIES = map drape_frontend routing search storage indexer drape platform geometry coding base \ - freetype expat fribidi tomcrypt gflags jansson protobuf osrm stats_client minizip succinct + freetype expat fribidi tomcrypt gflags jansson protobuf osrm stats_client minizip succinct \ + opening_hours include($$ROOT_DIR/common.pri) diff --git a/qt/mainwindow.cpp b/qt/mainwindow.cpp index 1a830914b8..7a17f446cf 100644 --- a/qt/mainwindow.cpp +++ b/qt/mainwindow.cpp @@ -359,15 +359,9 @@ void MainWindow::OnMyPosition() void MainWindow::OnSearchButtonClicked() { if (m_pSearchAction->isChecked()) - { - m_pDrawWidget->GetFramework().PrepareSearch(); - m_Docks[0]->show(); - } else - { m_Docks[0]->hide(); - } } void MainWindow::OnBeforeEngineCreation() diff --git a/search/intermediate_result.cpp b/search/intermediate_result.cpp index cac42bc674..ee17c0f349 100644 --- a/search/intermediate_result.cpp +++ b/search/intermediate_result.cpp @@ -193,11 +193,13 @@ Result PreResult2::GenerateFinalResult(storage::CountryInfoGetter const & infoGe } } -Result PreResult2::GeneratePointResult(CategoriesHolder const * pCat, set const * pTypes, - int8_t locale) const +Result PreResult2::GeneratePointResult(storage::CountryInfoGetter const & infoGetter, + CategoriesHolder const * pCat, + set const * pTypes, int8_t locale) const { uint32_t const type = GetBestType(pTypes); - return Result(m_id, GetCenter(), m_str, ReadableFeatureType(pCat, type, locale)); + return Result(m_id, GetCenter(), m_str, GetRegionName(infoGetter, type), + ReadableFeatureType(pCat, type, locale)); } // static diff --git a/search/intermediate_result.hpp b/search/intermediate_result.hpp index a8c0908ee8..001c394925 100644 --- a/search/intermediate_result.hpp +++ b/search/intermediate_result.hpp @@ -75,8 +75,9 @@ public: CategoriesHolder const * pCat, set const * pTypes, int8_t locale) const; - Result GeneratePointResult(CategoriesHolder const * pCat, set const * pTypes, - int8_t locale) const; + Result GeneratePointResult(storage::CountryInfoGetter const & infoGetter, + CategoriesHolder const * pCat, + set const * pTypes, int8_t locale) const; static bool LessRank(PreResult2 const & r1, PreResult2 const & r2); static bool LessDistance(PreResult2 const & r1, PreResult2 const & r2); diff --git a/search/result.cpp b/search/result.cpp index ac4197979c..ba388711bd 100644 --- a/search/result.cpp +++ b/search/result.cpp @@ -13,26 +13,30 @@ Result::Result(FeatureID const & id, m2::PointD const & pt, string const & str, , m_region(region) , m_type(type) , m_featureType(featureType) - , m_positionInResults(-1) , m_metadata(meta) { Init(true /* metadataInitialized */); } -Result::Result(FeatureID const & id, m2::PointD const & pt, string const & str, string const & type) - : m_id(id), m_center(pt), m_str(str), m_type(type), m_positionInResults(-1) +Result::Result(FeatureID const & id, m2::PointD const & pt, string const & str, + string const & region, string const & type) + : m_id(id) + , m_center(pt) + , m_str(str) + , m_region(region) + , m_type(type) { Init(false /* metadataInitialized */); } Result::Result(m2::PointD const & pt, string const & str, string const & region, string const & type) - : m_center(pt), m_str(str), m_region(region), m_type(type), m_positionInResults(-1) + : m_center(pt), m_str(str), m_region(region), m_type(type) { } Result::Result(string const & str, string const & suggest) - : m_str(str), m_suggestionStr(suggest), m_positionInResults(-1) + : m_str(str), m_suggestionStr(suggest) { } @@ -45,7 +49,6 @@ Result::Result(Result const & res, string const & suggest) , m_featureType(res.m_featureType) , m_suggestionStr(suggest) , m_hightlightRanges(res.m_hightlightRanges) - , m_positionInResults(-1) { } diff --git a/search/result.hpp b/search/result.hpp index a6ec04a73f..6ca29d9ce6 100644 --- a/search/result.hpp +++ b/search/result.hpp @@ -41,7 +41,8 @@ public: string const & type, uint32_t featureType, Metadata const & meta); /// Used for generation viewport results. - Result(FeatureID const & id, m2::PointD const & pt, string const & str, string const & type); + Result(FeatureID const & id, m2::PointD const & pt, string const & str, + string const & region, string const & type); /// @param[in] type Empty string - RESULT_LATLON, building address otherwise. Result(m2::PointD const & pt, string const & str, diff --git a/search/search_engine.cpp b/search/search_engine.cpp index cc2ca72ac0..8978ec935c 100644 --- a/search/search_engine.cpp +++ b/search/search_engine.cpp @@ -1,48 +1,33 @@ #include "geometry_utils.hpp" #include "search_engine.hpp" #include "search_query.hpp" -#include "suggest.hpp" #include "storage/country_info_getter.hpp" #include "indexer/categories_holder.hpp" -#include "indexer/search_string_utils.hpp" -#include "indexer/scales.hpp" #include "indexer/classificator.hpp" +#include "indexer/scales.hpp" +#include "indexer/search_string_utils.hpp" #include "platform/platform.hpp" #include "geometry/distance_on_sphere.hpp" #include "geometry/mercator.hpp" +#include "base/scope_guard.hpp" #include "base/stl_add.hpp" -#include "std/algorithm.hpp" #include "std/bind.hpp" #include "std/map.hpp" #include "std/vector.hpp" #include "3party/Alohalytics/src/alohalytics.h" - namespace search { - -double const DIST_EQUAL_QUERY = 100.0; - -using TSuggestsContainer = vector; - -class EngineData -{ -public: - EngineData(Reader * categoriesR) : m_categories(categoriesR) {} - - CategoriesHolder m_categories; - TSuggestsContainer m_suggests; -}; - namespace { +int const kResultsCount = 30; class InitSuggestions { @@ -50,7 +35,7 @@ class InitSuggestions TSuggestMap m_suggests; public: - void operator() (CategoriesHolder::Category::Name const & name) + void operator()(CategoriesHolder::Category::Name const & name) { if (name.m_prefixLengthToSuggest != CategoriesHolder::Category::EMPTY_PREFIX_LENGTH) { @@ -62,11 +47,11 @@ public: } } - void GetSuggests(TSuggestsContainer & cont) const + void GetSuggests(vector & suggests) const { - cont.reserve(m_suggests.size()); - for (TSuggestMap::const_iterator i = m_suggests.begin(); i != m_suggests.end(); ++i) - cont.push_back(Suggest(i->first.first, i->second, i->first.second)); + suggests.reserve(suggests.size() + m_suggests.size()); + for (auto const & s : m_suggests) + suggests.emplace_back(s.first.first, s.second, s.first.second); } }; @@ -100,89 +85,85 @@ void SendStatistics(SearchParams const & params, m2::RectD const & viewport, Res } } // namespace +QueryHandle::QueryHandle() : m_query(nullptr), m_cancelled(false) {} + +void QueryHandle::Cancel() +{ + lock_guard lock(m_mu); + m_cancelled = true; + if (m_query) + m_query->Cancel(); +} + +void QueryHandle::Attach(Query & query) +{ + lock_guard lock(m_mu); + m_query = &query; + if (m_cancelled) + m_query->Cancel(); +} + +void QueryHandle::Detach() +{ + lock_guard lock(m_mu); + m_query = nullptr; +} + Engine::Engine(Index & index, Reader * categoriesR, storage::CountryInfoGetter const & infoGetter, string const & locale, unique_ptr && factory) - : m_factory(move(factory)) - , m_data(make_unique(categoriesR)) + : m_categories(categoriesR), m_factory(move(factory)), m_shutdown(false) { - m_isReadyThread.clear(); - InitSuggestions doInit; - m_data->m_categories.ForEachName(bind(ref(doInit), _1)); - doInit.GetSuggests(m_data->m_suggests); + m_categories.ForEachName(bind(ref(doInit), _1)); + doInit.GetSuggests(m_suggests); - m_query = - m_factory->BuildSearchQuery(index, m_data->m_categories, m_data->m_suggests, infoGetter); + m_query = m_factory->BuildSearchQuery(index, m_categories, m_suggests, infoGetter); m_query->SetPreferredLocale(locale); + + m_loop = threads::SimpleThread(&Engine::MainLoop, this); } Engine::~Engine() { -} - -void Engine::SupportOldFormat(bool b) -{ - m_query->SupportOldFormat(b); -} - -void Engine::PrepareSearch(m2::RectD const & viewport) -{ - // bind does copy of all rects - GetPlatform().RunAsync(bind(&Engine::SetViewportAsync, this, viewport)); -} - -bool Engine::Search(SearchParams const & params, m2::RectD const & viewport) -{ - // Check for equal query. - // There is no need to synchronize here for reading m_params, - // because this function is always called from main thread (one-by-one for queries). - - if (!params.IsForceSearch() && - m_params.IsEqualCommon(params) && - (m_viewport.IsValid() && IsEqualMercator(m_viewport, viewport, DIST_EQUAL_QUERY))) { - if (m_params.IsSearchAroundPosition() && - ms::DistanceOnEarth(m_params.m_lat, m_params.m_lon, params.m_lat, params.m_lon) > DIST_EQUAL_QUERY) - { - // Go forward only if we search around position and it's changed significantly. - } - else - { - // Skip this query in all other cases. - return false; - } + lock_guard lock(m_mu); + m_shutdown = true; + m_cv.notify_one(); + } + m_loop.join(); +} + +weak_ptr Engine::Search(SearchParams const & params, m2::RectD const & viewport) +{ + shared_ptr handle(new QueryHandle()); + PostTask(bind(&Engine::SearchTask, this, params, viewport, handle)); + return handle; +} + +void Engine::SetSupportOldFormat(bool support) +{ + PostTask(bind(&Engine::SupportOldFormatTask, this, support)); +} + +void Engine::ClearCaches() { PostTask(bind(&Engine::ClearCachesTask, this)); } + +bool Engine::GetNameByType(uint32_t type, int8_t locale, string & name) const +{ + uint8_t level = ftype::GetLevel(type); + ASSERT_GREATER(level, 0, ()); + + while (true) + { + if (m_categories.GetNameByType(type, locale, name)) + return true; + + if (--level == 0) + break; + + ftype::TruncValue(type, level); } - { - // Assign new search params. - // Put the synch here, because this params are reading in search threads. - threads::MutexGuard guard(m_updateMutex); - m_params = params; - m_viewport = viewport; - } - - // Run task. - GetPlatform().RunAsync(bind(&Engine::SearchAsync, this)); - - return true; -} - -void Engine::SetViewportAsync(m2::RectD const & viewport) -{ - // First of all - cancel previous query. - m_query->Cancel(); - - // Enter to run new search. - threads::MutexGuard searchGuard(m_searchMutex); - - m2::RectD r(viewport); - (void)GetInflatedViewport(r); - m_query->SetViewport(r, true); -} - -void Engine::EmitResults(SearchParams const & params, Results const & res) -{ - params.m_callback(res); + return false; } void Engine::SetRankPivot(SearchParams const & params, @@ -201,41 +182,58 @@ void Engine::SetRankPivot(SearchParams const & params, m_query->SetRankPivot(viewport.Center()); } -void Engine::SearchAsync() +void Engine::EmitResults(SearchParams const & params, Results const & res) { - if (m_isReadyThread.test_and_set()) - return; - - // First of all - cancel previous query. - m_query->Cancel(); - - // Enter to run new search. - threads::MutexGuard searchGuard(m_searchMutex); - - m_isReadyThread.clear(); - - // Get current search params. - SearchParams params; - m2::RectD viewport; + params.m_callback(res); +} +void Engine::MainLoop() +{ + while (true) { - threads::MutexGuard updateGuard(m_updateMutex); - params = m_params; + unique_lock lock(m_mu); + m_cv.wait(lock, [this]() + { + return m_shutdown || !m_tasks.empty(); + }); - if (!params.GetSearchRect(viewport)) - viewport = m_viewport; + if (m_shutdown) + break; + + function task(move(m_tasks.front())); + m_tasks.pop(); + lock.unlock(); + + task(); } +} +void Engine::PostTask(function && task) +{ + lock_guard lock(m_mu); + m_tasks.push(std::move(task)); + m_cv.notify_one(); +} + +void Engine::SearchTask(SearchParams const & params, m2::RectD const & viewport, + shared_ptr handle) +{ bool const viewportSearch = params.HasSearchMode(SearchParams::IN_VIEWPORT_ONLY); // Initialize query. m_query->Init(viewportSearch); + handle->Attach(*m_query); + MY_SCOPE_GUARD(detach, [&handle] { handle->Detach(); }); + + // Early exit when query is cancelled. + if (m_query->IsCancelled()) + { + params.m_callback(Results::GetEndMarker(true /* isCancelled */)); + return; + } SetRankPivot(params, viewport, viewportSearch); - m_query->SetSearchInWorld(params.HasSearchMode(SearchParams::SEARCH_WORLD)); - - // Language validity is checked inside m_query->SetInputLocale(params.m_inputLocale); ASSERT(!params.m_query.empty(), ()); @@ -243,16 +241,10 @@ void Engine::SearchAsync() Results res; - // Call m_query->IsCancelled() everywhere it needed without storing - // return value. This flag can be changed from another thread. - m_query->SearchCoordinates(params.m_query, res); try { - // Do search for address in all modes. - // params.HasSearchMode(SearchParams::SEARCH_ADDRESS) - if (viewportSearch) { m_query->SetViewport(viewport, true /* forceUpdate */); @@ -261,7 +253,7 @@ void Engine::SearchAsync() else { m_query->SetViewport(viewport, params.IsSearchAroundPosition() /* forceUpdate */); - m_query->Search(res, RESULTS_COUNT); + m_query->Search(res, kResultsCount); } if (res.GetCount() > 0) @@ -278,44 +270,7 @@ void Engine::SearchAsync() params.m_callback(Results::GetEndMarker(m_query->IsCancelled())); } -bool Engine::GetNameByType(uint32_t type, int8_t locale, string & name) const -{ - uint8_t level = ftype::GetLevel(type); - ASSERT_GREATER(level, 0, ()); - - while (true) - { - if (m_data->m_categories.GetNameByType(type, locale, name)) - return true; - - if (--level == 0) - break; - - ftype::TruncValue(type, level); - } - - return false; -} - -void Engine::ClearViewportsCache() -{ - threads::MutexGuard guard(m_searchMutex); - - m_query->ClearCaches(); -} - -void Engine::ClearAllCaches() -{ - //threads::MutexGuard guard(m_searchMutex); - - // Trying to lock mutex, because this function calls on memory warning notification. - // So that allows to prevent lock of UI while search query wouldn't be processed. - if (m_searchMutex.TryLock()) - { - m_query->ClearCaches(); - - m_searchMutex.Unlock(); - } -} +void Engine::SupportOldFormatTask(bool support) { m_query->SupportOldFormat(support); } +void Engine::ClearCachesTask() { m_query->ClearCaches(); } } // namespace search diff --git a/search/search_engine.hpp b/search/search_engine.hpp index 394aa7cc0b..1fc001ad18 100644 --- a/search/search_engine.hpp +++ b/search/search_engine.hpp @@ -3,17 +3,26 @@ #include "params.hpp" #include "result.hpp" #include "search_query_factory.hpp" +#include "suggest.hpp" #include "geometry/rect2d.hpp" +#include "indexer/categories_holder.hpp" + #include "coding/reader.hpp" +#include "base/macros.hpp" #include "base/mutex.hpp" +#include "base/thread.hpp" #include "std/atomic.hpp" +#include "std/condition_variable.hpp" #include "std/function.hpp" +#include "std/mutex.hpp" +#include "std/queue.hpp" #include "std/string.hpp" #include "std/unique_ptr.hpp" +#include "std/weak_ptr.hpp" class Index; @@ -27,48 +36,92 @@ namespace search class EngineData; class Query; +// This class is used as a reference to a search query in the +// SearchEngine's queue. It's only possible to cancel a search +// request via this reference. +// +// NOTE: this class is thread-safe. +class QueryHandle +{ +public: + QueryHandle(); + + // Cancels query this handle points to. + void Cancel(); + +private: + friend class Engine; + + // Attaches the handle to a |query|. If there was or will be a + // cancel signal, this signal will be propagated to |query|. This + // method is called only once, when search engine starts to process + // query this handle corresponds to. + void Attach(Query & query); + + // Detaches handle from a query. This method is called only once, + // when search engine completes process of a query this handle + // corresponds to. + void Detach(); + + Query * m_query; + bool m_cancelled; + mutex m_mu; + + DISALLOW_COPY_AND_MOVE(QueryHandle); +}; + +// This class is a wrapper around thread processing search queries one +// by one. +// +// NOTE: this class is thread safe. class Engine { - typedef function SearchCallbackT; - public: // Doesn't take ownership of index. Takes ownership of pCategories Engine(Index & index, Reader * categoriesR, storage::CountryInfoGetter const & infoGetter, string const & locale, unique_ptr && factory); ~Engine(); - void SupportOldFormat(bool b); + // Posts search request to the queue and returns it's handle. + weak_ptr Search(SearchParams const & params, m2::RectD const & viewport); - void PrepareSearch(m2::RectD const & viewport); - bool Search(SearchParams const & params, m2::RectD const & viewport); + // Posts request to support old format to the queue. + void SetSupportOldFormat(bool support); - int8_t GetCurrentLanguage() const; + // Posts request to clear caches to the queue. + void ClearCaches(); bool GetNameByType(uint32_t type, int8_t lang, string & name) const; - void ClearViewportsCache(); - void ClearAllCaches(); - private: - static const int RESULTS_COUNT = 30; + // *ALL* following methods are executed on a m_loop thread. - void SetRankPivot(SearchParams const & params, - m2::RectD const & viewport, bool viewportSearch); - void SetViewportAsync(m2::RectD const & viewport); - void SearchAsync(); + void SetRankPivot(SearchParams const & params, m2::RectD const & viewport, bool viewportSearch); void EmitResults(SearchParams const & params, Results const & res); - threads::Mutex m_searchMutex; - threads::Mutex m_updateMutex; - atomic_flag m_isReadyThread; + // This method executes tasks from |m_tasks| in a FIFO manner. + void MainLoop(); - SearchParams m_params; - m2::RectD m_viewport; + void PostTask(function && task); + + void SearchTask(SearchParams const & params, m2::RectD const & viewport, + shared_ptr handle); + + void SupportOldFormatTask(bool support); + + void ClearCachesTask(); + + CategoriesHolder m_categories; + vector m_suggests; unique_ptr m_query; unique_ptr m_factory; - unique_ptr const m_data; -}; + bool m_shutdown; + mutex m_mu; + condition_variable m_cv; + queue> m_tasks; + threads::SimpleThread m_loop; +}; } // namespace search diff --git a/search/search_integration_tests/retrieval_test.cpp b/search/search_integration_tests/retrieval_test.cpp index 0d5985a347..dc1b011bd5 100644 --- a/search/search_integration_tests/retrieval_test.cpp +++ b/search/search_integration_tests/retrieval_test.cpp @@ -454,7 +454,9 @@ UNIT_TEST(Retrieval_CafeMTV) TestSearchRequest request(engine, "Moscow ", "en", search::SearchParams::ALL, moscowViewport); request.Wait(); - vector> rules = {make_shared(testWorldId, mskCity), + initializer_list> mskCityAlts = { + make_shared(testWorldId, mskCity), make_shared(mskId, mskCity)}; + vector> rules = {make_shared(mskCityAlts), make_shared(mtvId, mskCafe)}; MatchResults(engine, rules, request.Results()); } @@ -462,7 +464,10 @@ UNIT_TEST(Retrieval_CafeMTV) { TestSearchRequest request(engine, "MTV ", "en", search::SearchParams::ALL, mtvViewport); request.Wait(); - vector> rules = {make_shared(testWorldId, mtvCity), + + initializer_list> mtvCityAlts = { + make_shared(testWorldId, mtvCity), make_shared(mtvId, mtvCity)}; + vector> rules = {make_shared(mtvCityAlts), make_shared(mskId, mtvCafe)}; MatchResults(engine, rules, request.Results()); } diff --git a/search/search_query.cpp b/search/search_query.cpp index 59d4eb1a6b..462a7b05d7 100644 --- a/search/search_query.cpp +++ b/search/search_query.cpp @@ -563,8 +563,9 @@ void Query::SearchViewportPoints(Results & res) { if (IsCancelled()) break; - res.AddResultNoChecks( - (*(indV[i])).GeneratePointResult(&m_categories, &m_prefferedTypes, m_currentLocaleCode)); + res.AddResultNoChecks((*(indV[i])) + .GeneratePointResult(m_infoGetter, &m_categories, &m_prefferedTypes, + m_currentLocaleCode)); } } @@ -1557,7 +1558,6 @@ public: void Query::SearchLocality(MwmValue const * pMwm, Locality & res1, Region & res2) { - LOG(LDEBUG, ("Query::SearchLocality")); SearchQueryParams params; InitParams(true /* localitySearch */, params); diff --git a/search/search_query.hpp b/search/search_query.hpp index 9eb28aaba1..3d28c68812 100644 --- a/search/search_query.hpp +++ b/search/search_query.hpp @@ -1,8 +1,8 @@ #pragma once -#include "intermediate_result.hpp" -#include "keyword_lang_matcher.hpp" -#include "retrieval.hpp" -#include "suggest.hpp" +#include "search/intermediate_result.hpp" +#include "search/keyword_lang_matcher.hpp" +#include "search/retrieval.hpp" +#include "search/suggest.hpp" #include "indexer/ftypes_matcher.hpp" #include "indexer/index.hpp" @@ -60,9 +60,6 @@ public: Query(Index & index, CategoriesHolder const & categories, vector const & suggests, storage::CountryInfoGetter const & infoGetter); - // TODO (@gorshenin): current cancellation logic is completely wrong - // and broken by design. Fix it ASAP. - // // my::Cancellable overrides: void Reset() override; void Cancel() override; diff --git a/search/search_tests_support/test_search_engine.cpp b/search/search_tests_support/test_search_engine.cpp index 2a8becc550..aba4f74b4a 100644 --- a/search/search_tests_support/test_search_engine.cpp +++ b/search/search_tests_support/test_search_engine.cpp @@ -55,7 +55,8 @@ TestSearchEngine::TestSearchEngine(string const & locale) { } -bool TestSearchEngine::Search(search::SearchParams const & params, m2::RectD const & viewport) +weak_ptr TestSearchEngine::Search(search::SearchParams const & params, + m2::RectD const & viewport) { return m_engine.Search(params, viewport); } diff --git a/search/search_tests_support/test_search_engine.hpp b/search/search_tests_support/test_search_engine.hpp index c7e3546bf2..2ae9c0a1c0 100644 --- a/search/search_tests_support/test_search_engine.hpp +++ b/search/search_tests_support/test_search_engine.hpp @@ -9,6 +9,7 @@ #include "storage/country_info_getter.hpp" #include "std/string.hpp" +#include "std/weak_ptr.hpp" class Platform; @@ -23,7 +24,8 @@ class TestSearchEngine : public Index public: TestSearchEngine(string const & locale); - bool Search(search::SearchParams const & params, m2::RectD const & viewport); + weak_ptr Search(search::SearchParams const & params, + m2::RectD const & viewport); private: Platform & m_platform; diff --git a/search/search_tests_support/test_search_request.cpp b/search/search_tests_support/test_search_request.cpp index a96fda0cc8..89adf84fec 100644 --- a/search/search_tests_support/test_search_request.cpp +++ b/search/search_tests_support/test_search_request.cpp @@ -22,7 +22,7 @@ TestSearchRequest::TestSearchRequest(TestSearchEngine & engine, string const & q Done(results); }; params.SetSearchMode(mode); - CHECK(engine.Search(params, viewport), ("Can't run search.")); + engine.Search(params, viewport); } void TestSearchRequest::Wait()