From 0adbd86f8529e268b86d991543a3eaa969e4f563 Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Wed, 18 Sep 2019 19:27:50 +0300 Subject: [PATCH] [storage] got rid mutex and separate worker thread from diff manager --- storage/diff_scheme/diff_manager.cpp | 61 ++++++++++---------------- storage/diff_scheme/diff_manager.hpp | 3 -- storage/storage.cpp | 64 ++++++++++++++-------------- 3 files changed, 55 insertions(+), 73 deletions(-) diff --git a/storage/diff_scheme/diff_manager.cpp b/storage/diff_scheme/diff_manager.cpp index 0a36a2b1de..beb2e16928 100644 --- a/storage/diff_scheme/diff_manager.cpp +++ b/storage/diff_scheme/diff_manager.cpp @@ -28,32 +28,25 @@ namespace diffs void Manager::Load(LocalMapsInfo && info) { LocalMapsInfo localMapsInfo = info; - { - std::lock_guard lock(m_mutex); - m_localMapsInfo = std::move(info); - } + m_localMapsInfo = std::move(info); - m_workerThread.Push([this, localMapsInfo] { - NameDiffInfoMap diffs = Checker::Check(localMapsInfo); + GetPlatform().RunTask(Platform::Thread::Network, [this, info = std::move(localMapsInfo)] { + NameDiffInfoMap diffs = Checker::Check(info); - std::lock_guard lock(m_mutex); + GetPlatform().RunTask(Platform::Thread::Gui, [this, diffs = std::move(diffs)] { + m_diffs = std::move(diffs); + if (m_diffs.empty()) + { + m_status = Status::NotAvailable; - m_diffs = std::move(diffs); - if (m_diffs.empty()) - { - m_status = Status::NotAvailable; + alohalytics::Stats::Instance().LogEvent("Downloader_DiffScheme_OnStart_fallback"); + } + else + { + m_status = Status::Available; + } - alohalytics::Stats::Instance().LogEvent("Downloader_DiffScheme_OnStart_fallback"); - } - else - { - m_status = Status::Available; - } - - auto & observers = m_observers; - auto status = m_status; - GetPlatform().RunTask(Platform::Thread::Gui, [observers, status]() mutable { - observers.ForEach(&Observer::OnDiffStatusReceived, status); + m_observers.ForEach(&Observer::OnDiffStatusReceived, m_status); }); }); } @@ -63,7 +56,7 @@ void Manager::ApplyDiff(ApplyDiffParams && p, base::Cancellable const & cancella { using namespace generator::mwm_diff; - m_workerThread.Push([this, p, &cancellable, task] { + GetPlatform().RunTask(Platform::Thread::File, [this, p = std::move(p), &cancellable, task] { CHECK(p.m_diffFile, ()); CHECK(p.m_oldMwmFile, ()); @@ -104,42 +97,39 @@ void Manager::ApplyDiff(ApplyDiffParams && p, base::Cancellable const & cancella switch (result) { case DiffApplicationResult::Ok: - { diffFile->DeleteFromDisk(MapOptions::Diff); break; - } case DiffApplicationResult::Cancelled: // The diff file will be deleted by storage. // Another way would be to leave it on disk but all consequences // of interacting with storage are much harder to be taken into account that way. break; case DiffApplicationResult::Failed: - { diffFile->DeleteFromDisk(MapOptions::Diff); alohalytics::Stats::Instance().LogEvent( "Downloader_DiffScheme_error", {{"type", "patching"}, {"error", isFilePrepared ? "Cannot apply diff" : "Cannot prepare file"}}); - - std::lock_guard lock(m_mutex); - m_status = Status::NotAvailable; break; } - } - task(result); + GetPlatform().RunTask(Platform::Thread::Gui, [this, task, result]() + { + if (result == DiffApplicationResult::Failed) + m_status = Status::NotAvailable; + + task(result); + }); }); } Status Manager::GetStatus() const { - std::lock_guard lock(m_mutex); return m_status; } bool Manager::SizeFor(storage::CountryId const & countryId, uint64_t & size) const { - std::lock_guard lock(m_mutex); if (m_status != Status::Available) return false; @@ -168,7 +158,6 @@ bool Manager::HasDiffFor(storage::CountryId const & countryId) const void Manager::MarkAsApplied(storage::CountryId const & countryId) { - std::lock_guard lock(m_mutex); auto it = m_diffs.find(countryId); if (it == m_diffs.end()) return; @@ -181,7 +170,6 @@ void Manager::MarkAsApplied(storage::CountryId const & countryId) void Manager::RemoveDiffForCountry(storage::CountryId const & countryId) { - std::lock_guard lock(m_mutex); m_diffs.erase(countryId); if (m_diffs.empty() || !IsDiffsAvailable(m_diffs)) @@ -190,15 +178,12 @@ void Manager::RemoveDiffForCountry(storage::CountryId const & countryId) void Manager::AbortDiffScheme() { - std::lock_guard lock(m_mutex); m_status = Status::NotAvailable; m_diffs.clear(); } bool Manager::IsPossibleToAutoupdate() const { - std::lock_guard lock(m_mutex); - if (m_status != Status::Available) return false; diff --git a/storage/diff_scheme/diff_manager.hpp b/storage/diff_scheme/diff_manager.hpp index c47ad08e5c..04269315e4 100644 --- a/storage/diff_scheme/diff_manager.hpp +++ b/storage/diff_scheme/diff_manager.hpp @@ -77,7 +77,6 @@ private: template bool WithNotAppliedDiff(storage::CountryId const & countryId, Fn && fn) const { - std::lock_guard lock(m_mutex); if (m_status != Status::Available) return false; @@ -89,12 +88,10 @@ private: return true; } - mutable std::mutex m_mutex; Status m_status = Status::Undefined; NameDiffInfoMap m_diffs; LocalMapsInfo m_localMapsInfo; base::ObserverListUnsafe m_observers; - base::thread_pool::delayed::ThreadPool m_workerThread; }; } // namespace diffs } // namespace storage diff --git a/storage/storage.cpp b/storage/storage.cpp index 12d4949260..7e2b2d8c3e 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -1570,47 +1570,47 @@ void Storage::ApplyDiff(CountryId const & countryId, functionGetCountryName(), kSourceKey)) { - base::DeleteFileX(diffFile->GetPath(MapOptions::Map)); + GetPlatform().RunTask(Platform::Thread::File, + [path = diffFile->GetPath(MapOptions::Map)] { base::DeleteFileX(path); }); result = DiffApplicationResult::Failed; } - GetPlatform().RunTask(Platform::Thread::Gui, [this, fn, diffFile, countryId, result] { - auto realResult = result; - if (m_diffsBeingApplied.count(countryId) == 0 && realResult == DiffApplicationResult::Ok) - realResult = DiffApplicationResult::Cancelled; + if (m_diffsBeingApplied.count(countryId) == 0 && result == DiffApplicationResult::Ok) + result = DiffApplicationResult::Cancelled; - LOG(LINFO, ("Diff application result for", countryId, ":", realResult)); + LOG(LINFO, ("Diff application result for", countryId, ":", result)); - m_latestDiffRequest = {}; - m_diffsBeingApplied.erase(countryId); - switch (realResult) - { - case DiffApplicationResult::Ok: - { - RegisterCountryFiles(diffFile); - Platform::DisableBackupForFile(diffFile->GetPath(MapOptions::Map)); - m_diffManager.MarkAsApplied(countryId); - fn(true); - break; - } - case DiffApplicationResult::Cancelled: - { - if (m_downloader->IsIdle()) - DownloadNextCountryFromQueue(); - break; - } - case DiffApplicationResult::Failed: - { - m_diffManager.RemoveDiffForCountry(countryId); - fn(false); - break; - } - } - }); + m_latestDiffRequest = {}; + m_diffsBeingApplied.erase(countryId); + switch (result) + { + case DiffApplicationResult::Ok: + { + RegisterCountryFiles(diffFile); + Platform::DisableBackupForFile(diffFile->GetPath(MapOptions::Map)); + m_diffManager.MarkAsApplied(countryId); + fn(true); + break; + } + case DiffApplicationResult::Cancelled: + { + if (m_downloader->IsIdle()) + DownloadNextCountryFromQueue(); + break; + } + case DiffApplicationResult::Failed: + { + m_diffManager.RemoveDiffForCountry(countryId); + fn(false); + break; + } + } }); }