From 694410c11b1038856340e79a6b81ef3963497f22 Mon Sep 17 00:00:00 2001 From: Daria Volvenkova Date: Thu, 1 Dec 2016 17:47:42 +0300 Subject: [PATCH] Review fixes. --- map/traffic_manager.cpp | 114 +++++++++++++++++++++------------------ map/traffic_manager.hpp | 31 ++++++----- traffic/traffic_info.cpp | 46 ++++++++-------- traffic/traffic_info.hpp | 8 +-- 4 files changed, 106 insertions(+), 93 deletions(-) diff --git a/map/traffic_manager.cpp b/map/traffic_manager.cpp index 0f65b1b129..be2c9b4e3e 100644 --- a/map/traffic_manager.cpp +++ b/map/traffic_manager.cpp @@ -1,7 +1,5 @@ #include "map/traffic_manager.hpp" -#include "platform/platform.hpp" - #include "routing/routing_helpers.hpp" #include "drape_frontend/drape_engine.hpp" @@ -10,6 +8,8 @@ #include "indexer/ftypes_matcher.hpp" #include "indexer/scales.hpp" +#include "platform/platform.hpp" + namespace { auto constexpr kUpdateInterval = minutes(1); @@ -19,10 +19,28 @@ auto constexpr kNetworkErrorTimeout = minutes(20); auto constexpr kMaxRetriesCount = 5; } // namespace + +TrafficManager::CacheEntry::CacheEntry() + : m_isLoaded(false) + , m_dataSize(0) + , m_retriesCount(0) + , m_isWaitingForResponse(false) + , m_lastAvailability(traffic::TrafficInfo::Availability::Unknown) +{} + +TrafficManager::CacheEntry::CacheEntry(time_point const & requestTime) + : m_isLoaded(false) + , m_dataSize(0) + , m_lastRequestTime(requestTime) + , m_retriesCount(0) + , m_isWaitingForResponse(false) + , m_lastAvailability(traffic::TrafficInfo::Availability::Unknown) +{} + TrafficManager::TrafficManager(GetMwmsByRectFn const & getMwmsByRectFn, size_t maxCacheSizeBytes) : m_getMwmsByRectFn(getMwmsByRectFn) + , m_currentDataVersion(0) , m_state(TrafficState::Disabled) - , m_notifyStateChanged(false) , m_maxCacheSizeBytes(maxCacheSizeBytes) , m_isRunning(true) , m_thread(&TrafficManager::ThreadRoutine, this) @@ -33,7 +51,7 @@ TrafficManager::TrafficManager(GetMwmsByRectFn const & getMwmsByRectFn, size_t m TrafficManager::~TrafficManager() { { - lock_guard lock(m_requestedMwmsLock); + lock_guard lock(m_mutex); m_isRunning = false; } m_condition.notify_one(); @@ -51,7 +69,15 @@ void TrafficManager::SetStateListener(TrafficStateChangedFn const & onStateChang void TrafficManager::SetEnabled(bool enabled) { { - lock_guard lock(m_requestedMwmsLock); + lock_guard lock(m_mutex); + if (enabled == IsEnabled()) + { + LOG(LWARNING, ("Invalid attempt to", enabled ? "enable" : "disable", + "traffic manager, it's already", enabled ? "enabled" : "disabled", + ", doing nothing.")); + return; + } + Clear(); ChangeState(enabled ? TrafficState::Enabled : TrafficState::Disabled); } @@ -66,8 +92,6 @@ void TrafficManager::SetEnabled(bool enabled) if (m_currentPosition.second) UpdateMyPosition(m_currentPosition.first); } - - NotifyStateChanged(); } void TrafficManager::Clear() @@ -92,10 +116,8 @@ void TrafficManager::OnDestroyGLContext() if (!IsEnabled()) return; - { - lock_guard lock(m_requestedMwmsLock); - Clear(); - } + lock_guard lock(m_mutex); + Clear(); } void TrafficManager::OnRecoverGLContext() @@ -111,7 +133,7 @@ void TrafficManager::OnRecoverGLContext() void TrafficManager::UpdateViewport(ScreenBase const & screen) { - m_currentModelView = {screen, true}; + m_currentModelView = {screen, true /* initialized */}; if (!IsEnabled() || IsInvalidState()) return; @@ -123,7 +145,7 @@ void TrafficManager::UpdateViewport(ScreenBase const & screen) auto mwms = m_getMwmsByRectFn(screen.ClipRect()); { - lock_guard lock(m_requestedMwmsLock); + lock_guard lock(m_mutex); m_activeMwms.clear(); for (auto const & mwm : mwms) @@ -134,13 +156,11 @@ void TrafficManager::UpdateViewport(ScreenBase const & screen) RequestTrafficData(); } - - NotifyStateChanged(); } void TrafficManager::UpdateMyPosition(MyPosition const & myPosition) { - m_currentPosition = {myPosition, true}; + m_currentPosition = {myPosition, true /* initialized */}; if (!IsEnabled() || IsInvalidState()) return; @@ -170,8 +190,6 @@ void TrafficManager::ThreadRoutine() LOG(LWARNING, ("Traffic request failed. Mwm =", mwm)); OnTrafficRequestFailed(info); } - - NotifyStateChanged(); } mwms.clear(); } @@ -179,7 +197,7 @@ void TrafficManager::ThreadRoutine() bool TrafficManager::WaitForRequest(vector & mwms) { - unique_lock lock(m_requestedMwmsLock); + unique_lock lock(m_mutex); bool const timeout = !m_condition.wait_for(lock, kUpdateInterval, [this] { @@ -189,8 +207,11 @@ bool TrafficManager::WaitForRequest(vector & mwms) if (!m_isRunning) return false; - if (timeout && IsEnabled() && !IsInvalidState()) + if (timeout) { + if (!IsEnabled() || IsInvalidState()) + return true; + mwms.reserve(m_activeMwms.size()); for (auto const & mwmId : m_activeMwms) mwms.push_back(mwmId); @@ -248,7 +269,7 @@ void TrafficManager::RequestTrafficData(MwmSet::MwmId const & mwmId) void TrafficManager::OnTrafficRequestFailed(traffic::TrafficInfo const & info) { - lock_guard lock(m_requestedMwmsLock); + lock_guard lock(m_mutex); auto it = m_mwmCache.find(info.GetMwmId()); if (it == m_mwmCache.end()) @@ -257,24 +278,22 @@ void TrafficManager::OnTrafficRequestFailed(traffic::TrafficInfo const & info) it->second.m_isWaitingForResponse = false; it->second.m_lastAvailability = info.GetAvailability(); - if (info.GetAvailability() == traffic::TrafficInfo::Availability::Unknown) + if (info.GetAvailability() == traffic::TrafficInfo::Availability::Unknown && + !it->second.m_isLoaded) { - if (!it->second.m_isLoaded) + if (m_activeMwms.find(info.GetMwmId()) != m_activeMwms.end()) { - if (m_activeMwms.find(info.GetMwmId()) != m_activeMwms.end()) + if (it->second.m_retriesCount < kMaxRetriesCount) { - if (it->second.m_retriesCount < kMaxRetriesCount) - { - it->second.m_lastRequestTime = steady_clock::now(); - it->second.m_isWaitingForResponse = true; - RequestTrafficData(info.GetMwmId()); - } - ++it->second.m_retriesCount; - } - else - { - it->second.m_retriesCount = 0; + it->second.m_lastRequestTime = steady_clock::now(); + it->second.m_isWaitingForResponse = true; + RequestTrafficData(info.GetMwmId()); } + ++it->second.m_retriesCount; + } + else + { + it->second.m_retriesCount = 0; } } @@ -283,7 +302,7 @@ void TrafficManager::OnTrafficRequestFailed(traffic::TrafficInfo const & info) void TrafficManager::OnTrafficDataResponse(traffic::TrafficInfo const & info) { - lock_guard lock(m_requestedMwmsLock); + lock_guard lock(m_mutex); auto it = m_mwmCache.find(info.GetMwmId()); if (it == m_mwmCache.end()) @@ -355,7 +374,7 @@ void TrafficManager::UpdateState() bool waiting = false; bool networkError = false; bool expiredApp = false; - bool expiredMwm = false; + bool expiredData = false; bool noData = false; for (auto const & mwmId : m_activeMwms) @@ -370,7 +389,7 @@ void TrafficManager::UpdateState() else { expiredApp |= it->second.m_lastAvailability == traffic::TrafficInfo::Availability::ExpiredApp; - expiredMwm |= it->second.m_lastAvailability == traffic::TrafficInfo::Availability::ExpiredMwm; + expiredData |= it->second.m_lastAvailability == traffic::TrafficInfo::Availability::ExpiredData; noData |= it->second.m_lastAvailability == traffic::TrafficInfo::Availability::NoData; } @@ -392,7 +411,7 @@ void TrafficManager::UpdateState() ChangeState(TrafficState::WaitingData); else if (expiredApp) ChangeState(TrafficState::ExpiredApp); - else if (expiredMwm) + else if (expiredData) ChangeState(TrafficState::ExpiredData); else if (noData) ChangeState(TrafficState::NoData); @@ -408,19 +427,10 @@ void TrafficManager::ChangeState(TrafficState newState) return; m_state = newState; - m_notifyStateChanged = true; -} -void TrafficManager::NotifyStateChanged() -{ - if (m_notifyStateChanged) + GetPlatform().RunOnGuiThread([this, newState]() { - m_notifyStateChanged = false; - - GetPlatform().RunOnGuiThread([this]() - { - if (m_onStateChangedFn != nullptr) - m_onStateChangedFn(m_state); - }); - } + if (m_onStateChangedFn != nullptr) + m_onStateChangedFn(newState); + }); } diff --git a/map/traffic_manager.hpp b/map/traffic_manager.hpp index 87da904521..865fd565e1 100644 --- a/map/traffic_manager.hpp +++ b/map/traffic_manager.hpp @@ -75,18 +75,19 @@ private: void ThreadRoutine(); bool WaitForRequest(vector & mwms); - void RequestTrafficData(); - void RequestTrafficData(MwmSet::MwmId const & mwmId); - void OnTrafficDataResponse(traffic::TrafficInfo const & info); void OnTrafficRequestFailed(traffic::TrafficInfo const & info); +private: + // This is a group of methods that haven't their own synchronization inside. + void RequestTrafficData(); + void RequestTrafficData(MwmSet::MwmId const & mwmId); + void Clear(); void CheckCacheSize(); void UpdateState(); void ChangeState(TrafficState newState); - void NotifyStateChanged(); bool IsInvalidState() const; bool IsEnabled() const; @@ -94,33 +95,31 @@ private: GetMwmsByRectFn m_getMwmsByRectFn; ref_ptr m_drapeEngine; - int64_t m_currentDataVersion = 0; + atomic m_currentDataVersion; + // These fields have a flag of their initialization. pair m_currentPosition = {MyPosition(), false}; pair m_currentModelView = {ScreenBase(), false}; atomic m_state; - atomic m_notifyStateChanged; TrafficStateChangedFn m_onStateChangedFn; struct CacheEntry { - CacheEntry() = default; + CacheEntry(); + CacheEntry(time_point const & requestTime); - CacheEntry(time_point const & requestTime) : m_lastRequestTime(requestTime) {} - - bool m_isLoaded = false; - size_t m_dataSize = 0; + bool m_isLoaded; + size_t m_dataSize; time_point m_lastSeenTime; time_point m_lastRequestTime; time_point m_lastResponseTime; - uint32_t m_retriesCount = 0; - bool m_isWaitingForResponse = false; + int m_retriesCount; + bool m_isWaitingForResponse; - traffic::TrafficInfo::Availability m_lastAvailability = - traffic::TrafficInfo::Availability::Unknown; + traffic::TrafficInfo::Availability m_lastAvailability; }; size_t m_maxCacheSizeBytes; @@ -134,6 +133,6 @@ private: set m_activeMwms; vector m_requestedMwms; - mutex m_requestedMwmsLock; + mutex m_mutex; thread m_thread; }; diff --git a/traffic/traffic_info.cpp b/traffic/traffic_info.cpp index a7e56bceab..e02c00e900 100644 --- a/traffic/traffic_info.cpp +++ b/traffic/traffic_info.cpp @@ -4,12 +4,14 @@ #include "coding/bit_streams.hpp" #include "coding/reader.hpp" +#include "coding/url_encode.hpp" #include "coding/varint.hpp" #include "coding/write_to_sink.hpp" #include "coding/writer.hpp" #include "base/assert.hpp" #include "base/logging.hpp" +#include "base/string_utils.hpp" #include "std/algorithm.hpp" #include "std/string.hpp" @@ -22,18 +24,21 @@ namespace traffic { namespace { -bool ReadRemoteFile(string const & url, string & result, int & errorCode) +bool ReadRemoteFile(string const & url, vector & contents, int & errorCode) { platform::HttpClient request(url); if (!request.RunHttpRequest()) { - LOG(LINFO, ("Couldn't run traffic request", url)); - errorCode = -1; + errorCode = request.ErrorCode(); + LOG(LINFO, ("Couldn't run traffic request", url, ". Error:", errorCode)); return false; } errorCode = request.ErrorCode(); - result = request.ServerResponse(); + + string const & result = request.ServerResponse(); + contents.resize(result.size()); + memcpy(contents.data(), result.data(), result.size()); if (errorCode != 200) { @@ -53,7 +58,7 @@ string MakeRemoteURL(string const & name, uint64_t version) ss << TRAFFIC_DATA_BASE_URL; if (version != 0) ss << version << "/"; - ss << name << TRAFFIC_FILE_EXTENSION; + ss << UrlEncode(name) << TRAFFIC_FILE_EXTENSION; return ss.str(); } } // namespace @@ -67,7 +72,7 @@ TrafficInfo::RoadSegmentId::RoadSegmentId(uint32_t fid, uint16_t idx, uint8_t di } // TrafficInfo -------------------------------------------------------------------------------- -TrafficInfo::TrafficInfo(MwmSet::MwmId const & mwmId, int64_t const & currentDataVersion) +TrafficInfo::TrafficInfo(MwmSet::MwmId const & mwmId, int64_t currentDataVersion) : m_mwmId(mwmId) , m_currentDataVersion(currentDataVersion) {} @@ -83,33 +88,31 @@ bool TrafficInfo::ReceiveTrafficData() if (url.empty()) return false; - string result; + vector contents; int errorCode; - if (!ReadRemoteFile(url, result, errorCode)) + if (!ReadRemoteFile(url, contents, errorCode)) { if (errorCode == 404) { - int64_t version = atoi(result.c_str()); + string const result(reinterpret_cast(contents.data()), contents.size()); + + int64_t version = 0; + strings::to_int64(result.c_str(), version); if (version > info->GetVersion() && version <= m_currentDataVersion) - m_availabilityStatus = Availability::ExpiredMwm; + m_availability = Availability::ExpiredData; else if (version > m_currentDataVersion) - m_availabilityStatus = Availability::ExpiredApp; + m_availability = Availability::ExpiredApp; else - m_availabilityStatus = Availability::NoData; + m_availability = Availability::NoData; } else { - m_availabilityStatus = Availability::Unknown; + m_availability = Availability::Unknown; } return false; } - vector contents; - contents.resize(result.size()); - for (size_t i = 0; i < result.size(); ++i) - contents[i] = static_cast(result[i]); - Coloring coloring; try { @@ -117,12 +120,13 @@ bool TrafficInfo::ReceiveTrafficData() } catch (Reader::Exception const & e) { - LOG(LINFO, ("Could not read traffic data received from server. MWM:", info->GetCountryName(), - "Version:", info->GetVersion())); + m_availability = Availability::NoData; + LOG(LWARNING, ("Could not read traffic data received from server. MWM:", info->GetCountryName(), + "Version:", info->GetVersion())); return false; } m_coloring.swap(coloring); - m_availabilityStatus = Availability::IsAvailable; + m_availability = Availability::IsAvailable; return true; } diff --git a/traffic/traffic_info.hpp b/traffic/traffic_info.hpp index 92de4f6b2a..a07e4d8cc4 100644 --- a/traffic/traffic_info.hpp +++ b/traffic/traffic_info.hpp @@ -20,7 +20,7 @@ public: { IsAvailable, NoData, - ExpiredMwm, + ExpiredData, ExpiredApp, Unknown }; @@ -65,7 +65,7 @@ public: TrafficInfo() = default; - TrafficInfo(MwmSet::MwmId const & mwmId, int64_t const & currentDataVersion); + TrafficInfo(MwmSet::MwmId const & mwmId, int64_t currentDataVersion); // Fetches the latest traffic data from the server and updates the coloring. // Construct the url by passing an MwmId. @@ -77,7 +77,7 @@ public: MwmSet::MwmId const & GetMwmId() const { return m_mwmId; } Coloring const & GetColoring() const { return m_coloring; } - Availability GetAvailability() const { return m_availabilityStatus; } + Availability GetAvailability() const { return m_availability; } static void SerializeTrafficData(Coloring const & coloring, vector & result); @@ -87,7 +87,7 @@ private: // The mapping from feature segments to speed groups (see speed_groups.hpp). Coloring m_coloring; MwmSet::MwmId m_mwmId; - Availability m_availabilityStatus = Availability::Unknown; + Availability m_availability = Availability::Unknown; int64_t m_currentDataVersion = 0; }; } // namespace traffic