From a6ed5b7694b76f8c57ab78d7ff2d0928df6ad207 Mon Sep 17 00:00:00 2001 From: Maxim Pimenov Date: Tue, 17 Sep 2019 15:55:19 +0300 Subject: [PATCH] [indexer] Removed the UPDATE event type from MwmSet. This event was used to save up on the calls to observers but it seems we'll be better off without it for now. 1. With current clients, the overhead of two calls Deregister(old)+Register(new) compared to one call Update(new,old) is negligible. 2. The OnMapsUpdated call in FeaturesFetcher used to bypass the reference count in MwmSet, thus deleting mwm files that still had clients holding handles to them. --- editor/osm_editor.cpp | 5 +++++ editor/osm_editor.hpp | 8 ++----- indexer/indexer_tests/data_source_test.cpp | 17 +++------------ indexer/mwm_set.cpp | 25 +++++----------------- indexer/mwm_set.hpp | 14 +++--------- map/features_fetcher.cpp | 9 -------- map/features_fetcher.hpp | 2 -- storage/diff_scheme/diff_manager.cpp | 2 +- storage/storage.cpp | 10 ++++++++- 9 files changed, 28 insertions(+), 64 deletions(-) diff --git a/editor/osm_editor.cpp b/editor/osm_editor.cpp index 23dfd66b71..430d8056c0 100644 --- a/editor/osm_editor.cpp +++ b/editor/osm_editor.cpp @@ -302,6 +302,11 @@ void Editor::ClearAllLocalEdits() Invalidate(); } +void Editor::OnMapRegistered(platform::LocalCountryFile const & localFile) +{ + LoadEdits(); +} + void Editor::OnMapDeregistered(platform::LocalCountryFile const & localFile) { // Can be called on non-main-thread in case of simultaneous uploading process. diff --git a/editor/osm_editor.hpp b/editor/osm_editor.hpp index e0264e323d..d3f45d581d 100644 --- a/editor/osm_editor.hpp +++ b/editor/osm_editor.hpp @@ -122,12 +122,8 @@ public: /// Resets editor to initial state: no any edits or created/deleted features. void ClearAllLocalEdits(); - void OnMapUpdated(platform::LocalCountryFile const &, - platform::LocalCountryFile const &) override - { - LoadEdits(); - } - + // MwmSet::Observer overrides: + void OnMapRegistered(platform::LocalCountryFile const & localFile) override; void OnMapDeregistered(platform::LocalCountryFile const & localFile) override; using FeatureIndexFunctor = std::function; diff --git a/indexer/indexer_tests/data_source_test.cpp b/indexer/indexer_tests/data_source_test.cpp index 2b80c2a71f..3d9beb9839 100644 --- a/indexer/indexer_tests/data_source_test.cpp +++ b/indexer/indexer_tests/data_source_test.cpp @@ -38,12 +38,6 @@ public: AddEvent(m_expected, MwmSet::Event::TYPE_REGISTERED, localFile); } - void ExpectUpdated(platform::LocalCountryFile const & newFile, - platform::LocalCountryFile const & oldFile) - { - AddEvent(m_expected, MwmSet::Event::TYPE_UPDATED, newFile, oldFile); - } - void ExpectDeregistered(platform::LocalCountryFile const & localFile) { AddEvent(m_expected, MwmSet::Event::TYPE_DEREGISTERED, localFile); @@ -69,12 +63,6 @@ public: AddEvent(m_actual, MwmSet::Event::TYPE_REGISTERED, localFile); } - void OnMapUpdated(platform::LocalCountryFile const & newFile, - platform::LocalCountryFile const & oldFile) override - { - AddEvent(m_actual, MwmSet::Event::TYPE_UPDATED, newFile, oldFile); - } - void OnMapDeregistered(platform::LocalCountryFile const & localFile) override { AddEvent(m_actual, MwmSet::Event::TYPE_DEREGISTERED, localFile); @@ -107,7 +95,7 @@ UNIT_CLASS_TEST(DataSourceTest, StatusNotifications) CountryFile const country("minsk-pass"); // These two classes point to the same file, but will be considered - // by Index as distinct files because versions are artificially set + // by DataSource as distinct files because versions are artificially set // to different numbers. LocalCountryFile const file1(mapsDir, country, 1 /* version */); LocalCountryFile const file2(mapsDir, country, 2 /* version */); @@ -147,7 +135,8 @@ UNIT_CLASS_TEST(DataSourceTest, StatusNotifications) TEST(id2.IsAlive(), ()); TEST_NOT_EQUAL(id1, id2, ()); - ExpectUpdated(file2, file1); + ExpectDeregistered(file1); + ExpectRegistered(file2); TEST(CheckExpectations(), ()); } diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index 8496c0c5b3..fb414515bc 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -112,17 +112,8 @@ pair MwmSet::Register(LocalCountryFile const & // Deregister old mwm for the country. if (info->GetVersion() < localFile.GetVersion()) { - EventList subEvents; - DeregisterImpl(id, subEvents); - result = RegisterImpl(localFile, subEvents); - - // In the case of success all sub-events are - // replaced with a single UPDATE event. Otherwise, - // sub-events are reported as is. - if (result.second == MwmSet::RegResult::Success) - events.Add(Event(Event::TYPE_UPDATED, localFile, info->GetLocalFile())); - else - events.Append(subEvents); + DeregisterImpl(id, events); + result = RegisterImpl(localFile, events); return; } @@ -239,7 +230,8 @@ void MwmSet::SetStatus(MwmInfo & info, MwmInfo::Status status, EventList & event case MwmInfo::STATUS_REGISTERED: events.Add(Event(Event::TYPE_REGISTERED, info.GetLocalFile())); break; - case MwmInfo::STATUS_MARKED_TO_DEREGISTER: break; + case MwmInfo::STATUS_MARKED_TO_DEREGISTER: + break; case MwmInfo::STATUS_DEREGISTERED: events.Add(Event(Event::TYPE_DEREGISTERED, info.GetLocalFile())); break; @@ -255,9 +247,6 @@ void MwmSet::ProcessEventList(EventList & events) case Event::TYPE_REGISTERED: m_observers.ForEach(&Observer::OnMapRegistered, event.m_file); break; - case Event::TYPE_UPDATED: - m_observers.ForEach(&Observer::OnMapUpdated, event.m_file, event.m_oldFile); - break; case Event::TYPE_DEREGISTERED: m_observers.ForEach(&Observer::OnMapDeregistered, event.m_file); break; @@ -456,7 +445,6 @@ string DebugPrint(MwmSet::Event::Type type) switch (type) { case MwmSet::Event::TYPE_REGISTERED: return "Registered"; - case MwmSet::Event::TYPE_UPDATED: return "Updated"; case MwmSet::Event::TYPE_DEREGISTERED: return "Deregistered"; } return "Undefined"; @@ -465,9 +453,6 @@ string DebugPrint(MwmSet::Event::Type type) string DebugPrint(MwmSet::Event const & event) { ostringstream os; - os << "MwmSet::Event [" << DebugPrint(event.m_type) << ", " << DebugPrint(event.m_file); - if (event.m_type == MwmSet::Event::TYPE_UPDATED) - os << ", " << DebugPrint(event.m_oldFile); - os << "]"; + os << "MwmSet::Event [" << DebugPrint(event.m_type) << ", " << DebugPrint(event.m_file) << "]"; return os.str(); } diff --git a/indexer/mwm_set.hpp b/indexer/mwm_set.hpp index 406eebac87..4e102e788e 100644 --- a/indexer/mwm_set.hpp +++ b/indexer/mwm_set.hpp @@ -184,7 +184,6 @@ public: { TYPE_REGISTERED, TYPE_DEREGISTERED, - TYPE_UPDATED, }; Event() = default; @@ -248,18 +247,11 @@ public: public: virtual ~Observer() = default; - // Called when a map is registered for a first time and can be - // used. - virtual void OnMapRegistered(platform::LocalCountryFile const & /*localFile*/) {} - - // Called when a map is updated to a newer version. Feel free to - // treat it as combined OnMapRegistered(newFile) + - // OnMapDeregistered(oldFile). - virtual void OnMapUpdated(platform::LocalCountryFile const & /*newFile*/, - platform::LocalCountryFile const & /*oldFile*/) {} + // Called when a map is registered for the first time and can be used. + virtual void OnMapRegistered(platform::LocalCountryFile const & /* localFile */) {} // Called when a map is deregistered and can no longer be used. - virtual void OnMapDeregistered(platform::LocalCountryFile const & /*localFile*/) {} + virtual void OnMapDeregistered(platform::LocalCountryFile const & /* localFile */) {} }; /// Registers a new map. diff --git a/map/features_fetcher.cpp b/map/features_fetcher.cpp index b74dd6a346..c0b9e6f8dc 100644 --- a/map/features_fetcher.cpp +++ b/map/features_fetcher.cpp @@ -78,17 +78,8 @@ m2::RectD FeaturesFetcher::GetWorldRect() const return m_rect; } -void FeaturesFetcher::OnMapUpdated(platform::LocalCountryFile const & newFile, - platform::LocalCountryFile const & oldFile) -{ - if (m_onMapDeregistered) - m_onMapDeregistered(oldFile); -} - void FeaturesFetcher::OnMapDeregistered(platform::LocalCountryFile const & localFile) { if (m_onMapDeregistered) - { m_onMapDeregistered(localFile); - } } diff --git a/map/features_fetcher.hpp b/map/features_fetcher.hpp index 6596816931..dac7ff36ea 100644 --- a/map/features_fetcher.hpp +++ b/map/features_fetcher.hpp @@ -75,8 +75,6 @@ public: m2::RectD GetWorldRect() const; // MwmSet::Observer overrides: - void OnMapUpdated(platform::LocalCountryFile const & newFile, - platform::LocalCountryFile const & oldFile) override; void OnMapDeregistered(platform::LocalCountryFile const & localFile) override; private: diff --git a/storage/diff_scheme/diff_manager.cpp b/storage/diff_scheme/diff_manager.cpp index d11fcd792a..0a36a2b1de 100644 --- a/storage/diff_scheme/diff_manager.cpp +++ b/storage/diff_scheme/diff_manager.cpp @@ -95,7 +95,7 @@ void Manager::ApplyDiff(ApplyDiffParams && p, base::Cancellable const & cancella result = DiffApplicationResult::Failed; } - base::DeleteFileX(diffApplyingInProgressPath); + Platform::RemoveFileIfExists(diffApplyingInProgressPath); if (result != DiffApplicationResult::Ok) Platform::RemoveFileIfExists(newMwmPath); diff --git a/storage/storage.cpp b/storage/storage.cpp index 94e081a1ed..12d4949260 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -284,6 +284,7 @@ void Storage::RegisterAllLocalMaps(bool enableDiffs) LocalCountryFile & localFile = *j; LOG(LINFO, ("Removing obsolete", localFile)); localFile.SyncWithDisk(); + DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting); DeleteFromDiskWithIndexes(localFile, MapOptions::Diff); ++j; @@ -927,8 +928,11 @@ void Storage::RegisterDownloadedFiles(CountryId const & countryId, MapOptions op { CHECK_THREAD_CHECKER(m_threadChecker, ()); - auto const fn = [this, countryId](bool isSuccess) { + auto const fn = [this, countryId, options](bool isSuccess) { CHECK_THREAD_CHECKER(m_threadChecker, ()); + + LOG(LINFO, ("Registering downloaded file:", countryId, options, "; success:", isSuccess)); + if (!isSuccess) { OnMapDownloadFailed(countryId); @@ -1540,6 +1544,8 @@ void Storage::LoadDiffScheme() void Storage::ApplyDiff(CountryId const & countryId, function const & fn) { + LOG(LINFO, ("Applying diff for", countryId)); + m_diffsCancellable.Reset(); auto const diffLocalFile = PreparePlaceForCountryFiles(GetCurrentDataVersion(), m_dataDir, GetCountryFile(countryId)); @@ -1577,6 +1583,8 @@ void Storage::ApplyDiff(CountryId const & countryId, function