From 4ac645a02d6d0e8f57a311bd92421b502f557bbd Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Tue, 12 Dec 2017 17:08:03 +0300 Subject: [PATCH] [booking] review fixes - no mutexes --- map/booking_filter.cpp | 87 +++++++++++++++++++++++++++--------- map/booking_filter.hpp | 14 ++++-- map/booking_filter_cache.cpp | 10 ----- map/booking_filter_cache.hpp | 6 --- map/framework.cpp | 55 +++++++++++++---------- map/framework.hpp | 5 ++- 6 files changed, 112 insertions(+), 65 deletions(-) diff --git a/map/booking_filter.cpp b/map/booking_filter.cpp index 20dfbacae1..4b6b6b057a 100644 --- a/map/booking_filter.cpp +++ b/map/booking_filter.cpp @@ -144,6 +144,49 @@ void PrepareData(Index const & index, search::Results const & results, p.m_hotelIds.push_back(std::move(hotelId)); } } + +void GetAvailableFeaturesFromCacheImpl(Index const & index, search::Results const & results, + availability::Cache & cache, + std::vector & sortedResults) +{ + std::vector features; + + for (auto const & r : results) + { + if (!r.m_metadata.m_isSponsoredHotel || + r.GetResultType() != search::Result::ResultType::RESULT_FEATURE) + { + continue; + } + + features.push_back(r.GetFeatureID()); + } + + std::sort(features.begin(), features.end()); + + MwmSet::MwmId mwmId; + std::unique_ptr guard; + for (auto const & featureId : features) + { + if (mwmId != featureId.m_mwmId) + { + guard = my::make_unique(index, featureId.m_mwmId); + mwmId = featureId.m_mwmId; + } + + FeatureType ft; + if (!guard->GetFeatureByIndex(featureId.m_index, ft)) + { + LOG(LERROR, ("Feature can't be loaded:", featureId)); + continue; + } + + auto const & hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID); + + if (cache.Get(hotelId) == availability::Cache::HotelStatus::Available) + sortedResults.push_back(featureId); + } +} } // namespace namespace booking @@ -157,18 +200,12 @@ void Filter::FilterAvailability(search::Results const & results, { GetPlatform().RunTask(Platform::Thread::File, [this, results, params]() { - std::lock_guard lock(m_mutex); - auto const & cb = params.m_callback; ASSERT(params.m_params.m_hotelIds.empty(), ()); m_currentParams.m_hotelIds.clear(); - if (m_currentParams != params.m_params) - { - m_currentParams = std::move(params.m_params); - m_availabilityCache = std::make_shared(); - } + UpdateAvailabilityParams(std::move(params.m_params)); HotelToResults hotelToResults; PrepareData(m_index, results, hotelToResults, *m_availabilityCache, m_currentParams); @@ -199,22 +236,32 @@ void Filter::FilterAvailability(search::Results const & results, }); } -availability::Cache::HotelStatus Filter::GetHotelAvailabilityStatus(std::string const & hotelId) -{ - std::lock_guard lock(m_mutex); - - return m_availabilityCache->Get(hotelId); -} - void Filter::OnParamsUpdated(AvailabilityParams const & params) { - std::lock_guard lock(m_mutex); - - if (m_currentParams != params) + GetPlatform().RunTask(Platform::Thread::File, [this, params]() { - m_currentParams = params; - m_availabilityCache = std::make_shared(); - } + UpdateAvailabilityParams(std::move(params)); + }); +} + +void Filter::GetAvailableFeaturesFromCache(search::Results const & results, + FillSearchMarksCallback const & callback) +{ + GetPlatform().RunTask(Platform::Thread::File, [this, results, callback]() + { + std::vector resultSorted; + GetAvailableFeaturesFromCacheImpl(m_index, results, *m_availabilityCache, resultSorted); + callback(resultSorted); + }); +} + +void Filter::UpdateAvailabilityParams(AvailabilityParams params) +{ + if (m_currentParams == params) + return; + + m_currentParams = std::move(params); + m_availabilityCache = std::make_shared(); } } // namespace filter } // namespace booking diff --git a/map/booking_filter.hpp b/map/booking_filter.hpp index 5e475f5249..684fec3f71 100644 --- a/map/booking_filter.hpp +++ b/map/booking_filter.hpp @@ -5,8 +5,9 @@ #include "base/macros.hpp" +#include #include -#include +#include class Index; @@ -21,7 +22,9 @@ class Api; namespace filter { -// NOTE: this class IS thread-safe. + +using FillSearchMarksCallback = std::function availableHotelsSorted)>; + class Filter { public: @@ -32,9 +35,13 @@ public: void OnParamsUpdated(AvailabilityParams const & params); - availability::Cache::HotelStatus GetHotelAvailabilityStatus(std::string const & hotelId); + void GetAvailableFeaturesFromCache(search::Results const & results, + FillSearchMarksCallback const & callback); private: + + void UpdateAvailabilityParams(AvailabilityParams params); + Index const & m_index; Api const & m_api; @@ -43,7 +50,6 @@ private: CachePtr m_availabilityCache = std::make_shared(); AvailabilityParams m_currentParams; - std::mutex m_mutex; DISALLOW_COPY_AND_MOVE(Filter); }; diff --git a/map/booking_filter_cache.cpp b/map/booking_filter_cache.cpp index 6af551f226..32dc68d6ae 100644 --- a/map/booking_filter_cache.cpp +++ b/map/booking_filter_cache.cpp @@ -15,8 +15,6 @@ Cache::Cache(size_t maxCount, size_t expiryPeriodSeconds) Cache::HotelStatus Cache::Get(std::string const & hotelId) { - std::lock_guard lock(m_mutex); - HotelStatus result = Get(m_notReadyHotels, hotelId); if (result == HotelStatus::Absent) @@ -27,8 +25,6 @@ Cache::HotelStatus Cache::Get(std::string const & hotelId) void Cache::Reserve(std::string const & hotelId) { - std::lock_guard lock(m_mutex); - ASSERT(m_hotelToStatus.find(hotelId) == m_hotelToStatus.end(), ()); m_notReadyHotels.emplace(hotelId, Item()); @@ -36,8 +32,6 @@ void Cache::Reserve(std::string const & hotelId) void Cache::Insert(std::string const & hotelId, HotelStatus const s) { - std::lock_guard lock(m_mutex); - ASSERT_NOT_EQUAL(s, HotelStatus::NotReady, ("Please, use Cache::Reserve method for HotelStatus::NotReady")); @@ -50,8 +44,6 @@ void Cache::Insert(std::string const & hotelId, HotelStatus const s) void Cache::RemoveOutdated() { - std::lock_guard lock(m_mutex); - if (m_expiryPeriodSeconds == 0) return; @@ -61,8 +53,6 @@ void Cache::RemoveOutdated() void Cache::Clear() { - std::lock_guard lock(m_mutex); - m_hotelToStatus.clear(); m_notReadyHotels.clear(); } diff --git a/map/booking_filter_cache.hpp b/map/booking_filter_cache.hpp index 3c1045e36c..049f086838 100644 --- a/map/booking_filter_cache.hpp +++ b/map/booking_filter_cache.hpp @@ -4,7 +4,6 @@ #include #include -#include namespace booking { @@ -12,7 +11,6 @@ namespace filter { namespace availability { -// NOTE: this class IS thread-safe. class Cache { public: @@ -66,10 +64,6 @@ private: size_t const m_maxCount = 1000; // Do not use aging when |m_expiryPeriodSeconds| is equal to zero. size_t const m_expiryPeriodSeconds = 60; - - std::mutex m_mutex; - - DISALLOW_COPY_AND_MOVE(Cache); }; std::string DebugPrint(Cache::HotelStatus status); diff --git a/map/framework.cpp b/map/framework.cpp index 305d043b23..aba40bf4a1 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -1599,7 +1599,8 @@ void Framework::FillSearchResultsMarks(bool clear, search::Results const & resul } void Framework::FillSearchResultsMarks(bool clear, search::Results::ConstIter begin, - search::Results::ConstIter end) + search::Results::ConstIter end, + SearchMarkPostProcesing fn /* = nullptr */) { UserMarkNotificationGuard guard(GetBookmarkManager(), UserMark::Type::SEARCH); @@ -1638,8 +1639,8 @@ void Framework::FillSearchResultsMarks(bool clear, search::Results::ConstIter be if (r.m_metadata.m_isSponsoredHotel) mark->SetMarkType(SearchMarkType::Booking); - if (GetSearchAPI().GetSponsoredMode() == SearchAPI::SponsoredMode::Booking) - SetPreparingStateForBookingHotel(r.GetFeatureID(), mark); + if (fn) + fn(*mark); } } @@ -3185,7 +3186,34 @@ void Framework::SetSearchDisplacementModeEnabled(bool enabled) void Framework::ShowViewportSearchResults(bool clear, search::Results::ConstIter begin, search::Results::ConstIter end) { - FillSearchResultsMarks(clear, begin, end); + if (GetSearchAPI().GetSponsoredMode() != SearchAPI::SponsoredMode::Booking) + { + FillSearchResultsMarks(clear, begin, end); + return; + } + + search::Results results; + results.AddResultsNoChecks(begin, end); + + auto const fillCallback = [this, clear, results] (std::vector featuresSorted) + { + auto const postProcessing = [featuresSorted] (SearchMarkPoint & mark) + { + auto const & id = mark.GetFeatureID(); + + if (!id.IsValid()) + return; + + auto const isAvailable = + std::binary_search(featuresSorted.cbegin(), featuresSorted.cend(), id); + + mark.SetPreparing(!isAvailable); + }; + + FillSearchResultsMarks(clear, results.begin(), results.end(), postProcessing); + }; + + m_bookingFilter.GetAvailableFeaturesFromCache(results, fillCallback); } void Framework::ClearViewportSearchResults() @@ -3456,22 +3484,3 @@ void Framework::OnBookingFilterParamsUpdate(booking::AvailabilityParams const & { m_bookingFilter.OnParamsUpdated(params); } - -void Framework::SetPreparingStateForBookingHotel(FeatureID const & id, SearchMarkPoint * mark) -{ - using booking::filter::availability::Cache; - Index::FeaturesLoaderGuard guard(m_model.GetIndex(), id.m_mwmId); - - FeatureType ft; - if (!guard.GetFeatureByIndex(id.m_index, ft)) - { - LOG(LERROR, ("Feature can't be loaded:", id)); - return; - } - - auto const & hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID); - - auto const isPreparing = - m_bookingFilter.GetHotelAvailabilityStatus(hotelId) != Cache::HotelStatus::Available; - mark->SetPreparing(isPreparing); -} diff --git a/map/framework.hpp b/map/framework.hpp index f7933a47b6..f5d26dad94 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -551,9 +551,11 @@ public: size_t ShowSearchResults(search::Results const & results); + using SearchMarkPostProcesing = function; + void FillSearchResultsMarks(bool clear, search::Results const & results); void FillSearchResultsMarks(bool clear, search::Results::ConstIter begin, - search::Results::ConstIter end); + search::Results::ConstIter end, SearchMarkPostProcesing fn = nullptr); void ClearSearchResultsMarks(); list const & GetLastSearchQueries() const { return m_searchQuerySaver.Get(); } @@ -853,7 +855,6 @@ public: void OnBookingFilterParamsUpdate(booking::AvailabilityParams const & params) override; private: - void SetPreparingStateForBookingHotel(FeatureID const & id, SearchMarkPoint * mark); // m_discoveryManager must be bellow m_searchApi, m_viatorApi, m_localsApi unique_ptr m_discoveryManager; };