diff --git a/map/everywhere_search_params.hpp b/map/everywhere_search_params.hpp index f0bf43d217..4433b10745 100644 --- a/map/everywhere_search_params.hpp +++ b/map/everywhere_search_params.hpp @@ -6,22 +6,26 @@ #include "search/hotels_filter.hpp" #include "search/result.hpp" +#include #include #include #include #include +#include + namespace search { struct EverywhereSearchParams { + using OnResults = + std::function const & productInfo)>; + std::string m_query; std::string m_inputLocale; std::shared_ptr m_hotelsFilter; booking::filter::Tasks m_bookingFilterTasks; - - using OnResults = - std::function const & productInfo)>; + boost::optional m_timeout; OnResults m_onResults; }; diff --git a/map/map_tests/search_api_tests.cpp b/map/map_tests/search_api_tests.cpp index ed1fb233f0..0aafeec9f2 100644 --- a/map/map_tests/search_api_tests.cpp +++ b/map/map_tests/search_api_tests.cpp @@ -12,6 +12,7 @@ #include "map/bookmarks_search_params.hpp" #include "map/search_api.hpp" +#include "map/search_product_info.hpp" #include "map/viewport_search_params.hpp" #include "storage/country_info_getter.hpp" @@ -130,6 +131,63 @@ UNIT_CLASS_TEST(SearchAPITest, MultipleViewportsRequests) future1.wait(); } +UNIT_CLASS_TEST(SearchAPITest, Cancellation) +{ + TestCafe cafe(m2::PointD(0, 0), "cafe", "en"); + + auto const id = BuildCountry("Wonderland", [&](TestMwmBuilder & builder) { builder.Add(cafe); }); + + EverywhereSearchParams params; + params.m_query = "cafe "; + params.m_inputLocale = "en"; + + { + promise promise; + auto future = promise.get_future(); + + params.m_onResults = [&](Results const & results, vector const &) { + TEST(!results.IsEndedCancelled(), ()); + + if (!results.IsEndMarker()) + return; + + Rules const rules = {ExactMatch(id, cafe)}; + TEST(MatchResults(m_dataSource, rules, results), ()); + + promise.set_value(); + }; + + m_api.OnViewportChanged(m2::RectD(0.0, 0.0, 1.0, 1.0)); + m_api.SearchEverywhere(params); + future.wait(); + } + + { + promise promise; + auto future = promise.get_future(); + + params.m_timeout = chrono::seconds(-1); + + params.m_onResults = [&](Results const & results, vector const &) { + // The deadline has fired but Search API does not expose it. + TEST(!results.IsEndedCancelled(), ()); + + if (!results.IsEndMarker()) + return; + + Rules const rules = {ExactMatch(id, cafe)}; + TEST(MatchResults(m_dataSource, rules, results), ()); + + promise.set_value(); + }; + + // Force the search by changing the viewport. + m_api.OnViewportChanged(m2::RectD(0.0, 0.0, 2.0, 2.0)); + m_api.SearchEverywhere(params); + future.wait(); + } +} + UNIT_CLASS_TEST(SearchAPITest, BookmarksSearch) { vector marks; diff --git a/map/search_api.cpp b/map/search_api.cpp index 99e77d6759..5364efeb37 100644 --- a/map/search_api.cpp +++ b/map/search_api.cpp @@ -190,6 +190,8 @@ bool SearchAPI::SearchEverywhere(EverywhereSearchParams const & params) p.m_needAddress = true; p.m_needHighlighting = true; p.m_hotelsFilter = params.m_hotelsFilter; + if (params.m_timeout) + p.m_timeout = *params.m_timeout; p.m_onResults = EverywhereSearchCallback( static_cast(*this), diff --git a/search/features_layer_matcher.hpp b/search/features_layer_matcher.hpp index 86e0f0f217..f6f907dfcf 100644 --- a/search/features_layer_matcher.hpp +++ b/search/features_layer_matcher.hpp @@ -108,6 +108,8 @@ public: void OnQueryFinished(); private: + void BailIfCancelled() { ::search::BailIfCancelled(m_cancellable); } + template void MatchPOIsWithBuildings(FeaturesLayer const & child, FeaturesLayer const & parent, Fn && fn) { @@ -121,7 +123,7 @@ private: auto const & pois = *child.m_sortedFeatures; auto const & buildings = *parent.m_sortedFeatures; - BailIfCancelled(m_cancellable); + BailIfCancelled(); std::vector poiCenters; poiCenters.reserve(pois.size()); @@ -139,6 +141,8 @@ private: buildingRects.reserve(buildings.size()); for (size_t i = 0; i < buildings.size(); ++i) { + BailIfCancelled(); + auto buildingFt = GetByIndex(buildings[i]); if (!buildingFt) continue; @@ -177,10 +181,13 @@ private: for (size_t i = 0; i < pois.size(); ++i) { + BailIfCancelled(); + m_context->ForEachFeature( - mercator::RectByCenterXYAndSizeInMeters(poiCenters[i].m_point, - kBuildingRadiusMeters), + mercator::RectByCenterXYAndSizeInMeters(poiCenters[i].m_point, kBuildingRadiusMeters), [&](FeatureType & ft) { + BailIfCancelled(); + if (m_postcodes && !m_postcodes->HasBit(ft.GetID().m_index) && !m_postcodes->HasBit(GetMatchingStreet(ft))) { @@ -201,7 +208,7 @@ private: template void MatchPOIsWithStreets(FeaturesLayer const & child, FeaturesLayer const & parent, Fn && fn) { - BailIfCancelled(m_cancellable); + BailIfCancelled(); ASSERT_EQUAL(child.m_type, Model::TYPE_POI, ()); ASSERT_EQUAL(parent.m_type, Model::TYPE_STREET, ()); @@ -259,7 +266,7 @@ private: streetProjectors.emplace_back(streetPoints); } - BailIfCancelled(m_cancellable); + BailIfCancelled(); PointRectMatcher::Match(poiCenters, streetRects, PointRectMatcher::RequestType::All, [&](size_t poiId, size_t streetId) { ASSERT_LESS(poiId, pois.size(), ()); @@ -292,6 +299,8 @@ private: { for (uint32_t const houseId : buildings) { + BailIfCancelled(); + uint32_t const streetId = GetMatchingStreet(houseId); if (std::binary_search(streets.begin(), streets.end(), streetId)) fn(houseId, streetId); @@ -307,7 +316,7 @@ private: std::unique_ptr & feature, bool & loaded) -> bool { ++numFilterInvocations; if ((numFilterInvocations & 0xFF) == 0) - BailIfCancelled(m_cancellable); + BailIfCancelled(); if (std::binary_search(buildings.begin(), buildings.end(), houseId)) return true; @@ -346,7 +355,8 @@ private: ProjectionOnStreet proj; for (uint32_t streetId : streets) { - BailIfCancelled(m_cancellable); + BailIfCancelled(); + StreetVicinityLoader::Street const & street = m_loader.GetStreet(streetId); if (street.IsEmpty()) continue; diff --git a/search/features_layer_path_finder.cpp b/search/features_layer_path_finder.cpp index 8243c647ff..11f1639dd3 100644 --- a/search/features_layer_path_finder.cpp +++ b/search/features_layer_path_finder.cpp @@ -1,6 +1,5 @@ #include "search/features_layer_path_finder.hpp" -#include "search/cancel_exception.hpp" #include "search/features_layer_matcher.hpp" #include "search/house_numbers_matcher.hpp" @@ -133,7 +132,7 @@ void FeaturesLayerPathFinder::FindReachableVerticesTopDown( for (size_t i = layers.size() - 1; i != 0; --i) { - BailIfCancelled(m_cancellable); + BailIfCancelled(); parentGraph.emplace_back(); FeaturesLayer parent(*layers[i]); @@ -197,7 +196,7 @@ void FeaturesLayerPathFinder::FindReachableVerticesBottomUp( for (size_t i = 0; i + 1 != layers.size(); ++i) { - BailIfCancelled(m_cancellable); + BailIfCancelled(); parentGraph.emplace_front(); FeaturesLayer child(*layers[i]); diff --git a/search/features_layer_path_finder.hpp b/search/features_layer_path_finder.hpp index f78a78ba5e..646952c4f7 100644 --- a/search/features_layer_path_finder.hpp +++ b/search/features_layer_path_finder.hpp @@ -1,5 +1,6 @@ #pragma once +#include "search/cancel_exception.hpp" #include "search/features_layer.hpp" #include "search/intersection_result.hpp" @@ -77,6 +78,8 @@ public: static void SetModeForTesting(Mode mode) { m_mode = mode; } private: + void BailIfCancelled() { ::search::BailIfCancelled(m_cancellable); } + void FindReachableVertices(FeaturesLayerMatcher & matcher, std::vector const & layers, std::vector & results); diff --git a/search/geocoder.cpp b/search/geocoder.cpp index 7579a4ad92..4e09844aca 100644 --- a/search/geocoder.cpp +++ b/search/geocoder.cpp @@ -578,7 +578,6 @@ void Geocoder::GoImpl(vector> & infos, bool inViewport) // Iterates through all alive mwms and performs geocoding. ForEachCountry(infos, processCountry); - m_preRanker.UpdateResults(true /* lastUpdate */); } void Geocoder::InitBaseContext(BaseContext & ctx) diff --git a/search/geocoder.hpp b/search/geocoder.hpp index a4f7ec68ef..a566c4bed1 100644 --- a/search/geocoder.hpp +++ b/search/geocoder.hpp @@ -187,7 +187,7 @@ private: void ForEachCountry(std::vector> const & infos, Fn && fn); // Throws CancelException if cancelled. - inline void BailIfCancelled() { ::search::BailIfCancelled(m_cancellable); } + void BailIfCancelled() { ::search::BailIfCancelled(m_cancellable); } // A fast-path branch for categorial requests. void MatchCategories(BaseContext & ctx, bool aroundPivot); diff --git a/search/pre_ranker.cpp b/search/pre_ranker.cpp index fd2951c3b7..6bf133371b 100644 --- a/search/pre_ranker.cpp +++ b/search/pre_ranker.cpp @@ -240,7 +240,7 @@ void PreRanker::UpdateResults(bool lastUpdate) FillMissingFieldsInPreResults(); Filter(m_params.m_viewportSearch); m_numSentResults += m_results.size(); - m_ranker.SetPreRankerResults(move(m_results)); + m_ranker.AddPreRankerResults(move(m_results)); m_results.clear(); m_ranker.UpdateResults(lastUpdate); diff --git a/search/pre_ranker.hpp b/search/pre_ranker.hpp index fb7c022714..a76d696818 100644 --- a/search/pre_ranker.hpp +++ b/search/pre_ranker.hpp @@ -65,7 +65,10 @@ public: void Emplace(Args &&... args) { if (m_numSentResults >= Limit()) + { + // todo(@m) Geocoder can be safely cancelled here, can't it? return; + } m_results.emplace_back(std::forward(args)...); } @@ -78,10 +81,10 @@ public: // Use |lastUpdate| to indicate that no more results will be added. void UpdateResults(bool lastUpdate); - inline size_t Size() const { return m_results.size(); } - inline size_t BatchSize() const { return m_params.m_batchSize; } - inline size_t NumSentResults() const { return m_numSentResults; } - inline size_t Limit() const { return m_params.m_limit; } + size_t Size() const { return m_results.size(); } + size_t BatchSize() const { return m_params.m_batchSize; } + size_t NumSentResults() const { return m_numSentResults; } + size_t Limit() const { return m_params.m_limit; } template void ForEach(Fn && fn) diff --git a/search/processor.cpp b/search/processor.cpp index 8931fa4742..d42c739434 100644 --- a/search/processor.cpp +++ b/search/processor.cpp @@ -1,4 +1,4 @@ -#include "processor.hpp" +#include "search/processor.hpp" #include "search/common.hpp" #include "search/cuisine_filter.hpp" @@ -395,6 +395,29 @@ void Processor::OnBookmarksDetachedFromGroup(bookmarks::GroupId const & groupId, m_bookmarksProcessor.DetachFromGroup(id, groupId); } +void Processor::Reset() +{ + base::Cancellable::Reset(); + m_lastUpdate = false; +} + +bool Processor::IsCancelled() const +{ + if (m_lastUpdate) + return false; + + bool const ret = base::Cancellable::IsCancelled(); + bool const byDeadline = CancellationStatus() == base::Cancellable::Status::DeadlineExceeded; + + // todo(@m) This is a "soft deadline": we ignore it if nothing has been + // found so far. We could also implement a "hard deadline" + // that would be impossible to ignore. + if (byDeadline && m_preRanker.Size() == 0 && m_preRanker.NumSentResults() == 0) + return false; + + return ret; +} + Locales Processor::GetCategoryLocales() const { static int8_t const enLocaleCode = CategoriesHolder::MapLocaleToInteger("en"); @@ -425,10 +448,15 @@ void Processor::ForEachCategoryTypeFuzzy(StringSliceBase const & slice, ToDo && void Processor::Search(SearchParams const & params) { + SetDeadline(chrono::steady_clock::now() + params.m_timeout); + + InitEmitter(params); + if (params.m_onStarted) params.m_onStarted(); - if (IsCancelled()) + // IsCancelled is not enough here because it depends on PreRanker being initialized. + if (IsCancelled() && CancellationStatus() == base::Cancellable::Status::CancelCalled) { Results results; results.SetEndMarker(true /* isCancelled */); @@ -452,7 +480,11 @@ void Processor::Search(SearchParams const & params) SetQuery(params.m_query); SetViewport(viewport); - InitEmitter(params); + // Used to store the earliest available cancellation status: + // if the search has been cancelled, we need to pinpoint the reason + // for cancellation and a further call to CancellationStatus() may + // return a different result. + auto cancellationStatus = base::Cancellable::Status::Active; switch (params.m_mode) { @@ -483,18 +515,28 @@ void Processor::Search(SearchParams const & params) } catch (CancelException const &) { - LOG(LDEBUG, ("Search has been cancelled.")); + LOG(LDEBUG, ("Search has been cancelled. Reason:", CancellationStatus())); + } + + cancellationStatus = CancellationStatus(); + if (cancellationStatus != base::Cancellable::Status::CancelCalled) + { + m_lastUpdate = true; + // Cancellable is effectively disabled now, so + // this call must not result in a CancelException. + m_preRanker.UpdateResults(true /* lastUpdate */); } // Emit finish marker to client. - m_geocoder.Finish(IsCancelled()); + m_geocoder.Finish(cancellationStatus == Cancellable::Status::CancelCalled); break; } case Mode::Bookmarks: SearchBookmarks(params.m_bookmarksGroupId); break; case Mode::Count: ASSERT(false, ("Invalid mode")); break; } - if (!viewportSearch && !IsCancelled()) + // todo(@m) Send the fact of cancelling by timeout to stats? + if (!viewportSearch && cancellationStatus != Cancellable::Status::CancelCalled) SendStatistics(params, viewport, m_emitter.GetResults()); } diff --git a/search/processor.hpp b/search/processor.hpp index 7b01e554a2..407eeab43e 100644 --- a/search/processor.hpp +++ b/search/processor.hpp @@ -85,10 +85,8 @@ public: void InitParams(QueryParams & params) const; - void InitGeocoder(Geocoder::Params &geocoderParams, - SearchParams const &searchParams); - void InitPreRanker(Geocoder::Params const &geocoderParams, - SearchParams const &searchParams); + void InitGeocoder(Geocoder::Params & geocoderParams, SearchParams const & searchParams); + void InitPreRanker(Geocoder::Params const & geocoderParams, SearchParams const & searchParams); void InitRanker(Geocoder::Params const & geocoderParams, SearchParams const & searchParams); void InitEmitter(SearchParams const & searchParams); @@ -110,6 +108,10 @@ public: void OnBookmarksDetachedFromGroup(bookmarks::GroupId const & groupId, std::vector const & marks); + // base::Cancellable overrides: + void Reset() override; + bool IsCancelled() const override; + protected: Locales GetCategoryLocales() const; @@ -138,6 +140,8 @@ protected: m2::RectD m_viewport; boost::optional m_position; + bool m_lastUpdate = false; + // Suggestions language code, not the same as we use in mwm data int8_t m_inputLocaleCode = StringUtf8Multilang::kUnsupportedLanguageCode; int8_t m_currentLocaleCode = StringUtf8Multilang::kUnsupportedLanguageCode; diff --git a/search/ranker.cpp b/search/ranker.cpp index 7072719d37..0a22f5a1d7 100644 --- a/search/ranker.cpp +++ b/search/ranker.cpp @@ -671,8 +671,6 @@ void Ranker::UpdateResults(bool lastUpdate) } m_tentativeResults.erase(m_tentativeResults.begin(), m_tentativeResults.begin() + i); - m_preRankerResults.clear(); - // The last update must be handled by a call to Finish(). if (!lastUpdate) { @@ -709,6 +707,8 @@ void Ranker::MakeRankerResults(Geocoder::Params const & geocoderParams, if (!ResultExists(*p, results, m_params.m_minDistanceBetweenResultsM)) results.push_back(move(*p)); }; + + m_preRankerResults.clear(); } void Ranker::GetBestMatchName(FeatureType & f, string & name) const diff --git a/search/ranker.hpp b/search/ranker.hpp index 1b9b20f657..627dd3760e 100644 --- a/search/ranker.hpp +++ b/search/ranker.hpp @@ -21,10 +21,11 @@ #include "base/string_utils.hpp" +#include #include #include +#include #include -#include #include class CategoriesHolder; @@ -97,9 +98,10 @@ public: void SuggestStrings(); - virtual void SetPreRankerResults(std::vector && preRankerResults) + virtual void AddPreRankerResults(std::vector && preRankerResults) { - m_preRankerResults = std::move(preRankerResults); + std::move(preRankerResults.begin(), preRankerResults.end(), + std::back_inserter(m_preRankerResults)); } virtual void UpdateResults(bool lastUpdate); diff --git a/search/retrieval.cpp b/search/retrieval.cpp index 7de92fa20a..40c75f9388 100644 --- a/search/retrieval.cpp +++ b/search/retrieval.cpp @@ -1,4 +1,4 @@ -#include "retrieval.hpp" +#include "search/retrieval.hpp" #include "search/cancel_exception.hpp" #include "search/feature_offset_match.hpp" @@ -59,15 +59,25 @@ public: { if ((++m_counter & 0xFF) == 0) BailIfCancelled(m_cancellable); + m_features.emplace_back(value.m_featureId); if (exactMatch) m_exactlyMatchedFeatures.emplace_back(value.m_featureId); } - void operator()(uint32_t feature) { m_features.emplace_back(feature); } + void operator()(uint32_t feature) + { + if ((++m_counter & 0xFF) == 0) + BailIfCancelled(m_cancellable); + + m_features.emplace_back(feature); + } void operator()(uint64_t feature, bool exactMatch) { + if ((++m_counter & 0xFF) == 0) + BailIfCancelled(m_cancellable); + m_features.emplace_back(feature); if (exactMatch) m_exactlyMatchedFeatures.emplace_back(feature); diff --git a/search/search_integration_tests/pre_ranker_test.cpp b/search/search_integration_tests/pre_ranker_test.cpp index 3c31e9a557..7136b30ad2 100644 --- a/search/search_integration_tests/pre_ranker_test.cpp +++ b/search/search_integration_tests/pre_ranker_test.cpp @@ -68,7 +68,7 @@ public: inline bool Finished() const { return m_finished; } // Ranker overrides: - void SetPreRankerResults(vector && preRankerResults) override + void AddPreRankerResults(vector && preRankerResults) override { CHECK(!Finished(), ()); move(preRankerResults.begin(), preRankerResults.end(), back_inserter(m_results)); diff --git a/search/search_params.cpp b/search/search_params.cpp index 3c97e78cf3..9c4b8b544f 100644 --- a/search/search_params.cpp +++ b/search/search_params.cpp @@ -9,9 +9,14 @@ #include using namespace std; +using namespace std::chrono; namespace search { +// static +steady_clock::duration const SearchParams::kDefaultTimeout = seconds(3); +steady_clock::duration const SearchParams::kDefaultDesktopTimeout = milliseconds(1500); + bool SearchParams::IsEqualCommon(SearchParams const & rhs) const { return m_query == rhs.m_query && m_inputLocale == rhs.m_inputLocale && diff --git a/search/search_params.hpp b/search/search_params.hpp index 349b472284..42de353ad6 100644 --- a/search/search_params.hpp +++ b/search/search_params.hpp @@ -8,6 +8,7 @@ #include "geometry/point2d.hpp" #include "geometry/rect2d.hpp" +#include #include #include #include @@ -26,6 +27,8 @@ struct SearchParams static size_t const kDefaultBatchSizeEverywhere = 10; static size_t const kDefaultNumResultsEverywhere = 30; static size_t const kDefaultNumResultsInViewport = 200; + static std::chrono::steady_clock::duration const kDefaultTimeout; + static std::chrono::steady_clock::duration const kDefaultDesktopTimeout; using OnStarted = std::function; using OnResults = std::function; @@ -83,6 +86,9 @@ struct SearchParams bookmarks::GroupId m_bookmarksGroupId = bookmarks::kInvalidGroupId; + // Amount of time after which the search is aborted. + std::chrono::steady_clock::duration m_timeout = kDefaultTimeout; + std::shared_ptr m_tracer; }; diff --git a/search/search_quality/assessment_tool/search_request_runner.cpp b/search/search_quality/assessment_tool/search_request_runner.cpp index b031d8ce1d..c3b1e8c4e7 100644 --- a/search/search_quality/assessment_tool/search_request_runner.cpp +++ b/search/search_quality/assessment_tool/search_request_runner.cpp @@ -5,6 +5,7 @@ #include "base/assert.hpp" #include "base/logging.hpp" +#include #include using namespace std; @@ -89,6 +90,7 @@ void SearchRequestRunner::RunRequest(size_t index, bool background, size_t times search::SearchParams params; sample.FillSearchParams(params); + params.m_timeout = search::SearchParams::kDefaultDesktopTimeout; params.m_onResults = [=](search::Results const & results) { vector> relevances; vector goldenMatching; diff --git a/search/search_quality/features_collector_tool/features_collector_tool.cpp b/search/search_quality/features_collector_tool/features_collector_tool.cpp index 761f7653d5..b84bcfc978 100644 --- a/search/search_quality/features_collector_tool/features_collector_tool.cpp +++ b/search/search_quality/features_collector_tool/features_collector_tool.cpp @@ -152,6 +152,7 @@ int main(int argc, char * argv[]) sample.FillSearchParams(params); params.m_batchSize = 100; params.m_maxNumResults = 300; + params.m_timeout = search::SearchParams::kDefaultDesktopTimeout; TestSearchRequest request(*engine, params); request.Run();