From 7f8752a216d289e82d4bb269899a707dcbffd3e8 Mon Sep 17 00:00:00 2001 From: eggfly Date: Sun, 4 Jul 2021 17:44:39 +0800 Subject: [PATCH 01/24] Implementation of two or more threads merging For both lightweight multiple engines and standalone multiple engines. --- fml/BUILD.gn | 2 + fml/memory/task_runner_checker.cc | 13 +- fml/memory/task_runner_checker.h | 2 +- fml/memory/task_runner_checker_unittest.cc | 2 +- fml/message_loop_task_queues.cc | 128 +++++----- fml/message_loop_task_queues.h | 7 +- ...oop_task_queues_merge_unmerge_unittests.cc | 41 +++- fml/raster_thread_merger.cc | 65 +++-- fml/raster_thread_merger.h | 16 +- fml/raster_thread_merger_unittests.cc | 227 +++++++++++++++++- fml/shared_thread_merger_impl.cc | 107 +++++++++ fml/shared_thread_merger_impl.h | 59 +++++ fml/task_source.h | 6 +- shell/common/rasterizer.cc | 14 +- shell/common/rasterizer.h | 10 - shell/common/shell.cc | 12 - .../external_view_embedder.cc | 6 + testing/run_tests.py | 2 +- 18 files changed, 565 insertions(+), 154 deletions(-) create mode 100644 fml/shared_thread_merger_impl.cc create mode 100644 fml/shared_thread_merger_impl.h diff --git a/fml/BUILD.gn b/fml/BUILD.gn index 3087b02b198aa..35a6cb8fbf7eb 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -58,6 +58,8 @@ source_set("fml") { "raster_thread_merger.cc", "raster_thread_merger.h", "size.h", + "shared_thread_merger_impl.cc", + "shared_thread_merger_impl.h", "synchronization/atomic_object.h", "synchronization/count_down_latch.cc", "synchronization/count_down_latch.h", diff --git a/fml/memory/task_runner_checker.cc b/fml/memory/task_runner_checker.cc index 6390472234602..53f3245ec004f 100644 --- a/fml/memory/task_runner_checker.cc +++ b/fml/memory/task_runner_checker.cc @@ -8,7 +8,7 @@ namespace fml { TaskRunnerChecker::TaskRunnerChecker() : initialized_queue_id_(InitTaskQueueId()), - subsumed_queue_id_( + subsumed_queue_ids_( MessageLoopTaskQueues::GetInstance()->GetSubsumedTaskQueueId( initialized_queue_id_)){}; @@ -17,8 +17,15 @@ TaskRunnerChecker::~TaskRunnerChecker() = default; bool TaskRunnerChecker::RunsOnCreationTaskRunner() const { FML_CHECK(fml::MessageLoop::IsInitializedForCurrentThread()); const auto current_queue_id = MessageLoop::GetCurrentTaskQueueId(); - return RunsOnTheSameThread(current_queue_id, initialized_queue_id_) || - RunsOnTheSameThread(current_queue_id, subsumed_queue_id_); + if (RunsOnTheSameThread(current_queue_id, initialized_queue_id_)) { + return true; + } + for (auto &subsumed: subsumed_queue_ids_) { + if (RunsOnTheSameThread(current_queue_id, subsumed)) { + return true; + } + } + return false; }; bool TaskRunnerChecker::RunsOnTheSameThread(TaskQueueId queue_a, diff --git a/fml/memory/task_runner_checker.h b/fml/memory/task_runner_checker.h index 4732452408801..42dbd60cf411d 100644 --- a/fml/memory/task_runner_checker.h +++ b/fml/memory/task_runner_checker.h @@ -22,7 +22,7 @@ class TaskRunnerChecker final { private: TaskQueueId initialized_queue_id_; - TaskQueueId subsumed_queue_id_; + std::set subsumed_queue_ids_; TaskQueueId InitTaskQueueId(); }; diff --git a/fml/memory/task_runner_checker_unittest.cc b/fml/memory/task_runner_checker_unittest.cc index 7da0dc93c0e04..e265d57fca535 100644 --- a/fml/memory/task_runner_checker_unittest.cc +++ b/fml/memory/task_runner_checker_unittest.cc @@ -152,7 +152,7 @@ TEST(TaskRunnerCheckerTests, }); latch3.Wait(); - fml::MessageLoopTaskQueues::GetInstance()->Unmerge(qid1); + fml::MessageLoopTaskQueues::GetInstance()->Unmerge(qid1, qid2); fml::AutoResetWaitableEvent latch4; loop2->GetTaskRunner()->PostTask([&]() { diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index a2b4b600d26eb..300c0ae365290 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -41,8 +41,7 @@ FML_THREAD_LOCAL ThreadLocalUniquePtr tls_task_source_grade; TaskQueueEntry::TaskQueueEntry(TaskQueueId created_for_arg) - : owner_of(_kUnmerged), - subsumed_by(_kUnmerged), + : subsumed_by(_kUnmerged), created_for(created_for_arg) { wakeable = NULL; task_observers = TaskObservers(); @@ -76,9 +75,9 @@ void MessageLoopTaskQueues::Dispose(TaskQueueId queue_id) { std::lock_guard guard(queue_mutex_); const auto& queue_entry = queue_entries_.at(queue_id); FML_DCHECK(queue_entry->subsumed_by == _kUnmerged); - TaskQueueId subsumed = queue_entry->owner_of; + auto &subsumed_set = queue_entry->owner_of; queue_entries_.erase(queue_id); - if (subsumed != _kUnmerged) { + for (auto &subsumed :subsumed_set) { queue_entries_.erase(subsumed); } } @@ -87,9 +86,9 @@ void MessageLoopTaskQueues::DisposeTasks(TaskQueueId queue_id) { std::lock_guard guard(queue_mutex_); const auto& queue_entry = queue_entries_.at(queue_id); FML_DCHECK(queue_entry->subsumed_by == _kUnmerged); - TaskQueueId subsumed = queue_entry->owner_of; + auto &subsumed_set = queue_entry->owner_of; queue_entry->task_source->ShutDown(); - if (subsumed != _kUnmerged) { + for (auto &subsumed :subsumed_set) { queue_entries_.at(subsumed)->task_source->ShutDown(); } } @@ -170,9 +169,9 @@ size_t MessageLoopTaskQueues::GetNumPendingTasks(TaskQueueId queue_id) const { size_t total_tasks = 0; total_tasks += queue_entry->task_source->GetNumPendingTasks(); - TaskQueueId subsumed = queue_entry->owner_of; - if (subsumed != _kUnmerged) { - const auto& subsumed_entry = queue_entries_.at(subsumed); + auto &subsumed_set = queue_entry->owner_of; + for (auto &subsumed :subsumed_set) { + const auto &subsumed_entry = queue_entries_.at(subsumed); total_tasks += subsumed_entry->task_source->GetNumPendingTasks(); } return total_tasks; @@ -205,8 +204,8 @@ std::vector MessageLoopTaskQueues::GetObserversToNotify( observers.push_back(observer.second); } - TaskQueueId subsumed = queue_entries_.at(queue_id)->owner_of; - if (subsumed != _kUnmerged) { + auto& subsumed_set = queue_entries_.at(queue_id)->owner_of; + for (auto &subsumed :subsumed_set) { for (const auto& observer : queue_entries_.at(subsumed)->task_observers) { observers.push_back(observer.second); } @@ -230,22 +229,26 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { std::lock_guard guard(queue_mutex_); auto& owner_entry = queue_entries_.at(owner); auto& subsumed_entry = queue_entries_.at(subsumed); - - if (owner_entry->owner_of == subsumed) { + auto& subsumed_set = owner_entry->owner_of; + if (subsumed_set.find(subsumed) != subsumed_set.end()) { return true; } - std::vector owner_subsumed_keys = { - owner_entry->owner_of, owner_entry->subsumed_by, subsumed_entry->owner_of, - subsumed_entry->subsumed_by}; - - for (auto key : owner_subsumed_keys) { - if (key != _kUnmerged) { - return false; - } + // Only don't check owner_entry->owner_of, it may contains items when merged with other different queues. + if (owner_entry->subsumed_by != _kUnmerged) { + FML_LOG(WARNING) << "Thread merging failed: owner_entry was already subsumed by others."; + return false; } - - owner_entry->owner_of = subsumed; + if (!subsumed_entry->owner_of.empty()) { + FML_LOG(WARNING) << "Thread merging failed: subsumed_entry already owns others."; + return false; + } + if (subsumed_entry->subsumed_by != _kUnmerged) { + FML_LOG(WARNING) << "Thread merging failed: subsumed_entry was already subsumed by others."; + return false; + } + // All checking is OK, set merged state. + owner_entry->owner_of.insert(subsumed); subsumed_entry->subsumed_by = owner; if (HasPendingTasksUnlocked(owner)) { @@ -255,16 +258,26 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { return true; } -bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner) { +bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner, TaskQueueId subsumed) { std::lock_guard guard(queue_mutex_); - const auto& owner_entry = queue_entries_.at(owner); - const TaskQueueId subsumed = owner_entry->owner_of; - if (subsumed == _kUnmerged) { + const auto &owner_entry = queue_entries_.at(owner); + if (owner_entry->owner_of.empty()) { + FML_LOG(WARNING) << "Thread unmerging failed: owner_entry doesn't own anyone."; + return false; + } + + if (owner_entry->subsumed_by != _kUnmerged) { + FML_LOG(WARNING) << "Thread unmerging failed: owner_entry was subsumed by others."; + return false; + } + + if (queue_entries_.at(subsumed)->subsumed_by == _kUnmerged) { + FML_LOG(WARNING) << "Thread unmerging failed: subsumed_entry wasn't subsumed by others."; return false; } queue_entries_.at(subsumed)->subsumed_by = _kUnmerged; - owner_entry->owner_of = _kUnmerged; + owner_entry->owner_of.erase(subsumed); if (HasPendingTasksUnlocked(owner)) { WakeUpUnlocked(owner, GetNextWakeTimeUnlocked(owner)); @@ -280,11 +293,14 @@ bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner) { bool MessageLoopTaskQueues::Owns(TaskQueueId owner, TaskQueueId subsumed) const { std::lock_guard guard(queue_mutex_); - return owner != _kUnmerged && subsumed != _kUnmerged && - subsumed == queue_entries_.at(owner)->owner_of; + if (owner == _kUnmerged || subsumed == _kUnmerged) { + return false; + } + auto &subsumed_set = queue_entries_.at(owner)->owner_of; + return subsumed_set.find(subsumed) != subsumed_set.end(); } -TaskQueueId MessageLoopTaskQueues::GetSubsumedTaskQueueId( +std::set MessageLoopTaskQueues::GetSubsumedTaskQueueId( TaskQueueId owner) const { std::lock_guard guard(queue_mutex_); return queue_entries_.at(owner)->owner_of; @@ -318,13 +334,13 @@ bool MessageLoopTaskQueues::HasPendingTasksUnlocked( return true; } - const TaskQueueId subsumed = entry->owner_of; - if (subsumed == _kUnmerged) { - // this is not an owner and queue is empty. - return false; - } else { - return !queue_entries_.at(subsumed)->task_source->IsEmpty(); + auto &subsumed_set = entry->owner_of; + for (auto &subsumed: subsumed_set) { + if (!queue_entries_.at(subsumed)->task_source->IsEmpty()) { + return true; + } } + return false; } fml::TimePoint MessageLoopTaskQueues::GetNextWakeTimeUnlocked( @@ -335,33 +351,27 @@ fml::TimePoint MessageLoopTaskQueues::GetNextWakeTimeUnlocked( TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( TaskQueueId owner) const { FML_DCHECK(HasPendingTasksUnlocked(owner)); - const auto& entry = queue_entries_.at(owner); - const TaskQueueId subsumed = entry->owner_of; - if (subsumed == _kUnmerged) { + const auto &entry = queue_entries_.at(owner); + if (entry->owner_of.empty()) { return entry->task_source->Top(); } - TaskSource* owner_tasks = entry->task_source.get(); - TaskSource* subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); - - // we are owning another task queue - const bool subsumed_has_task = !subsumed_tasks->IsEmpty(); - const bool owner_has_task = !owner_tasks->IsEmpty(); - fml::TaskQueueId top_queue_id = owner; - if (owner_has_task && subsumed_has_task) { - const auto owner_task = owner_tasks->Top(); - const auto subsumed_task = subsumed_tasks->Top(); - if (owner_task.task > subsumed_task.task) { - top_queue_id = subsumed; - } else { - top_queue_id = owner; + TaskSource *owner_tasks = entry->task_source.get(); + std::vector candidate_top_tasks; + + if (!owner_tasks->IsEmpty()) { + candidate_top_tasks.push_back(owner_tasks->Top()); + } + for (TaskQueueId subsumed: entry->owner_of) { + TaskSource *subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); + if (!subsumed_tasks->IsEmpty()) { + candidate_top_tasks.push_back(subsumed_tasks->Top()); } - } else if (owner_has_task) { - top_queue_id = owner; - } else { - top_queue_id = subsumed; } - return queue_entries_.at(top_queue_id)->task_source->Top(); + // At least one task at the top because PeekNextTaskUnlocked() is called after HasPendingTasksUnlocked() + FML_CHECK(!candidate_top_tasks.empty()); + TaskSource::TopTask &top = *std::min_element(candidate_top_tasks.begin(), candidate_top_tasks.end()); + return top; } } // namespace fml diff --git a/fml/message_loop_task_queues.h b/fml/message_loop_task_queues.h index da3362f1c3792..f56d48cc43583 100644 --- a/fml/message_loop_task_queues.h +++ b/fml/message_loop_task_queues.h @@ -7,6 +7,7 @@ #include #include +#include #include #include "flutter/fml/closure.h" @@ -38,7 +39,7 @@ class TaskQueueEntry { // this queue has not been merged or subsumed. OR exactly one // of these will be _kUnmerged, if owner_of is _kUnmerged, it means // that the queue has been subsumed or else it owns another queue. - TaskQueueId owner_of; + std::set owner_of; TaskQueueId subsumed_by; TaskQueueId created_for; @@ -119,14 +120,14 @@ class MessageLoopTaskQueues bool Merge(TaskQueueId owner, TaskQueueId subsumed); // Will return false if the owner has not been merged before. - bool Unmerge(TaskQueueId owner); + bool Unmerge(TaskQueueId owner, TaskQueueId subsumed); /// Returns \p true if \p owner owns the \p subsumed task queue. bool Owns(TaskQueueId owner, TaskQueueId subsumed) const; // Returns the subsumed task queue if any or |TaskQueueId::kUnmerged| // otherwise. - TaskQueueId GetSubsumedTaskQueueId(TaskQueueId owner) const; + std::set GetSubsumedTaskQueueId(TaskQueueId owner) const; void PauseSecondarySource(TaskQueueId queue_id); diff --git a/fml/message_loop_task_queues_merge_unmerge_unittests.cc b/fml/message_loop_task_queues_merge_unmerge_unittests.cc index 0c928eba41157..a2705bac058ab 100644 --- a/fml/message_loop_task_queues_merge_unmerge_unittests.cc +++ b/fml/message_loop_task_queues_merge_unmerge_unittests.cc @@ -99,13 +99,27 @@ TEST(MessageLoopTaskQueueMergeUnmerge, MergeUnmergeTasksPreserved) { ASSERT_EQ(2u, task_queue->GetNumPendingTasks(queue_id_1)); ASSERT_EQ(0u, task_queue->GetNumPendingTasks(queue_id_2)); - task_queue->Unmerge(queue_id_1); + task_queue->Unmerge(queue_id_1, queue_id_2); ASSERT_EQ(1u, task_queue->GetNumPendingTasks(queue_id_1)); ASSERT_EQ(1u, task_queue->GetNumPendingTasks(queue_id_2)); } -TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadyMergedOrSubsumed) { +/// Multiple standalone engines scene +TEST(MessageLoopTaskQueueMergeUnmerge, OneCanOwnMultipleQueues) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + + auto queue_id_1 = task_queue->CreateTaskQueue(); + auto queue_id_2 = task_queue->CreateTaskQueue(); + auto queue_id_3 = task_queue->CreateTaskQueue(); + auto queue_id_4 = task_queue->CreateTaskQueue(); + + ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_2)); + ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_3)); + ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_4)); +} + +TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadySubsumed) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto queue_id_1 = task_queue->CreateTaskQueue(); @@ -114,19 +128,34 @@ TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadyMergedOrSubsumed) { task_queue->Merge(queue_id_1, queue_id_2); - ASSERT_FALSE(task_queue->Merge(queue_id_1, queue_id_3)); ASSERT_FALSE(task_queue->Merge(queue_id_2, queue_id_3)); + ASSERT_FALSE(task_queue->Merge(queue_id_2, queue_id_1)); } -TEST(MessageLoopTaskQueueMergeUnmerge, UnmergeFailsOnSubsumed) { +TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadyOwnsButTryToBeSubsumed) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto queue_id_1 = task_queue->CreateTaskQueue(); auto queue_id_2 = task_queue->CreateTaskQueue(); + auto queue_id_3 = task_queue->CreateTaskQueue(); task_queue->Merge(queue_id_1, queue_id_2); + // A recursively linked merging will fail + ASSERT_FALSE(task_queue->Merge(queue_id_3, queue_id_1)); +} + +TEST(MessageLoopTaskQueueMergeUnmerge, UnmergeFailsOnSubsumedOrNeverMerged) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + + auto queue_id_1 = task_queue->CreateTaskQueue(); + auto queue_id_2 = task_queue->CreateTaskQueue(); + auto queue_id_3 = task_queue->CreateTaskQueue(); - ASSERT_FALSE(task_queue->Unmerge(queue_id_2)); + task_queue->Merge(queue_id_1, queue_id_2); + ASSERT_FALSE(task_queue->Unmerge(queue_id_2, queue_id_3)); + ASSERT_FALSE(task_queue->Unmerge(queue_id_1, queue_id_3)); + ASSERT_FALSE(task_queue->Unmerge(queue_id_3, queue_id_1)); + ASSERT_FALSE(task_queue->Unmerge(queue_id_2, queue_id_1)); } TEST(MessageLoopTaskQueueMergeUnmerge, MergeInvokesBothWakeables) { @@ -176,7 +205,7 @@ TEST(MessageLoopTaskQueueMergeUnmerge, queue_id_2, []() {}, fml::TimePoint::Now()); task_queue->Merge(queue_id_1, queue_id_2); - task_queue->Unmerge(queue_id_1); + task_queue->Unmerge(queue_id_1, queue_id_2); CountRemainingTasks(task_queue, queue_id_1); diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index cfeeea3dcb527..7404ffb1c7c94 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -10,16 +10,13 @@ namespace fml { -const int RasterThreadMerger::kLeaseNotSet = -1; - RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - task_queues_(fml::MessageLoopTaskQueues::GetInstance()), - lease_term_(kLeaseNotSet), + shared_merger_impl_(fml::SharedThreadMergerImpl::GetSharedImpl(platform_queue_id, gpu_queue_id)), enabled_(true) { - FML_CHECK(!task_queues_->Owns(platform_queue_id_, gpu_queue_id_)); + FML_LOG(ERROR) << "shared_merger_impl_=" << shared_merger_impl_.get(); } void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { @@ -27,7 +24,7 @@ void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { } void RasterThreadMerger::MergeWithLease(size_t lease_term) { - std::scoped_lock lock(lease_term_mutex_); + std::scoped_lock lock(mutex_); if (TaskQueuesAreSame()) { return; } @@ -41,30 +38,27 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { return; } - bool success = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); + bool success = shared_merger_impl_->MergeWithLease(lease_term); if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } - FML_CHECK(success) << "Unable to merge the raster and platform threads."; - lease_term_ = lease_term; merged_condition_.notify_one(); } -void RasterThreadMerger::UnMergeNow() { - std::scoped_lock lock(lease_term_mutex_); +void RasterThreadMerger::UnMergeNowIfLastOne() { + std::scoped_lock lock(mutex_); + if (TaskQueuesAreSame()) { return; } if (!IsEnabledUnSafe()) { return; } - lease_term_ = 0; - bool success = task_queues_->Unmerge(platform_queue_id_); + bool success = shared_merger_impl_->UnMergeNowIfLastOne(this); if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } - FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; } bool RasterThreadMerger::IsOnPlatformThread() const { @@ -80,34 +74,32 @@ bool RasterThreadMerger::IsOnRasterizingThread() const { } void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { + FML_DCHECK(lease_term > 0) << "lease_term should be positive."; if (TaskQueuesAreSame()) { return; } - std::scoped_lock lock(lease_term_mutex_); - FML_DCHECK(IsMergedUnSafe()) << "lease_term should be positive."; - if (lease_term_ != kLeaseNotSet && - static_cast(lease_term) > lease_term_) { - lease_term_ = lease_term; - } + std::scoped_lock lock(mutex_); + shared_merger_impl_->ExtendLeaseTo(lease_term); } bool RasterThreadMerger::IsMerged() { - std::scoped_lock lock(lease_term_mutex_); - return IsMergedUnSafe(); + std::scoped_lock lock(mutex_); + bool is_merged = IsMergedUnSafe(); + return is_merged; } void RasterThreadMerger::Enable() { - std::scoped_lock lock(lease_term_mutex_); + std::scoped_lock lock(mutex_); enabled_ = true; } void RasterThreadMerger::Disable() { - std::scoped_lock lock(lease_term_mutex_); + std::scoped_lock lock(mutex_); enabled_ = false; } bool RasterThreadMerger::IsEnabled() { - std::scoped_lock lock(lease_term_mutex_); + std::scoped_lock lock(mutex_); return IsEnabledUnSafe(); } @@ -116,7 +108,7 @@ bool RasterThreadMerger::IsEnabledUnSafe() const { } bool RasterThreadMerger::IsMergedUnSafe() const { - return lease_term_ > 0 || TaskQueuesAreSame(); + return TaskQueuesAreSame() || shared_merger_impl_->IsMergedUnSafe(); } bool RasterThreadMerger::TaskQueuesAreSame() const { @@ -128,7 +120,7 @@ void RasterThreadMerger::WaitUntilMerged() { return; } FML_CHECK(IsOnPlatformThread()); - std::unique_lock lock(lease_term_mutex_); + std::unique_lock lock(mutex_); merged_condition_.wait(lock, [&] { return IsMergedUnSafe(); }); } @@ -136,24 +128,27 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { if (TaskQueuesAreSame()) { return RasterThreadStatus::kRemainsMerged; } - std::unique_lock lock(lease_term_mutex_); + std::scoped_lock lock(mutex_); if (!IsMergedUnSafe()) { return RasterThreadStatus::kRemainsUnmerged; } if (!IsEnabledUnSafe()) { return RasterThreadStatus::kRemainsMerged; } - FML_DCHECK(lease_term_ > 0) - << "lease_term should always be positive when merged."; - lease_term_--; - if (lease_term_ == 0) { - // |UnMergeNow| is going to acquire the lock again. - lock.unlock(); - UnMergeNow(); + bool unmerged_after_decrement = shared_merger_impl_->DecrementLease(); + if (unmerged_after_decrement) { + if (merge_unmerge_callback_ != nullptr) { + merge_unmerge_callback_(); + } return RasterThreadStatus::kUnmergedNow; } return RasterThreadStatus::kRemainsMerged; } +void RasterThreadMerger::RecordMergeCaller() { + std::scoped_lock lock(mutex_); + shared_merger_impl_->RecordMergerCaller(this); +} + } // namespace fml diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 9e38b23a555ee..cc02b0f2fe308 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -11,6 +11,7 @@ #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" #include "flutter/fml/message_loop_task_queues.h" +#include "flutter/fml/shared_thread_merger_impl.h" namespace fml { @@ -43,7 +44,7 @@ class RasterThreadMerger // If the task queues are the same, we consider them statically merged. // When task queues are statically merged, we never unmerge them and // this method becomes no-op. - void UnMergeNow(); + void UnMergeNowIfLastOne(); // If the task queues are the same, we consider them statically merged. // When task queues are statically merged this method becomes no-op. @@ -56,6 +57,9 @@ class RasterThreadMerger // When task queues are statically merged this method becomes no-op. RasterThreadStatus DecrementLease(); + void RecordMergeCaller(); + + // TODO DOC bool IsMerged(); // Waits until the threads are merged. @@ -78,11 +82,11 @@ class RasterThreadMerger void Enable(); // Disables the thread merger. Once disabled, any call to - // |MergeWithLease| or |UnMergeNow| results in a noop. + // |MergeWithLease| or |UnMergeNowIfLastOne| results in a noop. void Disable(); // Whether the thread merger is enabled. By default, the thread merger is - // enabled. If false, calls to |MergeWithLease| or |UnMergeNow| results in a + // enabled. If false, calls to |MergeWithLease| or |UnMergeNowIfLastOne| results in a // noop. bool IsEnabled(); @@ -94,13 +98,11 @@ class RasterThreadMerger void SetMergeUnmergeCallback(const fml::closure& callback); private: - static const int kLeaseNotSet; fml::TaskQueueId platform_queue_id_; fml::TaskQueueId gpu_queue_id_; - fml::RefPtr task_queues_; - std::atomic_int lease_term_; + fml::RefPtr shared_merger_impl_; std::condition_variable merged_condition_; - std::mutex lease_term_mutex_; + std::mutex mutex_; fml::closure merge_unmerge_callback_; bool enabled_; diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 5c7b6fd4bfffc..dd624378de24d 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -396,7 +396,7 @@ TEST(RasterThreadMerger, Disable) { ASSERT_TRUE(raster_thread_merger_->IsMerged()); raster_thread_merger_->Disable(); - raster_thread_merger_->UnMergeNow(); + raster_thread_merger_->UnMergeNowIfLastOne(); ASSERT_TRUE(raster_thread_merger_->IsMerged()); { @@ -407,7 +407,7 @@ TEST(RasterThreadMerger, Disable) { ASSERT_TRUE(raster_thread_merger_->IsMerged()); raster_thread_merger_->Enable(); - raster_thread_merger_->UnMergeNow(); + raster_thread_merger_->UnMergeNowIfLastOne(); ASSERT_FALSE(raster_thread_merger_->IsMerged()); raster_thread_merger_->MergeWithLease(1); @@ -623,5 +623,228 @@ TEST(RasterThreadMerger, SetMergeUnmergeCallback) { thread2.join(); } +TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { + fml::MessageLoop* loop1 = nullptr; + fml::AutoResetWaitableEvent latch1; + fml::AutoResetWaitableEvent term1; + std::thread thread1([&loop1, &latch1, &term1]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop1 = &fml::MessageLoop::GetCurrent(); + latch1.Signal(); + term1.Wait(); + }); + + fml::MessageLoop* loop2 = nullptr; + fml::AutoResetWaitableEvent latch2; + fml::AutoResetWaitableEvent term2; + std::thread thread2([&loop2, &latch2, &term2]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop2 = &fml::MessageLoop::GetCurrent(); + latch2.Signal(); + term2.Wait(); + }); + + latch1.Wait(); + latch2.Wait(); + + fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + const auto raster_thread_merger1_ = + fml::MakeRefCounted(qid1, qid2); + const auto raster_thread_merger2_ = + fml::MakeRefCounted(qid1, qid2); + const int kNumFramesMerged = 5; + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + // Merge using the second merger + raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + + // let there be one more turn till the leases expire. + for (int i = 0; i < kNumFramesMerged - 1; i++) { + // Check merge state using the first merger + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + raster_thread_merger1_->DecrementLease(); + } + + // extend the lease once using the first merger + raster_thread_merger1_->ExtendLeaseTo(kNumFramesMerged); + + // we will NOT last for 1 extra turn, we just set it. + for (int i = 0; i < kNumFramesMerged; i++) { + // Check merge state using the second merger + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + raster_thread_merger2_->DecrementLease(); + } + + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + term1.Signal(); + term2.Signal(); + thread1.join(); + thread2.join(); +} + +TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { + fml::MessageLoop* loop1 = nullptr; + fml::AutoResetWaitableEvent latch1; + fml::AutoResetWaitableEvent term1; + std::thread thread1([&loop1, &latch1, &term1]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop1 = &fml::MessageLoop::GetCurrent(); + latch1.Signal(); + term1.Wait(); + }); + + fml::MessageLoop* loop2 = nullptr; + fml::AutoResetWaitableEvent latch2; + fml::AutoResetWaitableEvent term2; + std::thread thread2([&loop2, &latch2, &term2]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop2 = &fml::MessageLoop::GetCurrent(); + latch2.Signal(); + term2.Wait(); + }); + + latch1.Wait(); + latch2.Wait(); + + fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + const auto raster_thread_merger1_ = + fml::MakeRefCounted(qid1, qid2); + const auto raster_thread_merger2_ = + fml::MakeRefCounted(qid1, qid2); + const int kNumFramesMerged = 5; + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + // Recording mergers themselves is needed. + raster_thread_merger1_->RecordMergeCaller(); + raster_thread_merger2_->RecordMergeCaller(); + // Merge using the mergers + raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + + // Two callers state becomes one caller left. + raster_thread_merger1_->UnMergeNowIfLastOne(); + + // Check if still merged + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + + // One caller state becomes no callers left. + raster_thread_merger2_->UnMergeNowIfLastOne(); + + // Check if unmerged + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + term1.Signal(); + term2.Signal(); + thread1.join(); + thread2.join(); +} + +TEST(RasterThreadMerger, MultipleMergersCanIndependentlyMergeTwoOrMoreThreadsIntoOne) { + fml::MessageLoop* loop1 = nullptr; + fml::AutoResetWaitableEvent latch1; + fml::AutoResetWaitableEvent term1; + std::thread thread1([&loop1, &latch1, &term1]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop1 = &fml::MessageLoop::GetCurrent(); + latch1.Signal(); + term1.Wait(); + }); + + fml::MessageLoop* loop2 = nullptr; + fml::AutoResetWaitableEvent latch2; + fml::AutoResetWaitableEvent term2; + std::thread thread2([&loop2, &latch2, &term2]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop2 = &fml::MessageLoop::GetCurrent(); + latch2.Signal(); + term2.Wait(); + }); + + fml::MessageLoop* loop3 = nullptr; + fml::AutoResetWaitableEvent latch3; + fml::AutoResetWaitableEvent term3; + std::thread thread3([&loop3, &latch3, &term3]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop3 = &fml::MessageLoop::GetCurrent(); + latch3.Signal(); + term3.Wait(); + }); + + latch1.Wait(); + latch2.Wait(); + latch3.Wait(); + + fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid3 = loop3->GetTaskRunner()->GetTaskQueueId(); + + const auto raster_thread_merger1_ = + fml::MakeRefCounted(qid1, qid2); + const auto raster_thread_merger2_ = + fml::MakeRefCounted(qid1, qid3); + const int kNumFramesMerged = 5; + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + // Merge thread2 into thread1 + raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + // Merge thread3 into thread1 + raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + + for (int i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + raster_thread_merger1_->DecrementLease(); + } + + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + + for (int i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + raster_thread_merger2_->DecrementLease(); + } + + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + // test caller records are separated below + raster_thread_merger1_->RecordMergeCaller(); + raster_thread_merger2_->RecordMergeCaller(); + + raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + + // Can unmerge independently + raster_thread_merger1_->UnMergeNowIfLastOne(); + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + + // Can unmerge independently + raster_thread_merger2_->UnMergeNowIfLastOne(); + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + + term1.Signal(); + term2.Signal(); + term3.Signal(); + thread1.join(); + thread2.join(); + thread3.join(); +} + } // namespace testing } // namespace fml diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger_impl.cc new file mode 100644 index 0000000000000..25d64aa2d1a18 --- /dev/null +++ b/fml/shared_thread_merger_impl.cc @@ -0,0 +1,107 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#define FML_USED_ON_EMBEDDER + +#include "flutter/fml/shared_thread_merger_impl.h" + +#include "flutter/fml/message_loop_impl.h" +#include + +namespace fml { + +std::mutex SharedThreadMergerImpl::creation_mutex_; + +// Guarded by creation_mutex_ +std::map> SharedThreadMergerImpl::shared_merger_instances_; + +const int SharedThreadMergerImpl::kLeaseNotSet = -1; + +fml::RefPtr SharedThreadMergerImpl::GetSharedImpl(fml::TaskQueueId owner, + fml::TaskQueueId subsumed) { + std::scoped_lock creation(creation_mutex_); + ThreadMergerKey key = {.owner = owner, .subsumed = subsumed}; + + if (shared_merger_instances_.find(key) != shared_merger_instances_.end()) { + return shared_merger_instances_[key]; + } + auto merger = fml::MakeRefCounted(owner, subsumed); + shared_merger_instances_[key] = merger; + return merger; +} + +void SharedThreadMergerImpl::RecordMergerCaller(RasterThreadMerger *caller) { + std::scoped_lock lock(mutex_); + // Record current merge caller into callers set. + merge_callers_.insert(caller); +} + +bool SharedThreadMergerImpl::MergeWithLease(size_t lease_term) { + std::scoped_lock lock(mutex_); + if (IsMergedUnSafe()) { + return true; + } + bool success = task_queues_->Merge(owner_, subsumed_); + FML_CHECK(success) << "Unable to merge the raster and platform threads."; + + lease_term_ = lease_term; + return success; +} + +bool SharedThreadMergerImpl::UnMergeNowUnSafe() { + FML_CHECK(lease_term_ == 0) << "lease_term_ must be 0 state before calling UnMergeNowUnSafe()"; + bool success = task_queues_->Unmerge(owner_, subsumed_); + FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; + return success; +} + +bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger *caller) { + std::scoped_lock lock(mutex_); + + merge_callers_.erase(caller); + // 如果我是最后一个caller，那么不管lease_term_，立马Unmerge + // 如果我不是最后一个caller，那么需要等lease_term_通过DecrementLease()降低到0来Unmerge + if (!merge_callers_.empty()) { + return true; + } + FML_CHECK(IsMergedUnSafe()) << "UnMergeNowIfLastOne() must be called only when thread are merged"; + lease_term_ = 0; // mark need unmerge now + return UnMergeNowUnSafe(); +} + + +bool SharedThreadMergerImpl::DecrementLease() { + std::scoped_lock lock(mutex_); + FML_DCHECK(lease_term_ > 0) + << "lease_term should always be positive when merged."; + lease_term_--; + if (lease_term_ == 0) { + // Unmerge now because lease_term_ decreased to zero. + UnMergeNowUnSafe(); + return true; + } + return false; +} + +SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed) + : owner_(owner), + subsumed_(subsumed), + task_queues_(fml::MessageLoopTaskQueues::GetInstance()), + lease_term_(kLeaseNotSet) {} + +void SharedThreadMergerImpl::ExtendLeaseTo(size_t lease_term) { + std::scoped_lock lock(mutex_); + + FML_DCHECK(IsMergedUnSafe()) << "should be merged state when calling this method"; + if (lease_term_ != kLeaseNotSet && + static_cast(lease_term) > lease_term_) { + lease_term_ = lease_term; + } +} + +bool SharedThreadMergerImpl::IsMergedUnSafe() const { + return lease_term_ > 0; +} + +} // namespace fml diff --git a/fml/shared_thread_merger_impl.h b/fml/shared_thread_merger_impl.h new file mode 100644 index 0000000000000..06c74a01dcde3 --- /dev/null +++ b/fml/shared_thread_merger_impl.h @@ -0,0 +1,59 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_FML_SHARED_THREAD_MERGER_H_ +#define FLUTTER_FML_SHARED_THREAD_MERGER_H_ + +#include +#include + +#include "flutter/fml/macros.h" +#include "flutter/fml/memory/ref_counted.h" +#include "flutter/fml/message_loop_task_queues.h" + +namespace fml { + +class MessageLoopImpl; +class RasterThreadMerger; + +struct ThreadMergerKey { + TaskQueueId owner; + TaskQueueId subsumed; + bool operator<(const ThreadMergerKey &other) const { + if (owner == other.owner) { + return subsumed < other.subsumed; + } else { + return owner < other.owner; + } + } +}; + +class SharedThreadMergerImpl : public fml::RefCountedThreadSafe { + public: + SharedThreadMergerImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed); + static fml::RefPtr GetSharedImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed); + void RecordMergerCaller(RasterThreadMerger *caller); + bool MergeWithLease(size_t lease_term); + /// TODO DOC + bool UnMergeNowIfLastOne(RasterThreadMerger *caller); + void ExtendLeaseTo(size_t lease_term); + bool IsMergedUnSafe() const; + bool DecrementLease(); + private: + static const int kLeaseNotSet; + fml::TaskQueueId owner_; + fml::TaskQueueId subsumed_; + fml::RefPtr task_queues_; + std::atomic_int lease_term_; + std::mutex mutex_; + std::set merge_callers_; + static std::mutex creation_mutex_; + // Guarded by creation_mutex_ + static std::map> shared_merger_instances_; + bool UnMergeNowUnSafe(); +}; + +} // namespace fml + +#endif // FLUTTER_FML_SHARED_THREAD_MERGER_H_ diff --git a/fml/task_source.h b/fml/task_source.h index 64e151d9482ec..65763de59fd52 100644 --- a/fml/task_source.h +++ b/fml/task_source.h @@ -36,7 +36,11 @@ class TaskSource { public: struct TopTask { TaskQueueId task_queue_id; - const DelayedTask& task; + const DelayedTask &task; + /// The operator less function is for std::vector to get min_element + bool operator<(const TopTask &other) const { + return other.task > task; + } }; /// Construts a TaskSource with the given `task_queue_id`. diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 8a149bebd5b2f..8c8329177ecb1 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -94,7 +94,7 @@ void Rasterizer::Teardown() { if (raster_thread_merger_.get() != nullptr && raster_thread_merger_.get()->IsMerged()) { FML_DCHECK(raster_thread_merger_->IsEnabled()); - raster_thread_merger_->UnMergeNow(); + raster_thread_merger_->UnMergeNowIfLastOne(); // TODO?? raster_thread_merger_->SetMergeUnmergeCallback(nullptr); } } @@ -490,24 +490,12 @@ RasterStatus Rasterizer::DrawToSurface( frame->supports_readback(), // surface supports pixel reads raster_thread_merger_ // thread merger ); - if (compositor_frame) { RasterStatus raster_status = compositor_frame->Raster(layer_tree, false); if (raster_status == RasterStatus::kFailed || raster_status == RasterStatus::kSkipAndRetry) { return raster_status; } - if (shared_engine_block_thread_merging_ && raster_thread_merger_ && - raster_thread_merger_->IsMerged()) { - // TODO(73620): Remove when platform views are accounted for. - FML_LOG(ERROR) - << "Error: Thread merging not implemented for engines with shared " - "components.\n\n" - "This is likely a result of using platform views with enigne " - "groups. See " - "https://github.com/flutter/flutter/issues/73620."; - fml::KillProcess(); - } if (external_view_embedder_ && (!raster_thread_merger_ || raster_thread_merger_->IsMerged())) { FML_DCHECK(!frame->IsSubmitted()); diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index aa7334179e970..b91dc8c125350 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -422,15 +422,6 @@ class Rasterizer final : public SnapshotDelegate { /// void DisableThreadMergerIfNeeded(); - /// @brief Mechanism to stop thread merging when using shared engine - /// components. - /// @details This is a temporary workaround until thread merging can be - /// supported correctly. This should be called on the raster - /// thread. - /// @see https://github.com/flutter/flutter/issues/73620 - /// - void BlockThreadMerging() { shared_engine_block_thread_merging_ = true; } - private: Delegate& delegate_; std::unique_ptr surface_; @@ -447,7 +438,6 @@ class Rasterizer final : public SnapshotDelegate { fml::RefPtr raster_thread_merger_; fml::TaskRunnerAffineWeakPtrFactory weak_factory_; std::shared_ptr external_view_embedder_; - bool shared_engine_block_thread_merging_ = false; // |SnapshotDelegate| sk_sp MakeRasterSnapshot( diff --git a/shell/common/shell.cc b/shell/common/shell.cc index beea55e86736e..169ce325cf367 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -505,18 +505,6 @@ std::unique_ptr Shell::Spawn( .SetIfTrue([&] { result = shell_maker(true); })); result->shared_resource_context_ = io_manager_->GetSharedResourceContext(); result->RunEngine(std::move(run_configuration)); - - task_runners_.GetRasterTaskRunner()->PostTask( - [rasterizer = rasterizer_->GetWeakPtr(), - spawn_rasterizer = result->rasterizer_->GetWeakPtr()]() { - if (rasterizer) { - rasterizer->BlockThreadMerging(); - } - if (spawn_rasterizer) { - spawn_rasterizer->BlockThreadMerging(); - } - }); - return result; } diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc index 070e54fd70551..f89862de21f76 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -6,6 +6,8 @@ #include "flutter/fml/trace_event.h" #include "flutter/shell/platform/android/surface/android_surface.h" +#include + namespace flutter { @@ -210,12 +212,16 @@ AndroidExternalViewEmbedder::CreateSurfaceIfNeeded(GrDirectContext* context, return frame; } +// static std::set merged_merger_map; + // |ExternalViewEmbedder| PostPrerollResult AndroidExternalViewEmbedder::PostPrerollAction( fml::RefPtr raster_thread_merger) { if (!FrameHasPlatformLayers()) { return PostPrerollResult::kSuccess; } + // Record merger firstly when frame has platform layers + raster_thread_merger->RecordMergeCaller(); if (!raster_thread_merger->IsMerged()) { // The raster thread merger may be disabled if the rasterizer is being // created or teared down. diff --git a/testing/run_tests.py b/testing/run_tests.py index 9897881e7a2f9..8d0ea685003ed 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -165,7 +165,7 @@ def RunCCTests(build_dir, filter): RunEngineExecutable(build_dir, 'flow_unittests', filter, flow_flags + shuffle_flags) # TODO(44614): Re-enable after https://github.com/flutter/flutter/issues/44614 has been addressed. - # RunEngineExecutable(build_dir, 'fml_unittests', filter, [ fml_unittests_filter ] + shuffle_flags) + RunEngineExecutable(build_dir, 'fml_unittests', filter, [ fml_unittests_filter ] + shuffle_flags) RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) From 1d56764642c553f93391293fe239f437caea7ed4 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 15:15:27 +0800 Subject: [PATCH 02/24] Format code Signed-off-by: eggfly --- fml/memory/task_runner_checker.cc | 2 +- fml/message_loop_task_queues.cc | 62 +++++++++++-------- ...oop_task_queues_merge_unmerge_unittests.cc | 3 +- fml/raster_thread_merger.cc | 4 +- fml/raster_thread_merger.h | 4 +- fml/raster_thread_merger_unittests.cc | 3 +- fml/shared_thread_merger_impl.cc | 31 ++++++---- fml/shared_thread_merger_impl.h | 19 +++--- fml/task_source.h | 6 +- shell/common/rasterizer.cc | 2 +- .../external_view_embedder.cc | 3 +- 11 files changed, 79 insertions(+), 60 deletions(-) diff --git a/fml/memory/task_runner_checker.cc b/fml/memory/task_runner_checker.cc index 53f3245ec004f..5320c49153b4d 100644 --- a/fml/memory/task_runner_checker.cc +++ b/fml/memory/task_runner_checker.cc @@ -20,7 +20,7 @@ bool TaskRunnerChecker::RunsOnCreationTaskRunner() const { if (RunsOnTheSameThread(current_queue_id, initialized_queue_id_)) { return true; } - for (auto &subsumed: subsumed_queue_ids_) { + for (auto& subsumed : subsumed_queue_ids_) { if (RunsOnTheSameThread(current_queue_id, subsumed)) { return true; } diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 300c0ae365290..063e0551e5f7f 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -41,8 +41,7 @@ FML_THREAD_LOCAL ThreadLocalUniquePtr tls_task_source_grade; TaskQueueEntry::TaskQueueEntry(TaskQueueId created_for_arg) - : subsumed_by(_kUnmerged), - created_for(created_for_arg) { + : subsumed_by(_kUnmerged), created_for(created_for_arg) { wakeable = NULL; task_observers = TaskObservers(); task_source = std::make_unique(created_for); @@ -75,9 +74,9 @@ void MessageLoopTaskQueues::Dispose(TaskQueueId queue_id) { std::lock_guard guard(queue_mutex_); const auto& queue_entry = queue_entries_.at(queue_id); FML_DCHECK(queue_entry->subsumed_by == _kUnmerged); - auto &subsumed_set = queue_entry->owner_of; + auto& subsumed_set = queue_entry->owner_of; queue_entries_.erase(queue_id); - for (auto &subsumed :subsumed_set) { + for (auto& subsumed : subsumed_set) { queue_entries_.erase(subsumed); } } @@ -86,9 +85,9 @@ void MessageLoopTaskQueues::DisposeTasks(TaskQueueId queue_id) { std::lock_guard guard(queue_mutex_); const auto& queue_entry = queue_entries_.at(queue_id); FML_DCHECK(queue_entry->subsumed_by == _kUnmerged); - auto &subsumed_set = queue_entry->owner_of; + auto& subsumed_set = queue_entry->owner_of; queue_entry->task_source->ShutDown(); - for (auto &subsumed :subsumed_set) { + for (auto& subsumed : subsumed_set) { queue_entries_.at(subsumed)->task_source->ShutDown(); } } @@ -169,9 +168,9 @@ size_t MessageLoopTaskQueues::GetNumPendingTasks(TaskQueueId queue_id) const { size_t total_tasks = 0; total_tasks += queue_entry->task_source->GetNumPendingTasks(); - auto &subsumed_set = queue_entry->owner_of; - for (auto &subsumed :subsumed_set) { - const auto &subsumed_entry = queue_entries_.at(subsumed); + auto& subsumed_set = queue_entry->owner_of; + for (auto& subsumed : subsumed_set) { + const auto& subsumed_entry = queue_entries_.at(subsumed); total_tasks += subsumed_entry->task_source->GetNumPendingTasks(); } return total_tasks; @@ -205,7 +204,7 @@ std::vector MessageLoopTaskQueues::GetObserversToNotify( } auto& subsumed_set = queue_entries_.at(queue_id)->owner_of; - for (auto &subsumed :subsumed_set) { + for (auto& subsumed : subsumed_set) { for (const auto& observer : queue_entries_.at(subsumed)->task_observers) { observers.push_back(observer.second); } @@ -234,17 +233,21 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { return true; } - // Only don't check owner_entry->owner_of, it may contains items when merged with other different queues. + // Only don't check owner_entry->owner_of, it may contains items when merged + // with other different queues. if (owner_entry->subsumed_by != _kUnmerged) { - FML_LOG(WARNING) << "Thread merging failed: owner_entry was already subsumed by others."; + FML_LOG(WARNING) + << "Thread merging failed: owner_entry was already subsumed by others."; return false; } if (!subsumed_entry->owner_of.empty()) { - FML_LOG(WARNING) << "Thread merging failed: subsumed_entry already owns others."; + FML_LOG(WARNING) + << "Thread merging failed: subsumed_entry already owns others."; return false; } if (subsumed_entry->subsumed_by != _kUnmerged) { - FML_LOG(WARNING) << "Thread merging failed: subsumed_entry was already subsumed by others."; + FML_LOG(WARNING) << "Thread merging failed: subsumed_entry was already " + "subsumed by others."; return false; } // All checking is OK, set merged state. @@ -260,19 +263,22 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner, TaskQueueId subsumed) { std::lock_guard guard(queue_mutex_); - const auto &owner_entry = queue_entries_.at(owner); + const auto& owner_entry = queue_entries_.at(owner); if (owner_entry->owner_of.empty()) { - FML_LOG(WARNING) << "Thread unmerging failed: owner_entry doesn't own anyone."; + FML_LOG(WARNING) + << "Thread unmerging failed: owner_entry doesn't own anyone."; return false; } if (owner_entry->subsumed_by != _kUnmerged) { - FML_LOG(WARNING) << "Thread unmerging failed: owner_entry was subsumed by others."; + FML_LOG(WARNING) + << "Thread unmerging failed: owner_entry was subsumed by others."; return false; } if (queue_entries_.at(subsumed)->subsumed_by == _kUnmerged) { - FML_LOG(WARNING) << "Thread unmerging failed: subsumed_entry wasn't subsumed by others."; + FML_LOG(WARNING) + << "Thread unmerging failed: subsumed_entry wasn't subsumed by others."; return false; } @@ -296,7 +302,7 @@ bool MessageLoopTaskQueues::Owns(TaskQueueId owner, if (owner == _kUnmerged || subsumed == _kUnmerged) { return false; } - auto &subsumed_set = queue_entries_.at(owner)->owner_of; + auto& subsumed_set = queue_entries_.at(owner)->owner_of; return subsumed_set.find(subsumed) != subsumed_set.end(); } @@ -334,8 +340,8 @@ bool MessageLoopTaskQueues::HasPendingTasksUnlocked( return true; } - auto &subsumed_set = entry->owner_of; - for (auto &subsumed: subsumed_set) { + auto& subsumed_set = entry->owner_of; + for (auto& subsumed : subsumed_set) { if (!queue_entries_.at(subsumed)->task_source->IsEmpty()) { return true; } @@ -351,26 +357,28 @@ fml::TimePoint MessageLoopTaskQueues::GetNextWakeTimeUnlocked( TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( TaskQueueId owner) const { FML_DCHECK(HasPendingTasksUnlocked(owner)); - const auto &entry = queue_entries_.at(owner); + const auto& entry = queue_entries_.at(owner); if (entry->owner_of.empty()) { return entry->task_source->Top(); } - TaskSource *owner_tasks = entry->task_source.get(); + TaskSource* owner_tasks = entry->task_source.get(); std::vector candidate_top_tasks; if (!owner_tasks->IsEmpty()) { candidate_top_tasks.push_back(owner_tasks->Top()); } - for (TaskQueueId subsumed: entry->owner_of) { - TaskSource *subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); + for (TaskQueueId subsumed : entry->owner_of) { + TaskSource* subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); if (!subsumed_tasks->IsEmpty()) { candidate_top_tasks.push_back(subsumed_tasks->Top()); } } - // At least one task at the top because PeekNextTaskUnlocked() is called after HasPendingTasksUnlocked() + // At least one task at the top because PeekNextTaskUnlocked() is called after + // HasPendingTasksUnlocked() FML_CHECK(!candidate_top_tasks.empty()); - TaskSource::TopTask &top = *std::min_element(candidate_top_tasks.begin(), candidate_top_tasks.end()); + TaskSource::TopTask& top = + *std::min_element(candidate_top_tasks.begin(), candidate_top_tasks.end()); return top; } diff --git a/fml/message_loop_task_queues_merge_unmerge_unittests.cc b/fml/message_loop_task_queues_merge_unmerge_unittests.cc index a2705bac058ab..e5608c2fb7134 100644 --- a/fml/message_loop_task_queues_merge_unmerge_unittests.cc +++ b/fml/message_loop_task_queues_merge_unmerge_unittests.cc @@ -132,7 +132,8 @@ TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadySubsumed) { ASSERT_FALSE(task_queue->Merge(queue_id_2, queue_id_1)); } -TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadyOwnsButTryToBeSubsumed) { +TEST(MessageLoopTaskQueueMergeUnmerge, + MergeFailIfAlreadyOwnsButTryToBeSubsumed) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto queue_id_1 = task_queue->CreateTaskQueue(); diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 7404ffb1c7c94..b92c02c66b1c8 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -14,7 +14,9 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - shared_merger_impl_(fml::SharedThreadMergerImpl::GetSharedImpl(platform_queue_id, gpu_queue_id)), + shared_merger_impl_( + fml::SharedThreadMergerImpl::GetSharedImpl(platform_queue_id, + gpu_queue_id)), enabled_(true) { FML_LOG(ERROR) << "shared_merger_impl_=" << shared_merger_impl_.get(); } diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index cc02b0f2fe308..755fffd642af7 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -86,8 +86,8 @@ class RasterThreadMerger void Disable(); // Whether the thread merger is enabled. By default, the thread merger is - // enabled. If false, calls to |MergeWithLease| or |UnMergeNowIfLastOne| results in a - // noop. + // enabled. If false, calls to |MergeWithLease| or |UnMergeNowIfLastOne| + // results in a noop. bool IsEnabled(); // Registers a callback that can be used to clean up global state right after diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index dd624378de24d..07d24117169e6 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -750,7 +750,8 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { thread2.join(); } -TEST(RasterThreadMerger, MultipleMergersCanIndependentlyMergeTwoOrMoreThreadsIntoOne) { +TEST(RasterThreadMerger, + MultipleMergersCanIndependentlyMergeTwoOrMoreThreadsIntoOne) { fml::MessageLoop* loop1 = nullptr; fml::AutoResetWaitableEvent latch1; fml::AutoResetWaitableEvent term1; diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger_impl.cc index 25d64aa2d1a18..8910f1e93b257 100644 --- a/fml/shared_thread_merger_impl.cc +++ b/fml/shared_thread_merger_impl.cc @@ -6,20 +6,22 @@ #include "flutter/fml/shared_thread_merger_impl.h" -#include "flutter/fml/message_loop_impl.h" #include +#include "flutter/fml/message_loop_impl.h" namespace fml { std::mutex SharedThreadMergerImpl::creation_mutex_; // Guarded by creation_mutex_ -std::map> SharedThreadMergerImpl::shared_merger_instances_; +std::map> + SharedThreadMergerImpl::shared_merger_instances_; const int SharedThreadMergerImpl::kLeaseNotSet = -1; -fml::RefPtr SharedThreadMergerImpl::GetSharedImpl(fml::TaskQueueId owner, - fml::TaskQueueId subsumed) { +fml::RefPtr SharedThreadMergerImpl::GetSharedImpl( + fml::TaskQueueId owner, + fml::TaskQueueId subsumed) { std::scoped_lock creation(creation_mutex_); ThreadMergerKey key = {.owner = owner, .subsumed = subsumed}; @@ -31,7 +33,7 @@ fml::RefPtr SharedThreadMergerImpl::GetSharedImpl(fml::T return merger; } -void SharedThreadMergerImpl::RecordMergerCaller(RasterThreadMerger *caller) { +void SharedThreadMergerImpl::RecordMergerCaller(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); // Record current merge caller into callers set. merge_callers_.insert(caller); @@ -50,13 +52,14 @@ bool SharedThreadMergerImpl::MergeWithLease(size_t lease_term) { } bool SharedThreadMergerImpl::UnMergeNowUnSafe() { - FML_CHECK(lease_term_ == 0) << "lease_term_ must be 0 state before calling UnMergeNowUnSafe()"; + FML_CHECK(lease_term_ == 0) + << "lease_term_ must be 0 state before calling UnMergeNowUnSafe()"; bool success = task_queues_->Unmerge(owner_, subsumed_); FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; return success; } -bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger *caller) { +bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); merge_callers_.erase(caller); @@ -65,16 +68,16 @@ bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger *caller) { if (!merge_callers_.empty()) { return true; } - FML_CHECK(IsMergedUnSafe()) << "UnMergeNowIfLastOne() must be called only when thread are merged"; - lease_term_ = 0; // mark need unmerge now + FML_CHECK(IsMergedUnSafe()) + << "UnMergeNowIfLastOne() must be called only when thread are merged"; + lease_term_ = 0; // mark need unmerge now return UnMergeNowUnSafe(); } - bool SharedThreadMergerImpl::DecrementLease() { std::scoped_lock lock(mutex_); FML_DCHECK(lease_term_ > 0) - << "lease_term should always be positive when merged."; + << "lease_term should always be positive when merged."; lease_term_--; if (lease_term_ == 0) { // Unmerge now because lease_term_ decreased to zero. @@ -84,7 +87,8 @@ bool SharedThreadMergerImpl::DecrementLease() { return false; } -SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed) +SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, + fml::TaskQueueId subsumed) : owner_(owner), subsumed_(subsumed), task_queues_(fml::MessageLoopTaskQueues::GetInstance()), @@ -93,7 +97,8 @@ SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, fml::Task void SharedThreadMergerImpl::ExtendLeaseTo(size_t lease_term) { std::scoped_lock lock(mutex_); - FML_DCHECK(IsMergedUnSafe()) << "should be merged state when calling this method"; + FML_DCHECK(IsMergedUnSafe()) + << "should be merged state when calling this method"; if (lease_term_ != kLeaseNotSet && static_cast(lease_term) > lease_term_) { lease_term_ = lease_term; diff --git a/fml/shared_thread_merger_impl.h b/fml/shared_thread_merger_impl.h index 06c74a01dcde3..008ab04f571c5 100644 --- a/fml/shared_thread_merger_impl.h +++ b/fml/shared_thread_merger_impl.h @@ -20,7 +20,7 @@ class RasterThreadMerger; struct ThreadMergerKey { TaskQueueId owner; TaskQueueId subsumed; - bool operator<(const ThreadMergerKey &other) const { + bool operator<(const ThreadMergerKey& other) const { if (owner == other.owner) { return subsumed < other.subsumed; } else { @@ -29,17 +29,21 @@ struct ThreadMergerKey { } }; -class SharedThreadMergerImpl : public fml::RefCountedThreadSafe { +class SharedThreadMergerImpl + : public fml::RefCountedThreadSafe { public: SharedThreadMergerImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed); - static fml::RefPtr GetSharedImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed); - void RecordMergerCaller(RasterThreadMerger *caller); + static fml::RefPtr GetSharedImpl( + fml::TaskQueueId owner, + fml::TaskQueueId subsumed); + void RecordMergerCaller(RasterThreadMerger* caller); bool MergeWithLease(size_t lease_term); /// TODO DOC - bool UnMergeNowIfLastOne(RasterThreadMerger *caller); + bool UnMergeNowIfLastOne(RasterThreadMerger* caller); void ExtendLeaseTo(size_t lease_term); bool IsMergedUnSafe() const; bool DecrementLease(); + private: static const int kLeaseNotSet; fml::TaskQueueId owner_; @@ -47,10 +51,11 @@ class SharedThreadMergerImpl : public fml::RefCountedThreadSafe task_queues_; std::atomic_int lease_term_; std::mutex mutex_; - std::set merge_callers_; + std::set merge_callers_; static std::mutex creation_mutex_; // Guarded by creation_mutex_ - static std::map> shared_merger_instances_; + static std::map> + shared_merger_instances_; bool UnMergeNowUnSafe(); }; diff --git a/fml/task_source.h b/fml/task_source.h index 65763de59fd52..a9e93ecdf20fe 100644 --- a/fml/task_source.h +++ b/fml/task_source.h @@ -36,11 +36,9 @@ class TaskSource { public: struct TopTask { TaskQueueId task_queue_id; - const DelayedTask &task; + const DelayedTask& task; /// The operator less function is for std::vector to get min_element - bool operator<(const TopTask &other) const { - return other.task > task; - } + bool operator<(const TopTask& other) const { return other.task > task; } }; /// Construts a TaskSource with the given `task_queue_id`. diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index f2baeb64d992a..8404f2d57ae74 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -94,7 +94,7 @@ void Rasterizer::Teardown() { if (raster_thread_merger_.get() != nullptr && raster_thread_merger_.get()->IsMerged()) { FML_DCHECK(raster_thread_merger_->IsEnabled()); - raster_thread_merger_->UnMergeNowIfLastOne(); // TODO?? + raster_thread_merger_->UnMergeNowIfLastOne(); raster_thread_merger_->SetMergeUnmergeCallback(nullptr); } } diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc index f89862de21f76..cf492654c5858 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -4,10 +4,9 @@ #include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h" +#include #include "flutter/fml/trace_event.h" #include "flutter/shell/platform/android/surface/android_surface.h" -#include - namespace flutter { From 49a883d764f0b6940346b7c13db7adfb3c5e945f Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 15:30:18 +0800 Subject: [PATCH 03/24] Format gn code Signed-off-by: eggfly --- fml/BUILD.gn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fml/BUILD.gn b/fml/BUILD.gn index 35a6cb8fbf7eb..c46f34c8a2517 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -57,9 +57,9 @@ source_set("fml") { "posix_wrappers.h", "raster_thread_merger.cc", "raster_thread_merger.h", - "size.h", "shared_thread_merger_impl.cc", "shared_thread_merger_impl.h", + "size.h", "synchronization/atomic_object.h", "synchronization/count_down_latch.cc", "synchronization/count_down_latch.h", From 8d50b2c0fcb1f8ad7d1a069117cdf70db94144e4 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 15:44:49 +0800 Subject: [PATCH 04/24] Remove error log Signed-off-by: eggfly --- fml/raster_thread_merger.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index b92c02c66b1c8..1bdc218447057 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -17,9 +17,7 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, shared_merger_impl_( fml::SharedThreadMergerImpl::GetSharedImpl(platform_queue_id, gpu_queue_id)), - enabled_(true) { - FML_LOG(ERROR) << "shared_merger_impl_=" << shared_merger_impl_.get(); -} + enabled_(true) {} void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; From 6ff3fb76905ba2b3f1b8777bca3602ba58d80db7 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 16:15:53 +0800 Subject: [PATCH 05/24] Add licenses for new files Signed-off-by: eggfly --- ci/licenses_golden/licenses_flutter | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 8251ec6c78025..fd3f7b1e19706 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -259,6 +259,8 @@ FILE: ../../../flutter/fml/posix_wrappers.h FILE: ../../../flutter/fml/raster_thread_merger.cc FILE: ../../../flutter/fml/raster_thread_merger.h FILE: ../../../flutter/fml/raster_thread_merger_unittests.cc +FILE: ../../../flutter/fml/shared_thread_merger_impl.cc +FILE: ../../../flutter/fml/shared_thread_merger_impl.h FILE: ../../../flutter/fml/size.h FILE: ../../../flutter/fml/status.h FILE: ../../../flutter/fml/synchronization/atomic_object.h From 1f675be5bfe50b21a0c7ea60e66a336115c98e86 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 16:39:02 +0800 Subject: [PATCH 06/24] Remove some comments and add EnsureInitializedForCurrentThread Signed-off-by: eggfly --- fml/memory/task_runner_checker_unittest.cc | 1 + fml/shared_thread_merger_impl.cc | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/fml/memory/task_runner_checker_unittest.cc b/fml/memory/task_runner_checker_unittest.cc index e265d57fca535..0e8137514d0a6 100644 --- a/fml/memory/task_runner_checker_unittest.cc +++ b/fml/memory/task_runner_checker_unittest.cc @@ -43,6 +43,7 @@ TEST(TaskRunnerCheckerTests, FailsTheCheckIfOnDifferentTaskRunner) { } TEST(TaskRunnerCheckerTests, SameTaskRunnerRunsOnTheSameThread) { + fml::MessageLoop::EnsureInitializedForCurrentThread(); fml::MessageLoop& loop1 = fml::MessageLoop::GetCurrent(); fml::MessageLoop& loop2 = fml::MessageLoop::GetCurrent(); TaskQueueId a = loop1.GetTaskRunner()->GetTaskQueueId(); diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger_impl.cc index 8910f1e93b257..d1ce56377f1a1 100644 --- a/fml/shared_thread_merger_impl.cc +++ b/fml/shared_thread_merger_impl.cc @@ -63,8 +63,6 @@ bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); merge_callers_.erase(caller); - // 如果我是最后一个caller，那么不管lease_term_，立马Unmerge - // 如果我不是最后一个caller，那么需要等lease_term_通过DecrementLease()降低到0来Unmerge if (!merge_callers_.empty()) { return true; } From 686b7be1cb73aab083aa1c9aba8cbc5437af2d05 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 18:03:57 +0800 Subject: [PATCH 07/24] Remove used code --- .../android/external_view_embedder/external_view_embedder.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc index cf492654c5858..b1c2be3dadad9 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -4,7 +4,6 @@ #include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h" -#include #include "flutter/fml/trace_event.h" #include "flutter/shell/platform/android/surface/android_surface.h" @@ -211,8 +210,6 @@ AndroidExternalViewEmbedder::CreateSurfaceIfNeeded(GrDirectContext* context, return frame; } -// static std::set merged_merger_map; - // |ExternalViewEmbedder| PostPrerollResult AndroidExternalViewEmbedder::PostPrerollAction( fml::RefPtr raster_thread_merger) { From 30b21ca62f562e2bb3a462ca1ff9598955b58ea3 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 23 Jul 2021 23:09:55 +0800 Subject: [PATCH 08/24] Disable fml_unittests only for test Signed-off-by: eggfly --- testing/run_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 7fdbd0a136fdf..72f13dfecea77 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -165,7 +165,7 @@ def RunCCTests(build_dir, filter): RunEngineExecutable(build_dir, 'flow_unittests', filter, flow_flags + shuffle_flags) # TODO(44614): Re-enable after https://github.com/flutter/flutter/issues/44614 has been addressed. - RunEngineExecutable(build_dir, 'fml_unittests', filter, [ fml_unittests_filter ] + shuffle_flags) + # RunEngineExecutable(build_dir, 'fml_unittests', filter, [ fml_unittests_filter ] + shuffle_flags) RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) From b0a2b4f8f7c5a8d437402b1c91f17bce316b9654 Mon Sep 17 00:00:00 2001 From: eggfly Date: Sun, 25 Jul 2021 23:32:14 +0800 Subject: [PATCH 09/24] Some modifications after first review Signed-off-by: eggfly --- fml/memory/task_runner_checker_unittest.cc | 1 + fml/message_loop_task_queues.cc | 16 +++-- fml/message_loop_task_queues_unittests.cc | 80 ++++++++++++++++++++++ fml/raster_thread_merger.cc | 7 +- fml/raster_thread_merger.h | 21 ++++-- fml/raster_thread_merger_unittests.cc | 18 ++++- fml/shared_thread_merger_impl.cc | 34 +++++---- fml/shared_thread_merger_impl.h | 22 +++++- fml/task_source.h | 2 - shell/common/shell_unittests.cc | 4 +- 10 files changed, 165 insertions(+), 40 deletions(-) diff --git a/fml/memory/task_runner_checker_unittest.cc b/fml/memory/task_runner_checker_unittest.cc index 0e8137514d0a6..90c896d3827d4 100644 --- a/fml/memory/task_runner_checker_unittest.cc +++ b/fml/memory/task_runner_checker_unittest.cc @@ -52,6 +52,7 @@ TEST(TaskRunnerCheckerTests, SameTaskRunnerRunsOnTheSameThread) { } TEST(TaskRunnerCheckerTests, RunsOnDifferentThreadsReturnsFalse) { + fml::MessageLoop::EnsureInitializedForCurrentThread(); fml::MessageLoop& loop1 = fml::MessageLoop::GetCurrent(); TaskQueueId a = loop1.GetTaskRunner()->GetTaskQueueId(); fml::AutoResetWaitableEvent latch; diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 063e0551e5f7f..7a8d7c00a3493 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -10,7 +10,6 @@ #include #include "flutter/fml/make_copyable.h" -#include "flutter/fml/message_loop_impl.h" #include "flutter/fml/task_source.h" #include "flutter/fml/thread_local.h" @@ -233,18 +232,22 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { return true; } - // Only don't check owner_entry->owner_of, it may contains items when merged - // with other different queues. + // Won't check owner_entry->owner_of, because it may contains items when + // merged with other different queues. + + // Ensure owner_entry->subsumed_by being _kUnmerged if (owner_entry->subsumed_by != _kUnmerged) { FML_LOG(WARNING) << "Thread merging failed: owner_entry was already subsumed by others."; return false; } + // Ensure subsumed_entry->owner_of being empty if (!subsumed_entry->owner_of.empty()) { FML_LOG(WARNING) << "Thread merging failed: subsumed_entry already owns others."; return false; } + // Ensure subsumed_entry->subsumed_by being _kUnmerged if (subsumed_entry->subsumed_by != _kUnmerged) { FML_LOG(WARNING) << "Thread merging failed: subsumed_entry was already " "subsumed by others."; @@ -378,7 +381,12 @@ TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( // HasPendingTasksUnlocked() FML_CHECK(!candidate_top_tasks.empty()); TaskSource::TopTask& top = - *std::min_element(candidate_top_tasks.begin(), candidate_top_tasks.end()); + *std::min_element(candidate_top_tasks.begin(), candidate_top_tasks.end(), + [](const auto& a, const auto& b) { + // Because only operator>() is implemented in + // DelayedTask + return b.task > a.task; + }); return top; } diff --git a/fml/message_loop_task_queues_unittests.cc b/fml/message_loop_task_queues_unittests.cc index 9939baaf0781d..47faaf91d2cb7 100644 --- a/fml/message_loop_task_queues_unittests.cc +++ b/fml/message_loop_task_queues_unittests.cc @@ -62,6 +62,35 @@ TEST(MessageLoopTaskQueue, RegisterTwoTasksAndCount) { ASSERT_TRUE(task_queue->GetNumPendingTasks(queue_id) == 2); } +TEST(MessageLoopTaskQueue, RegisterTasksOnMergedQueuesAndCount) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + auto platform_queue = task_queue->CreateTaskQueue(); + auto raster_queue = task_queue->CreateTaskQueue(); + // A task in platform_queue + task_queue->RegisterTask( + platform_queue, []() {}, fml::TimePoint::Now()); + // A task in raster_queue + task_queue->RegisterTask( + raster_queue, []() {}, fml::TimePoint::Now()); + ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 1); + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 1); + + ASSERT_FALSE(task_queue->Owns(platform_queue, raster_queue)); + task_queue->Merge(platform_queue, raster_queue); + ASSERT_TRUE(task_queue->Owns(platform_queue, raster_queue)); + + ASSERT_TRUE(task_queue->HasPendingTasks(platform_queue)); + ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 2); + // The task count of subsumed queue is 0 + ASSERT_FALSE(task_queue->HasPendingTasks(raster_queue)); + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 0); + + task_queue->Unmerge(platform_queue, raster_queue); + ASSERT_FALSE(task_queue->Owns(platform_queue, raster_queue)); + ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 1); + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 1); +} + TEST(MessageLoopTaskQueue, PreserveTaskOrdering) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto queue_id = task_queue->CreateTaskQueue(); @@ -88,6 +117,46 @@ TEST(MessageLoopTaskQueue, PreserveTaskOrdering) { } } +TEST(MessageLoopTaskQueue, RegisterTasksOnMergedQueuesPreserveTaskOrdering) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + auto platform_queue = task_queue->CreateTaskQueue(); + auto raster1_queue = task_queue->CreateTaskQueue(); + auto raster2_queue = task_queue->CreateTaskQueue(); + int test_val = 0; + + // order 0 in raster1_queue + task_queue->RegisterTask( + raster1_queue, [&test_val]() { test_val = 0; }, fml::TimePoint::Now()); + + // order 1 in platform_queue + task_queue->RegisterTask( + platform_queue, [&test_val]() { test_val = 1; }, fml::TimePoint::Now()); + + // order 2 in raster2_queue + task_queue->RegisterTask( + raster2_queue, [&test_val]() { test_val = 2; }, fml::TimePoint::Now()); + + task_queue->Merge(platform_queue, raster1_queue); + ASSERT_TRUE(task_queue->Owns(platform_queue, raster1_queue)); + task_queue->Merge(platform_queue, raster2_queue); + ASSERT_TRUE(task_queue->Owns(platform_queue, raster2_queue)); + const auto now = fml::TimePoint::Now(); + int expected_value = 0; + // Right order: + // "test_val = 0" in raster1_queue + // "test_val = 1" in platform_queue + // "test_val = 2" in raster2_queue + for (;;) { + fml::closure invocation = task_queue->GetNextTaskToRun(platform_queue, now); + if (!invocation) { + break; + } + invocation(); + ASSERT_TRUE(test_val == expected_value); + expected_value++; + } +} + void TestNotifyObservers(fml::TaskQueueId queue_id) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); std::vector observers = @@ -194,6 +263,17 @@ TEST(MessageLoopTaskQueue, QueueDoNotOwnUnmergedTaskQueueId) { ASSERT_FALSE(task_queue->Owns(_kUnmerged, _kUnmerged)); } +TEST(MessageLoopTaskQueue, QueueOwnsMergedTaskQueueId) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + auto platform_queue = task_queue->CreateTaskQueue(); + auto raster_queue = task_queue->CreateTaskQueue(); + ASSERT_FALSE(task_queue->Owns(platform_queue, raster_queue)); + ASSERT_FALSE(task_queue->Owns(raster_queue, platform_queue)); + task_queue->Merge(platform_queue, raster_queue); + ASSERT_TRUE(task_queue->Owns(platform_queue, raster_queue)); + ASSERT_FALSE(task_queue->Owns(raster_queue, platform_queue)); +} + //------------------------------------------------------------------------------ /// Verifies that tasks can be added to task queues concurrently. /// diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 1bdc218447057..009a5d4edb75a 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -23,7 +23,7 @@ void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; } -void RasterThreadMerger::MergeWithLease(size_t lease_term) { +void RasterThreadMerger::MergeWithLease(int lease_term) { std::scoped_lock lock(mutex_); if (TaskQueuesAreSame()) { return; @@ -73,7 +73,7 @@ bool RasterThreadMerger::IsOnRasterizingThread() const { } } -void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { +void RasterThreadMerger::ExtendLeaseTo(int lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; if (TaskQueuesAreSame()) { return; @@ -84,8 +84,7 @@ void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { bool RasterThreadMerger::IsMerged() { std::scoped_lock lock(mutex_); - bool is_merged = IsMergedUnSafe(); - return is_merged; + return IsMergedUnSafe(); } void RasterThreadMerger::Enable() { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 755fffd642af7..259f311d9d428 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -35,9 +35,12 @@ class RasterThreadMerger // // If the task queues are the same, we consider them statically merged. // When task queues are statically merged this method becomes no-op. - void MergeWithLease(size_t lease_term); + void MergeWithLease(int lease_term); - // Un-merges the threads now, and resets the lease term to 0. + // Un-merges the threads now if current caller is the last merge caller, + // and it resets the lease term to 0, otherwise it will remove the caller + // record and return. The multiple caller records were recorded by the + // |RecordMergeCaller| method. // // Must be executed on the raster task runner. // @@ -48,7 +51,7 @@ class RasterThreadMerger // If the task queues are the same, we consider them statically merged. // When task queues are statically merged this method becomes no-op. - void ExtendLeaseTo(size_t lease_term); + void ExtendLeaseTo(int lease_term); // Returns |RasterThreadStatus::kUnmergedNow| if this call resulted in // splitting the raster and platform threads. Reduces the lease term by 1. @@ -57,9 +60,19 @@ class RasterThreadMerger // When task queues are statically merged this method becomes no-op. RasterThreadStatus DecrementLease(); + // Record current merge caller in the set of SharedThreadMergerImpl object. + // This method should be called before multiple merge callers of same + // owner/subsumed pair are going to call |MergeWithLease| method. + // + // And the reason why not putting this method inside |MergeWithLease| is + // |MergeWithLease| should not called when another caller already merged the + // threads and |IsMerged| return true. In this case only recording the + // current caller is needed. void RecordMergeCaller(); - // TODO DOC + // The method is locked by current instance, and asks the shared instance of + // SharedThreadMergerImpl and the merging state is determined by the + // lease_term_ counter. bool IsMerged(); // Waits until the threads are merged. diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 07d24117169e6..8c167a1561c84 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -722,24 +722,28 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { // Recording mergers themselves is needed. raster_thread_merger1_->RecordMergeCaller(); + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); raster_thread_merger2_->RecordMergeCaller(); + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + // Merge using the mergers raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); raster_thread_merger2_->MergeWithLease(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); ASSERT_TRUE(raster_thread_merger2_->IsMerged()); // Two callers state becomes one caller left. raster_thread_merger1_->UnMergeNowIfLastOne(); - // Check if still merged ASSERT_TRUE(raster_thread_merger1_->IsMerged()); ASSERT_TRUE(raster_thread_merger2_->IsMerged()); // One caller state becomes no callers left. raster_thread_merger2_->UnMergeNowIfLastOne(); - // Check if unmerged ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); @@ -824,10 +828,18 @@ TEST(RasterThreadMerger, // test caller records are separated below raster_thread_merger1_->RecordMergeCaller(); + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); raster_thread_merger2_->RecordMergeCaller(); + ASSERT_FALSE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_FALSE(raster_thread_merger2_->IsMerged()); raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); // Can unmerge independently raster_thread_merger1_->UnMergeNowIfLastOne(); diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger_impl.cc index d1ce56377f1a1..8cdab027add5b 100644 --- a/fml/shared_thread_merger_impl.cc +++ b/fml/shared_thread_merger_impl.cc @@ -7,7 +7,6 @@ #include "flutter/fml/shared_thread_merger_impl.h" #include -#include "flutter/fml/message_loop_impl.h" namespace fml { @@ -24,22 +23,30 @@ fml::RefPtr SharedThreadMergerImpl::GetSharedImpl( fml::TaskQueueId subsumed) { std::scoped_lock creation(creation_mutex_); ThreadMergerKey key = {.owner = owner, .subsumed = subsumed}; - - if (shared_merger_instances_.find(key) != shared_merger_instances_.end()) { - return shared_merger_instances_[key]; + auto iter = shared_merger_instances_.find(key); + if (iter != shared_merger_instances_.end()) { + return iter->second; } auto merger = fml::MakeRefCounted(owner, subsumed); shared_merger_instances_[key] = merger; return merger; } +SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, + fml::TaskQueueId subsumed) + : owner_(owner), + subsumed_(subsumed), + task_queues_(fml::MessageLoopTaskQueues::GetInstance()), + lease_term_(kLeaseNotSet) {} + void SharedThreadMergerImpl::RecordMergerCaller(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); // Record current merge caller into callers set. merge_callers_.insert(caller); } -bool SharedThreadMergerImpl::MergeWithLease(size_t lease_term) { +bool SharedThreadMergerImpl::MergeWithLease(int lease_term) { + FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); if (IsMergedUnSafe()) { return true; @@ -61,13 +68,12 @@ bool SharedThreadMergerImpl::UnMergeNowUnSafe() { bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); - merge_callers_.erase(caller); if (!merge_callers_.empty()) { return true; } FML_CHECK(IsMergedUnSafe()) - << "UnMergeNowIfLastOne() must be called only when thread are merged"; + << "UnMergeNowIfLastOne() must be called only when threads are merged"; lease_term_ = 0; // mark need unmerge now return UnMergeNowUnSafe(); } @@ -85,20 +91,12 @@ bool SharedThreadMergerImpl::DecrementLease() { return false; } -SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, - fml::TaskQueueId subsumed) - : owner_(owner), - subsumed_(subsumed), - task_queues_(fml::MessageLoopTaskQueues::GetInstance()), - lease_term_(kLeaseNotSet) {} - -void SharedThreadMergerImpl::ExtendLeaseTo(size_t lease_term) { +void SharedThreadMergerImpl::ExtendLeaseTo(int lease_term) { + FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); - FML_DCHECK(IsMergedUnSafe()) << "should be merged state when calling this method"; - if (lease_term_ != kLeaseNotSet && - static_cast(lease_term) > lease_term_) { + if (lease_term_ != kLeaseNotSet && lease_term > lease_term_) { lease_term_ = lease_term; } } diff --git a/fml/shared_thread_merger_impl.h b/fml/shared_thread_merger_impl.h index 008ab04f571c5..1ebac87ed8922 100644 --- a/fml/shared_thread_merger_impl.h +++ b/fml/shared_thread_merger_impl.h @@ -36,12 +36,28 @@ class SharedThreadMergerImpl static fml::RefPtr GetSharedImpl( fml::TaskQueueId owner, fml::TaskQueueId subsumed); + // It's called by |RasterThreadMerger::RecordMergerCaller()|. + // See the doc of |RasterThreadMerger::RecordMergerCaller()|. void RecordMergerCaller(RasterThreadMerger* caller); - bool MergeWithLease(size_t lease_term); - /// TODO DOC + + // It's called by |RasterThreadMerger::MergeWithLease()|. + // See the doc of |RasterThreadMerger::MergeWithLease()|. + bool MergeWithLease(int lease_term); + + // It's called by |RasterThreadMerger::UnMergeNowIfLastOne()|. + // See the doc of |RasterThreadMerger::UnMergeNowIfLastOne()|. bool UnMergeNowIfLastOne(RasterThreadMerger* caller); - void ExtendLeaseTo(size_t lease_term); + + // It's called by |RasterThreadMerger::ExtendLeaseTo()|. + // See the doc of |RasterThreadMerger::ExtendLeaseTo()|. + void ExtendLeaseTo(int lease_term); + + // It's called by |RasterThreadMerger::IsMergedUnSafe()|. + // See the doc of |RasterThreadMerger::IsMergedUnSafe()|. bool IsMergedUnSafe() const; + + // It's called by |RasterThreadMerger::DecrementLease()|. + // See the doc of |RasterThreadMerger::DecrementLease()|. bool DecrementLease(); private: diff --git a/fml/task_source.h b/fml/task_source.h index a9e93ecdf20fe..64e151d9482ec 100644 --- a/fml/task_source.h +++ b/fml/task_source.h @@ -37,8 +37,6 @@ class TaskSource { struct TopTask { TaskQueueId task_queue_id; const DelayedTask& task; - /// The operator less function is for std::vector to get min_element - bool operator<(const TopTask& other) const { return other.task > task; } }; /// Construts a TaskSource with the given `task_queue_id`. diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index a9812957bed9b..213707549f1b9 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -816,7 +816,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads #endif ) { - const size_t ThreadMergingLease = 10; + const int ThreadMergingLease = 10; auto settings = CreateSettingsForFixture(); fml::AutoResetWaitableEvent end_frame_latch; std::shared_ptr external_view_embedder; @@ -896,7 +896,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging #endif ) { - const size_t kThreadMergingLease = 10; + const int kThreadMergingLease = 10; auto settings = CreateSettingsForFixture(); fml::AutoResetWaitableEvent end_frame_latch; auto end_frame_callback = From dfda6a50d346dacbe8f0a4867730a551ea9e1fc4 Mon Sep 17 00:00:00 2001 From: eggfly Date: Mon, 26 Jul 2021 00:09:04 +0800 Subject: [PATCH 10/24] Change the owner_of member's doc --- fml/message_loop_task_queues.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fml/message_loop_task_queues.h b/fml/message_loop_task_queues.h index f56d48cc43583..386e10b83bf33 100644 --- a/fml/message_loop_task_queues.h +++ b/fml/message_loop_task_queues.h @@ -35,11 +35,11 @@ class TaskQueueEntry { TaskObservers task_observers; std::unique_ptr task_source; - // Note: Both of these can be _kUnmerged, which indicates that - // this queue has not been merged or subsumed. OR exactly one - // of these will be _kUnmerged, if owner_of is _kUnmerged, it means - // that the queue has been subsumed or else it owns another queue. + // If owner_of is not empty, it means it owns another queue. std::set owner_of; + + // If subsumed_by is not _kUnmerged, which indicates that + // this queue has been subsumed (owned by another queue) TaskQueueId subsumed_by; TaskQueueId created_for; From c0effef0cbf3b7d0424161b73d77e617a6edb6d7 Mon Sep 17 00:00:00 2001 From: eggfly Date: Mon, 26 Jul 2021 16:15:10 +0800 Subject: [PATCH 11/24] Factor out a wrapper to avoid repeated code for raster_thread_merger_unittests --- fml/raster_thread_merger_unittests.cc | 371 ++++++-------------------- 1 file changed, 75 insertions(+), 296 deletions(-) diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 8c167a1561c84..0e42794ac33ce 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -6,7 +6,6 @@ #include "flutter/fml/raster_thread_merger.h" -#include #include #include "flutter/fml/memory/ref_ptr.h" @@ -20,32 +19,49 @@ namespace fml { namespace testing { -TEST(RasterThreadMerger, RemainMergedTillLeaseExpires) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { +/// A mock task queue NOT calling MessageLoop->Run() in thread +struct TaskQueueWrapper { + fml::MessageLoop *loop = nullptr; + std::thread thread; + + /// The waiter for message loop initialized ok + fml::AutoResetWaitableEvent latch; + + /// The waiter for thread finished + fml::AutoResetWaitableEvent term; + + TaskQueueWrapper() : thread([this]() { fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); + loop = &fml::MessageLoop::GetCurrent(); + latch.Signal(); + term.Wait(); + }) { + latch.Wait(); + } - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { + void ThreadFunc() { fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); + loop = &fml::MessageLoop::GetCurrent(); + latch.Signal(); + term.Wait(); + } - latch1.Wait(); - latch2.Wait(); + ~TaskQueueWrapper() { + term.Signal(); + thread.join(); + } - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId GetTaskQueueId() { + return loop->GetTaskRunner()->GetTaskQueueId(); + } +}; + + +TEST(RasterThreadMerger, RemainMergedTillLeaseExpires) { + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); const int kNumFramesMerged = 5; @@ -60,11 +76,6 @@ TEST(RasterThreadMerger, RemainMergedTillLeaseExpires) { } ASSERT_FALSE(raster_thread_merger_->IsMerged()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, IsNotOnRasterizingThread) { @@ -159,31 +170,11 @@ TEST(RasterThreadMerger, IsNotOnRasterizingThread) { } TEST(RasterThreadMerger, LeaseExtension) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); const int kNumFramesMerged = 5; @@ -208,11 +199,6 @@ TEST(RasterThreadMerger, LeaseExtension) { } ASSERT_FALSE(raster_thread_merger_->IsMerged()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, WaitUntilMerged) { @@ -269,19 +255,8 @@ TEST(RasterThreadMerger, WaitUntilMerged) { } TEST(RasterThreadMerger, HandleTaskQueuesAreTheSame) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - latch1.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue; + fml::TaskQueueId qid1 = queue.GetTaskQueueId(); fml::TaskQueueId qid2 = qid1; const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); @@ -303,37 +278,13 @@ TEST(RasterThreadMerger, HandleTaskQueuesAreTheSame) { // Wait until merged should also return immediately. raster_thread_merger_->WaitUntilMerged(); ASSERT_TRUE(raster_thread_merger_->IsMerged()); - - term1.Signal(); - thread1.join(); } TEST(RasterThreadMerger, Enable) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); @@ -349,39 +300,13 @@ TEST(RasterThreadMerger, Enable) { raster_thread_merger_->DecrementLease(); ASSERT_FALSE(raster_thread_merger_->IsMerged()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, Disable) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); @@ -420,39 +345,13 @@ TEST(RasterThreadMerger, Disable) { } ASSERT_FALSE(raster_thread_merger_->IsMerged()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, IsEnabled) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); ASSERT_TRUE(raster_thread_merger_->IsEnabled()); @@ -462,11 +361,6 @@ TEST(RasterThreadMerger, IsEnabled) { raster_thread_merger_->Enable(); ASSERT_TRUE(raster_thread_merger_->IsEnabled()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) { @@ -576,31 +470,10 @@ TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskUnMergesThreads) { } TEST(RasterThreadMerger, SetMergeUnmergeCallback) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); @@ -616,39 +489,13 @@ TEST(RasterThreadMerger, SetMergeUnmergeCallback) { raster_thread_merger->DecrementLease(); ASSERT_EQ(2, callbacks); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger1_ = fml::MakeRefCounted(qid1, qid2); const auto raster_thread_merger2_ = @@ -679,39 +526,14 @@ TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } -TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - latch1.Wait(); - latch2.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); +TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger1_ = fml::MakeRefCounted(qid1, qid2); const auto raster_thread_merger2_ = @@ -747,52 +569,16 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { // Check if unmerged ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - - term1.Signal(); - term2.Signal(); - thread1.join(); - thread2.join(); } TEST(RasterThreadMerger, MultipleMergersCanIndependentlyMergeTwoOrMoreThreadsIntoOne) { - fml::MessageLoop* loop1 = nullptr; - fml::AutoResetWaitableEvent latch1; - fml::AutoResetWaitableEvent term1; - std::thread thread1([&loop1, &latch1, &term1]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop1 = &fml::MessageLoop::GetCurrent(); - latch1.Signal(); - term1.Wait(); - }); - - fml::MessageLoop* loop2 = nullptr; - fml::AutoResetWaitableEvent latch2; - fml::AutoResetWaitableEvent term2; - std::thread thread2([&loop2, &latch2, &term2]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop2 = &fml::MessageLoop::GetCurrent(); - latch2.Signal(); - term2.Wait(); - }); - - fml::MessageLoop* loop3 = nullptr; - fml::AutoResetWaitableEvent latch3; - fml::AutoResetWaitableEvent term3; - std::thread thread3([&loop3, &latch3, &term3]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop3 = &fml::MessageLoop::GetCurrent(); - latch3.Signal(); - term3.Wait(); - }); - - latch1.Wait(); - latch2.Wait(); - latch3.Wait(); - - fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid3 = loop3->GetTaskRunner()->GetTaskQueueId(); + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + TaskQueueWrapper queue3; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); + fml::TaskQueueId qid3 = queue3.GetTaskQueueId(); const auto raster_thread_merger1_ = fml::MakeRefCounted(qid1, qid2); @@ -850,13 +636,6 @@ TEST(RasterThreadMerger, raster_thread_merger2_->UnMergeNowIfLastOne(); ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - - term1.Signal(); - term2.Signal(); - term3.Signal(); - thread1.join(); - thread2.join(); - thread3.join(); } } // namespace testing From 747cb12c6725c31ff8a34dd9169984b19862e94e Mon Sep 17 00:00:00 2001 From: eggfly Date: Tue, 27 Jul 2021 15:48:55 +0800 Subject: [PATCH 12/24] Use optional instead of vector in PeekNextTaskUnlocked method --- fml/message_loop_task_queues.cc | 37 ++++++++++++++------------- fml/raster_thread_merger_unittests.cc | 7 ----- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 7a8d7c00a3493..7164a4d61b73d 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -362,32 +362,33 @@ TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( FML_DCHECK(HasPendingTasksUnlocked(owner)); const auto& entry = queue_entries_.at(owner); if (entry->owner_of.empty()) { + FML_CHECK(!entry->task_source->IsEmpty()); return entry->task_source->Top(); } - TaskSource* owner_tasks = entry->task_source.get(); - std::vector candidate_top_tasks; + // Use optional for the memory of TopTask object. + std::optional top_task; - if (!owner_tasks->IsEmpty()) { - candidate_top_tasks.push_back(owner_tasks->Top()); - } - for (TaskQueueId subsumed : entry->owner_of) { - TaskSource* subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); - if (!subsumed_tasks->IsEmpty()) { - candidate_top_tasks.push_back(subsumed_tasks->Top()); + auto top_task_updater = [&top_task](const TaskSource *source) { + if (source && !source->IsEmpty()) { + auto other_task = source->Top(); + if (!top_task.has_value() || top_task->task > other_task.task) { + top_task.emplace(other_task); + } } + }; + + TaskSource *owner_tasks = entry->task_source.get(); + top_task_updater(owner_tasks); + + for (TaskQueueId subsumed : entry->owner_of) { + TaskSource *subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); + top_task_updater(subsumed_tasks); } // At least one task at the top because PeekNextTaskUnlocked() is called after // HasPendingTasksUnlocked() - FML_CHECK(!candidate_top_tasks.empty()); - TaskSource::TopTask& top = - *std::min_element(candidate_top_tasks.begin(), candidate_top_tasks.end(), - [](const auto& a, const auto& b) { - // Because only operator>() is implemented in - // DelayedTask - return b.task > a.task; - }); - return top; + FML_CHECK(top_task.has_value()); + return top_task.value(); } } // namespace fml diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 0e42794ac33ce..519621011d184 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -39,13 +39,6 @@ struct TaskQueueWrapper { latch.Wait(); } - void ThreadFunc() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop = &fml::MessageLoop::GetCurrent(); - latch.Signal(); - term.Wait(); - } - ~TaskQueueWrapper() { term.Signal(); thread.join(); From 42b0808c25a683053865c252d0f9687aefe636f6 Mon Sep 17 00:00:00 2001 From: eggfly Date: Tue, 27 Jul 2021 16:23:50 +0800 Subject: [PATCH 13/24] Add detail logs for merge error cases --- fml/message_loop_task_queues.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 7164a4d61b73d..db476cf821a8e 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -238,19 +238,22 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { // Ensure owner_entry->subsumed_by being _kUnmerged if (owner_entry->subsumed_by != _kUnmerged) { FML_LOG(WARNING) - << "Thread merging failed: owner_entry was already subsumed by others."; + << "Thread merging failed: owner_entry was already subsumed by others, owner=" << owner << ", subsumed=" << subsumed + << ", owner->subsumed_by=" << owner_entry->subsumed_by; return false; } // Ensure subsumed_entry->owner_of being empty if (!subsumed_entry->owner_of.empty()) { FML_LOG(WARNING) - << "Thread merging failed: subsumed_entry already owns others."; + << "Thread merging failed: subsumed_entry already owns others, owner=" << owner << ", subsumed=" << subsumed + << ", subsumed->owner_of.size()=" << subsumed_entry->owner_of.size(); return false; } // Ensure subsumed_entry->subsumed_by being _kUnmerged if (subsumed_entry->subsumed_by != _kUnmerged) { - FML_LOG(WARNING) << "Thread merging failed: subsumed_entry was already " - "subsumed by others."; + FML_LOG(WARNING) + << "Thread merging failed: subsumed_entry was already subsumed by others, owner=" << owner << ", subsumed=" + << subsumed << ", subsumed->subsumed_by=" << subsumed_entry->subsumed_by; return false; } // All checking is OK, set merged state. From 7be5911b05a08a07d1f3fa677a894a30895a5761 Mon Sep 17 00:00:00 2001 From: eggfly Date: Tue, 27 Jul 2021 17:59:05 +0800 Subject: [PATCH 14/24] Update Merge/Unmerge docs, add more tests --- fml/message_loop_task_queues.cc | 16 ++-- fml/message_loop_task_queues.h | 14 ++-- ...oop_task_queues_merge_unmerge_unittests.cc | 33 ++++++-- fml/message_loop_task_queues_unittests.cc | 78 +++++++++++++++++++ 4 files changed, 124 insertions(+), 17 deletions(-) diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index db476cf821a8e..3e5b3db927dba 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -272,19 +272,25 @@ bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner, TaskQueueId subsumed) { const auto& owner_entry = queue_entries_.at(owner); if (owner_entry->owner_of.empty()) { FML_LOG(WARNING) - << "Thread unmerging failed: owner_entry doesn't own anyone."; + << "Thread unmerging failed: owner_entry doesn't own anyone, owner=" << owner << ", subsumed=" << subsumed; return false; } - if (owner_entry->subsumed_by != _kUnmerged) { FML_LOG(WARNING) - << "Thread unmerging failed: owner_entry was subsumed by others."; + << "Thread unmerging failed: owner_entry was subsumed by others, owner=" << owner << ", subsumed=" << subsumed + << ", owner_entry->subsumed_by=" << owner_entry->subsumed_by; return false; } - if (queue_entries_.at(subsumed)->subsumed_by == _kUnmerged) { FML_LOG(WARNING) - << "Thread unmerging failed: subsumed_entry wasn't subsumed by others."; + << "Thread unmerging failed: subsumed_entry wasn't subsumed by others, owner=" << owner << ", subsumed=" + << subsumed; + return false; + } + if (owner_entry->owner_of.find(subsumed) == owner_entry->owner_of.end()) { + FML_LOG(WARNING) + << "Thread unmerging failed: owner_entry didn't own the given subsumed queue id, owner=" << owner << ", subsumed=" + << subsumed; return false; } diff --git a/fml/message_loop_task_queues.h b/fml/message_loop_task_queues.h index 386e10b83bf33..80c35f85efe47 100644 --- a/fml/message_loop_task_queues.h +++ b/fml/message_loop_task_queues.h @@ -109,17 +109,19 @@ class MessageLoopTaskQueues // to it. It is not aware of whether a queue is merged or not. Same with // task observers. // 2. When we get the tasks to run now, we look at both the queue_ids - // for the owner, subsumed will spin. - // 3. Each task queue can only be merged and subsumed once. + // for the owner and the subsumed task queues. + // 3. We can merge multiple subsumed queues into one owner task queue, but + // the subsumed queues cannot be subsumed by other owner, and the owner + // cannot be owned by other owners. The merge function will check these + // error cases and return false result. // // Methods currently aware of the merged state of the queues: // HasPendingTasks, GetNextTaskToRun, GetNumPendingTasks - - // This method returns false if either the owner or subsumed has already been - // merged with something else. bool Merge(TaskQueueId owner, TaskQueueId subsumed); - // Will return false if the owner has not been merged before. + // Will return false if the owner has not been merged before, or owner was + // subsumed by others, or subsumed wasn't subsumed by others, or owner + // didn't own the given subsumed queue id. bool Unmerge(TaskQueueId owner, TaskQueueId subsumed); /// Returns \p true if \p owner owns the \p subsumed task queue. diff --git a/fml/message_loop_task_queues_merge_unmerge_unittests.cc b/fml/message_loop_task_queues_merge_unmerge_unittests.cc index e5608c2fb7134..ba4b57885977d 100644 --- a/fml/message_loop_task_queues_merge_unmerge_unittests.cc +++ b/fml/message_loop_task_queues_merge_unmerge_unittests.cc @@ -106,17 +106,39 @@ TEST(MessageLoopTaskQueueMergeUnmerge, MergeUnmergeTasksPreserved) { } /// Multiple standalone engines scene -TEST(MessageLoopTaskQueueMergeUnmerge, OneCanOwnMultipleQueues) { +TEST(MessageLoopTaskQueueMergeUnmerge, OneCanOwnMultipleQueuesAndUnmergeIndependently) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); - auto queue_id_1 = task_queue->CreateTaskQueue(); auto queue_id_2 = task_queue->CreateTaskQueue(); auto queue_id_3 = task_queue->CreateTaskQueue(); - auto queue_id_4 = task_queue->CreateTaskQueue(); + // merge ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_2)); + ASSERT_TRUE(task_queue->Owns(queue_id_1, queue_id_2)); + ASSERT_FALSE(task_queue->Owns(queue_id_1, queue_id_3)); + ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_3)); - ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_4)); + ASSERT_TRUE(task_queue->Owns(queue_id_1, queue_id_2)); + ASSERT_TRUE(task_queue->Owns(queue_id_1, queue_id_3)); + + // unmerge + ASSERT_TRUE(task_queue->Unmerge(queue_id_1, queue_id_2)); + ASSERT_FALSE(task_queue->Owns(queue_id_1, queue_id_2)); + ASSERT_TRUE(task_queue->Owns(queue_id_1, queue_id_3)); + + ASSERT_TRUE(task_queue->Unmerge(queue_id_1, queue_id_3)); + ASSERT_FALSE(task_queue->Owns(queue_id_1, queue_id_2)); + ASSERT_FALSE(task_queue->Owns(queue_id_1, queue_id_3)); +} + +TEST(MessageLoopTaskQueueMergeUnmerge, CannotMergeSameQueueToTwoDifferentOwners) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + auto queue = task_queue->CreateTaskQueue(); + auto owner_1 = task_queue->CreateTaskQueue(); + auto owner_2 = task_queue->CreateTaskQueue(); + + ASSERT_TRUE(task_queue->Merge(owner_1, queue)); + ASSERT_FALSE(task_queue->Merge(owner_2, queue)); } TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadySubsumed) { @@ -126,8 +148,7 @@ TEST(MessageLoopTaskQueueMergeUnmerge, MergeFailIfAlreadySubsumed) { auto queue_id_2 = task_queue->CreateTaskQueue(); auto queue_id_3 = task_queue->CreateTaskQueue(); - task_queue->Merge(queue_id_1, queue_id_2); - + ASSERT_TRUE(task_queue->Merge(queue_id_1, queue_id_2)); ASSERT_FALSE(task_queue->Merge(queue_id_2, queue_id_3)); ASSERT_FALSE(task_queue->Merge(queue_id_2, queue_id_1)); } diff --git a/fml/message_loop_task_queues_unittests.cc b/fml/message_loop_task_queues_unittests.cc index 47faaf91d2cb7..910bddee860b4 100644 --- a/fml/message_loop_task_queues_unittests.cc +++ b/fml/message_loop_task_queues_unittests.cc @@ -157,6 +157,84 @@ TEST(MessageLoopTaskQueue, RegisterTasksOnMergedQueuesPreserveTaskOrdering) { } } + +TEST(MessageLoopTaskQueue, UnmergeRespectTheOriginalTaskOrderingInQueues) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + auto platform_queue = task_queue->CreateTaskQueue(); + auto raster_queue = task_queue->CreateTaskQueue(); + int test_val = 0; + + // order 0 in platform_queue + task_queue->RegisterTask( + platform_queue, [&test_val]() { test_val = 0; }, fml::TimePoint::Now()); + // order 1 in platform_queue + task_queue->RegisterTask( + platform_queue, [&test_val]() { test_val = 1; }, fml::TimePoint::Now()); + // order 2 in raster_queue + task_queue->RegisterTask( + raster_queue, [&test_val]() { test_val = 2; }, fml::TimePoint::Now()); + // order 3 in raster_queue + task_queue->RegisterTask( + raster_queue, [&test_val]() { test_val = 3; }, fml::TimePoint::Now()); + // order 4 in platform_queue + task_queue->RegisterTask( + platform_queue, [&test_val]() { test_val = 4; }, fml::TimePoint::Now()); + // order 5 in raster_queue + task_queue->RegisterTask( + raster_queue, [&test_val]() { test_val = 5; }, fml::TimePoint::Now()); + + ASSERT_TRUE(task_queue->Merge(platform_queue, raster_queue)); + ASSERT_TRUE(task_queue->Owns(platform_queue, raster_queue)); + const auto now = fml::TimePoint::Now(); + // The right order after merged and consumed 3 tasks: + // "test_val = 0" in platform_queue + // "test_val = 1" in platform_queue + // "test_val = 2" in raster_queue (running on platform) + for (int i = 0; i < 3; i++) { + fml::closure invocation = task_queue->GetNextTaskToRun(platform_queue, now); + ASSERT_FALSE(!invocation); + invocation(); + ASSERT_TRUE(test_val == i); + } + ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 3); + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 0); + + ASSERT_TRUE(task_queue->Unmerge(platform_queue, raster_queue)); + ASSERT_FALSE(task_queue->Owns(platform_queue, raster_queue)); + + // The right order after unmerged and left 3 tasks: + // "test_val = 3" in raster_queue + // "test_val = 4" in platform_queue + // "test_val = 5" in raster_queue + + // platform_queue has 1 task left: "test_val = 4" + { + ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 1); + fml::closure invocation = task_queue->GetNextTaskToRun(platform_queue, now); + ASSERT_FALSE(!invocation); + invocation(); + ASSERT_TRUE(test_val == 4); + ASSERT_TRUE(task_queue->GetNumPendingTasks(platform_queue) == 0); + } + + // raster_queue has 2 tasks left: "test_val = 3" and "test_val = 5" + { + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 2); + fml::closure invocation = task_queue->GetNextTaskToRun(raster_queue, now); + ASSERT_FALSE(!invocation); + invocation(); + ASSERT_TRUE(test_val == 3); + } + { + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 1); + fml::closure invocation = task_queue->GetNextTaskToRun(raster_queue, now); + ASSERT_FALSE(!invocation); + invocation(); + ASSERT_TRUE(test_val == 5); + ASSERT_TRUE(task_queue->GetNumPendingTasks(raster_queue) == 0); + } +} + void TestNotifyObservers(fml::TaskQueueId queue_id) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); std::vector observers = From 6883d9ddd14d4dbd7176fa82476e8289a6e710db Mon Sep 17 00:00:00 2001 From: eggfly Date: Tue, 27 Jul 2021 21:52:27 +0800 Subject: [PATCH 15/24] Use raw pointers, add some comment --- fml/shared_thread_merger_impl.cc | 6 +++--- fml/shared_thread_merger_impl.h | 13 ++++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger_impl.cc index 8cdab027add5b..4c8536bbe48bd 100644 --- a/fml/shared_thread_merger_impl.cc +++ b/fml/shared_thread_merger_impl.cc @@ -13,12 +13,12 @@ namespace fml { std::mutex SharedThreadMergerImpl::creation_mutex_; // Guarded by creation_mutex_ -std::map> +std::map SharedThreadMergerImpl::shared_merger_instances_; const int SharedThreadMergerImpl::kLeaseNotSet = -1; -fml::RefPtr SharedThreadMergerImpl::GetSharedImpl( +SharedThreadMergerImpl* SharedThreadMergerImpl::GetSharedImpl( fml::TaskQueueId owner, fml::TaskQueueId subsumed) { std::scoped_lock creation(creation_mutex_); @@ -27,7 +27,7 @@ fml::RefPtr SharedThreadMergerImpl::GetSharedImpl( if (iter != shared_merger_instances_.end()) { return iter->second; } - auto merger = fml::MakeRefCounted(owner, subsumed); + auto merger = new SharedThreadMergerImpl(owner, subsumed); shared_merger_instances_[key] = merger; return merger; } diff --git a/fml/shared_thread_merger_impl.h b/fml/shared_thread_merger_impl.h index 1ebac87ed8922..98d48dd572245 100644 --- a/fml/shared_thread_merger_impl.h +++ b/fml/shared_thread_merger_impl.h @@ -32,10 +32,8 @@ struct ThreadMergerKey { class SharedThreadMergerImpl : public fml::RefCountedThreadSafe { public: - SharedThreadMergerImpl(fml::TaskQueueId owner, fml::TaskQueueId subsumed); - static fml::RefPtr GetSharedImpl( - fml::TaskQueueId owner, - fml::TaskQueueId subsumed); + SharedThreadMergerImpl(TaskQueueId owner, TaskQueueId subsumed); + static SharedThreadMergerImpl* GetSharedImpl(TaskQueueId owner, TaskQueueId subsumed); // It's called by |RasterThreadMerger::RecordMergerCaller()|. // See the doc of |RasterThreadMerger::RecordMergerCaller()|. void RecordMergerCaller(RasterThreadMerger* caller); @@ -67,10 +65,15 @@ class SharedThreadMergerImpl fml::RefPtr task_queues_; std::atomic_int lease_term_; std::mutex mutex_; + + /// The |RecordMergerCaller| method will record the caller + /// into this merge_callers_ set, |UnMergeNowIfLastOne()| + /// method will remove the caller from this merge_callers_. std::set merge_callers_; + static std::mutex creation_mutex_; // Guarded by creation_mutex_ - static std::map> + static std::map shared_merger_instances_; bool UnMergeNowUnSafe(); }; From 1ce6a56dd9aa5e6c8c1ffe8586c9730ce0d3799f Mon Sep 17 00:00:00 2001 From: eggfly Date: Wed, 28 Jul 2021 17:52:14 +0800 Subject: [PATCH 16/24] Fix a wrong dispose order to avoid objc test failure And format code --- fml/message_loop_task_queues.cc | 49 +++++++++++-------- ...oop_task_queues_merge_unmerge_unittests.cc | 6 ++- fml/message_loop_task_queues_unittests.cc | 1 - fml/raster_thread_merger_unittests.cc | 17 +++---- fml/shared_thread_merger_impl.cc | 2 +- fml/shared_thread_merger_impl.h | 5 +- 6 files changed, 44 insertions(+), 36 deletions(-) diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 3e5b3db927dba..b090fefafb4d4 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -74,10 +74,11 @@ void MessageLoopTaskQueues::Dispose(TaskQueueId queue_id) { const auto& queue_entry = queue_entries_.at(queue_id); FML_DCHECK(queue_entry->subsumed_by == _kUnmerged); auto& subsumed_set = queue_entry->owner_of; - queue_entries_.erase(queue_id); for (auto& subsumed : subsumed_set) { queue_entries_.erase(subsumed); } + // Erase owner queue_id at last to avoid &subsumed_set from being invalid + queue_entries_.erase(queue_id); } void MessageLoopTaskQueues::DisposeTasks(TaskQueueId queue_id) { @@ -237,23 +238,27 @@ bool MessageLoopTaskQueues::Merge(TaskQueueId owner, TaskQueueId subsumed) { // Ensure owner_entry->subsumed_by being _kUnmerged if (owner_entry->subsumed_by != _kUnmerged) { - FML_LOG(WARNING) - << "Thread merging failed: owner_entry was already subsumed by others, owner=" << owner << ", subsumed=" << subsumed - << ", owner->subsumed_by=" << owner_entry->subsumed_by; + FML_LOG(WARNING) << "Thread merging failed: owner_entry was already " + "subsumed by others, owner=" + << owner << ", subsumed=" << subsumed + << ", owner->subsumed_by=" << owner_entry->subsumed_by; return false; } // Ensure subsumed_entry->owner_of being empty if (!subsumed_entry->owner_of.empty()) { FML_LOG(WARNING) - << "Thread merging failed: subsumed_entry already owns others, owner=" << owner << ", subsumed=" << subsumed - << ", subsumed->owner_of.size()=" << subsumed_entry->owner_of.size(); + << "Thread merging failed: subsumed_entry already owns others, owner=" + << owner << ", subsumed=" << subsumed + << ", subsumed->owner_of.size()=" << subsumed_entry->owner_of.size(); return false; } // Ensure subsumed_entry->subsumed_by being _kUnmerged if (subsumed_entry->subsumed_by != _kUnmerged) { - FML_LOG(WARNING) - << "Thread merging failed: subsumed_entry was already subsumed by others, owner=" << owner << ", subsumed=" - << subsumed << ", subsumed->subsumed_by=" << subsumed_entry->subsumed_by; + FML_LOG(WARNING) << "Thread merging failed: subsumed_entry was already " + "subsumed by others, owner=" + << owner << ", subsumed=" << subsumed + << ", subsumed->subsumed_by=" + << subsumed_entry->subsumed_by; return false; } // All checking is OK, set merged state. @@ -272,25 +277,27 @@ bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner, TaskQueueId subsumed) { const auto& owner_entry = queue_entries_.at(owner); if (owner_entry->owner_of.empty()) { FML_LOG(WARNING) - << "Thread unmerging failed: owner_entry doesn't own anyone, owner=" << owner << ", subsumed=" << subsumed; + << "Thread unmerging failed: owner_entry doesn't own anyone, owner=" + << owner << ", subsumed=" << subsumed; return false; } if (owner_entry->subsumed_by != _kUnmerged) { FML_LOG(WARNING) - << "Thread unmerging failed: owner_entry was subsumed by others, owner=" << owner << ", subsumed=" << subsumed - << ", owner_entry->subsumed_by=" << owner_entry->subsumed_by; + << "Thread unmerging failed: owner_entry was subsumed by others, owner=" + << owner << ", subsumed=" << subsumed + << ", owner_entry->subsumed_by=" << owner_entry->subsumed_by; return false; } if (queue_entries_.at(subsumed)->subsumed_by == _kUnmerged) { - FML_LOG(WARNING) - << "Thread unmerging failed: subsumed_entry wasn't subsumed by others, owner=" << owner << ", subsumed=" - << subsumed; + FML_LOG(WARNING) << "Thread unmerging failed: subsumed_entry wasn't " + "subsumed by others, owner=" + << owner << ", subsumed=" << subsumed; return false; } if (owner_entry->owner_of.find(subsumed) == owner_entry->owner_of.end()) { - FML_LOG(WARNING) - << "Thread unmerging failed: owner_entry didn't own the given subsumed queue id, owner=" << owner << ", subsumed=" - << subsumed; + FML_LOG(WARNING) << "Thread unmerging failed: owner_entry didn't own the " + "given subsumed queue id, owner=" + << owner << ", subsumed=" << subsumed; return false; } @@ -378,7 +385,7 @@ TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( // Use optional for the memory of TopTask object. std::optional top_task; - auto top_task_updater = [&top_task](const TaskSource *source) { + auto top_task_updater = [&top_task](const TaskSource* source) { if (source && !source->IsEmpty()) { auto other_task = source->Top(); if (!top_task.has_value() || top_task->task > other_task.task) { @@ -387,11 +394,11 @@ TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( } }; - TaskSource *owner_tasks = entry->task_source.get(); + TaskSource* owner_tasks = entry->task_source.get(); top_task_updater(owner_tasks); for (TaskQueueId subsumed : entry->owner_of) { - TaskSource *subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); + TaskSource* subsumed_tasks = queue_entries_.at(subsumed)->task_source.get(); top_task_updater(subsumed_tasks); } // At least one task at the top because PeekNextTaskUnlocked() is called after diff --git a/fml/message_loop_task_queues_merge_unmerge_unittests.cc b/fml/message_loop_task_queues_merge_unmerge_unittests.cc index ba4b57885977d..8f28882498a92 100644 --- a/fml/message_loop_task_queues_merge_unmerge_unittests.cc +++ b/fml/message_loop_task_queues_merge_unmerge_unittests.cc @@ -106,7 +106,8 @@ TEST(MessageLoopTaskQueueMergeUnmerge, MergeUnmergeTasksPreserved) { } /// Multiple standalone engines scene -TEST(MessageLoopTaskQueueMergeUnmerge, OneCanOwnMultipleQueuesAndUnmergeIndependently) { +TEST(MessageLoopTaskQueueMergeUnmerge, + OneCanOwnMultipleQueuesAndUnmergeIndependently) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto queue_id_1 = task_queue->CreateTaskQueue(); auto queue_id_2 = task_queue->CreateTaskQueue(); @@ -131,7 +132,8 @@ TEST(MessageLoopTaskQueueMergeUnmerge, OneCanOwnMultipleQueuesAndUnmergeIndepend ASSERT_FALSE(task_queue->Owns(queue_id_1, queue_id_3)); } -TEST(MessageLoopTaskQueueMergeUnmerge, CannotMergeSameQueueToTwoDifferentOwners) { +TEST(MessageLoopTaskQueueMergeUnmerge, + CannotMergeSameQueueToTwoDifferentOwners) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto queue = task_queue->CreateTaskQueue(); auto owner_1 = task_queue->CreateTaskQueue(); diff --git a/fml/message_loop_task_queues_unittests.cc b/fml/message_loop_task_queues_unittests.cc index 910bddee860b4..7df9c44c6b142 100644 --- a/fml/message_loop_task_queues_unittests.cc +++ b/fml/message_loop_task_queues_unittests.cc @@ -157,7 +157,6 @@ TEST(MessageLoopTaskQueue, RegisterTasksOnMergedQueuesPreserveTaskOrdering) { } } - TEST(MessageLoopTaskQueue, UnmergeRespectTheOriginalTaskOrderingInQueues) { auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); auto platform_queue = task_queue->CreateTaskQueue(); diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 519621011d184..be97e910bdbb7 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -21,7 +21,7 @@ namespace testing { /// A mock task queue NOT calling MessageLoop->Run() in thread struct TaskQueueWrapper { - fml::MessageLoop *loop = nullptr; + fml::MessageLoop* loop = nullptr; std::thread thread; /// The waiter for message loop initialized ok @@ -30,12 +30,13 @@ struct TaskQueueWrapper { /// The waiter for thread finished fml::AutoResetWaitableEvent term; - TaskQueueWrapper() : thread([this]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop = &fml::MessageLoop::GetCurrent(); - latch.Signal(); - term.Wait(); - }) { + TaskQueueWrapper() + : thread([this]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop = &fml::MessageLoop::GetCurrent(); + latch.Signal(); + term.Wait(); + }) { latch.Wait(); } @@ -49,7 +50,6 @@ struct TaskQueueWrapper { } }; - TEST(RasterThreadMerger, RemainMergedTillLeaseExpires) { TaskQueueWrapper queue1; TaskQueueWrapper queue2; @@ -521,7 +521,6 @@ TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { ASSERT_FALSE(raster_thread_merger2_->IsMerged()); } - TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { TaskQueueWrapper queue1; TaskQueueWrapper queue2; diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger_impl.cc index 4c8536bbe48bd..5cbc192df329a 100644 --- a/fml/shared_thread_merger_impl.cc +++ b/fml/shared_thread_merger_impl.cc @@ -13,7 +13,7 @@ namespace fml { std::mutex SharedThreadMergerImpl::creation_mutex_; // Guarded by creation_mutex_ -std::map +std::map SharedThreadMergerImpl::shared_merger_instances_; const int SharedThreadMergerImpl::kLeaseNotSet = -1; diff --git a/fml/shared_thread_merger_impl.h b/fml/shared_thread_merger_impl.h index 98d48dd572245..73ed485ea8e34 100644 --- a/fml/shared_thread_merger_impl.h +++ b/fml/shared_thread_merger_impl.h @@ -33,7 +33,8 @@ class SharedThreadMergerImpl : public fml::RefCountedThreadSafe { public: SharedThreadMergerImpl(TaskQueueId owner, TaskQueueId subsumed); - static SharedThreadMergerImpl* GetSharedImpl(TaskQueueId owner, TaskQueueId subsumed); + static SharedThreadMergerImpl* GetSharedImpl(TaskQueueId owner, + TaskQueueId subsumed); // It's called by |RasterThreadMerger::RecordMergerCaller()|. // See the doc of |RasterThreadMerger::RecordMergerCaller()|. void RecordMergerCaller(RasterThreadMerger* caller); @@ -73,7 +74,7 @@ class SharedThreadMergerImpl static std::mutex creation_mutex_; // Guarded by creation_mutex_ - static std::map + static std::map shared_merger_instances_; bool UnMergeNowUnSafe(); }; From 61eda81de7d37017a592af73c9af5b38a554a327 Mon Sep 17 00:00:00 2001 From: eggfly Date: Wed, 28 Jul 2021 19:41:44 +0800 Subject: [PATCH 17/24] fix raw pointer problem --- fml/raster_thread_merger.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 259f311d9d428..2f879aecaaf1c 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -113,7 +113,7 @@ class RasterThreadMerger private: fml::TaskQueueId platform_queue_id_; fml::TaskQueueId gpu_queue_id_; - fml::RefPtr shared_merger_impl_; + SharedThreadMergerImpl* shared_merger_impl_; std::condition_variable merged_condition_; std::mutex mutex_; fml::closure merge_unmerge_callback_; From 2330090b567977b6074a3423bc42db95de016deb Mon Sep 17 00:00:00 2001 From: eggfly Date: Wed, 28 Jul 2021 20:01:29 +0800 Subject: [PATCH 18/24] Fix windows msvc build error --- fml/message_loop_task_queues.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index b090fefafb4d4..093ef5190d801 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -8,6 +8,7 @@ #include #include +#include #include "flutter/fml/make_copyable.h" #include "flutter/fml/task_source.h" From dd555db4f8cd1d4218684fc1d535238670cb9b0a Mon Sep 17 00:00:00 2001 From: eggfly Date: Thu, 29 Jul 2021 22:20:02 +0800 Subject: [PATCH 19/24] Fix wrong member order which causes windows fml_unittests failure --- fml/raster_thread_merger_unittests.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index be97e910bdbb7..ade8f7214a77d 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -22,7 +22,6 @@ namespace testing { /// A mock task queue NOT calling MessageLoop->Run() in thread struct TaskQueueWrapper { fml::MessageLoop* loop = nullptr; - std::thread thread; /// The waiter for message loop initialized ok fml::AutoResetWaitableEvent latch; @@ -30,6 +29,12 @@ struct TaskQueueWrapper { /// The waiter for thread finished fml::AutoResetWaitableEvent term; + /// This field must below latch and term member, because + /// cpp standard reference: + /// non-static data members are initialized in the order they were declared in + /// the class definition + std::thread thread; + TaskQueueWrapper() : thread([this]() { fml::MessageLoop::EnsureInitializedForCurrentThread(); @@ -45,7 +50,7 @@ struct TaskQueueWrapper { thread.join(); } - fml::TaskQueueId GetTaskQueueId() { + fml::TaskQueueId GetTaskQueueId() const { return loop->GetTaskRunner()->GetTaskQueueId(); } }; From ec8df0f061ffb773e6c30b9169fe23a971d2e517 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 30 Jul 2021 16:06:45 +0800 Subject: [PATCH 20/24] Rename some files and change to better doc --- ci/licenses_golden/licenses_flutter | 4 +-- fml/BUILD.gn | 4 +-- fml/message_loop_task_queues.h | 16 ++++++---- fml/raster_thread_merger.cc | 17 +++++----- fml/raster_thread_merger.h | 8 ++--- ...merger_impl.cc => shared_thread_merger.cc} | 32 +++++++++---------- ...d_merger_impl.h => shared_thread_merger.h} | 12 +++---- 7 files changed, 47 insertions(+), 46 deletions(-) rename fml/{shared_thread_merger_impl.cc => shared_thread_merger.cc} (71%) rename fml/{shared_thread_merger_impl.h => shared_thread_merger.h} (87%) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index fd3f7b1e19706..94d02ff8f2c3e 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -259,8 +259,8 @@ FILE: ../../../flutter/fml/posix_wrappers.h FILE: ../../../flutter/fml/raster_thread_merger.cc FILE: ../../../flutter/fml/raster_thread_merger.h FILE: ../../../flutter/fml/raster_thread_merger_unittests.cc -FILE: ../../../flutter/fml/shared_thread_merger_impl.cc -FILE: ../../../flutter/fml/shared_thread_merger_impl.h +FILE: ../../../flutter/fml/shared_thread_merger.cc +FILE: ../../../flutter/fml/shared_thread_merger.h FILE: ../../../flutter/fml/size.h FILE: ../../../flutter/fml/status.h FILE: ../../../flutter/fml/synchronization/atomic_object.h diff --git a/fml/BUILD.gn b/fml/BUILD.gn index c46f34c8a2517..452e56d67203a 100644 --- a/fml/BUILD.gn +++ b/fml/BUILD.gn @@ -57,8 +57,8 @@ source_set("fml") { "posix_wrappers.h", "raster_thread_merger.cc", "raster_thread_merger.h", - "shared_thread_merger_impl.cc", - "shared_thread_merger_impl.h", + "shared_thread_merger.cc", + "shared_thread_merger.h", "size.h", "synchronization/atomic_object.h", "synchronization/count_down_latch.cc", diff --git a/fml/message_loop_task_queues.h b/fml/message_loop_task_queues.h index 80c35f85efe47..34a62f5eaa9e7 100644 --- a/fml/message_loop_task_queues.h +++ b/fml/message_loop_task_queues.h @@ -35,11 +35,12 @@ class TaskQueueEntry { TaskObservers task_observers; std::unique_ptr task_source; - // If owner_of is not empty, it means it owns another queue. + // Set of TaskQueueIds and in-turn the TaskQueues owned by this TaskQueue. If + // the set is empty, this TaskQueue does not own any other TaskQueues. std::set owner_of; - // If subsumed_by is not _kUnmerged, which indicates that - // this queue has been subsumed (owned by another queue) + // Identifies the TaskQueue that subsumes this TaskQueue. If it is _kUnmerged, + // it indiacates that this TaskQueue is not owned by any other TaskQueue. TaskQueueId subsumed_by; TaskQueueId created_for; @@ -110,10 +111,11 @@ class MessageLoopTaskQueues // task observers. // 2. When we get the tasks to run now, we look at both the queue_ids // for the owner and the subsumed task queues. - // 3. We can merge multiple subsumed queues into one owner task queue, but - // the subsumed queues cannot be subsumed by other owner, and the owner - // cannot be owned by other owners. The merge function will check these - // error cases and return false result. + // 3. One TaskQueue can subsume multiple other TaskQueues. A TaskQueue can be + // in exactly one of the following three states: + // a. Be an owner of multiple other TaskQueues. + // b. Be subsumed by a TaskQueue (an owner can never be subsumed). + // c. Be independent, i.e, neither owner nor be subsumed. // // Methods currently aware of the merged state of the queues: // HasPendingTasks, GetNextTaskToRun, GetNumPendingTasks diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 009a5d4edb75a..26c112977ca88 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -14,9 +14,8 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - shared_merger_impl_( - fml::SharedThreadMergerImpl::GetSharedImpl(platform_queue_id, - gpu_queue_id)), + shared_merger_(fml::SharedThreadMerger::GetSharedMerger(platform_queue_id, + gpu_queue_id)), enabled_(true) {} void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { @@ -38,7 +37,7 @@ void RasterThreadMerger::MergeWithLease(int lease_term) { return; } - bool success = shared_merger_impl_->MergeWithLease(lease_term); + bool success = shared_merger_->MergeWithLease(lease_term); if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } @@ -55,7 +54,7 @@ void RasterThreadMerger::UnMergeNowIfLastOne() { if (!IsEnabledUnSafe()) { return; } - bool success = shared_merger_impl_->UnMergeNowIfLastOne(this); + bool success = shared_merger_->UnMergeNowIfLastOne(this); if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } @@ -79,7 +78,7 @@ void RasterThreadMerger::ExtendLeaseTo(int lease_term) { return; } std::scoped_lock lock(mutex_); - shared_merger_impl_->ExtendLeaseTo(lease_term); + shared_merger_->ExtendLeaseTo(lease_term); } bool RasterThreadMerger::IsMerged() { @@ -107,7 +106,7 @@ bool RasterThreadMerger::IsEnabledUnSafe() const { } bool RasterThreadMerger::IsMergedUnSafe() const { - return TaskQueuesAreSame() || shared_merger_impl_->IsMergedUnSafe(); + return TaskQueuesAreSame() || shared_merger_->IsMergedUnSafe(); } bool RasterThreadMerger::TaskQueuesAreSame() const { @@ -134,7 +133,7 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { if (!IsEnabledUnSafe()) { return RasterThreadStatus::kRemainsMerged; } - bool unmerged_after_decrement = shared_merger_impl_->DecrementLease(); + bool unmerged_after_decrement = shared_merger_->DecrementLease(); if (unmerged_after_decrement) { if (merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); @@ -147,7 +146,7 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { void RasterThreadMerger::RecordMergeCaller() { std::scoped_lock lock(mutex_); - shared_merger_impl_->RecordMergerCaller(this); + shared_merger_->RecordMergerCaller(this); } } // namespace fml diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 2f879aecaaf1c..f3da7cd4fdab6 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -11,7 +11,7 @@ #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" #include "flutter/fml/message_loop_task_queues.h" -#include "flutter/fml/shared_thread_merger_impl.h" +#include "flutter/fml/shared_thread_merger.h" namespace fml { @@ -60,7 +60,7 @@ class RasterThreadMerger // When task queues are statically merged this method becomes no-op. RasterThreadStatus DecrementLease(); - // Record current merge caller in the set of SharedThreadMergerImpl object. + // Record current merge caller in the set of SharedThreadMerger object. // This method should be called before multiple merge callers of same // owner/subsumed pair are going to call |MergeWithLease| method. // @@ -71,7 +71,7 @@ class RasterThreadMerger void RecordMergeCaller(); // The method is locked by current instance, and asks the shared instance of - // SharedThreadMergerImpl and the merging state is determined by the + // SharedThreadMerger and the merging state is determined by the // lease_term_ counter. bool IsMerged(); @@ -113,7 +113,7 @@ class RasterThreadMerger private: fml::TaskQueueId platform_queue_id_; fml::TaskQueueId gpu_queue_id_; - SharedThreadMergerImpl* shared_merger_impl_; + SharedThreadMerger* shared_merger_; std::condition_variable merged_condition_; std::mutex mutex_; fml::closure merge_unmerge_callback_; diff --git a/fml/shared_thread_merger_impl.cc b/fml/shared_thread_merger.cc similarity index 71% rename from fml/shared_thread_merger_impl.cc rename to fml/shared_thread_merger.cc index 5cbc192df329a..c82d6d0e42216 100644 --- a/fml/shared_thread_merger_impl.cc +++ b/fml/shared_thread_merger.cc @@ -4,21 +4,21 @@ #define FML_USED_ON_EMBEDDER -#include "flutter/fml/shared_thread_merger_impl.h" +#include "flutter/fml/shared_thread_merger.h" #include namespace fml { -std::mutex SharedThreadMergerImpl::creation_mutex_; +std::mutex SharedThreadMerger::creation_mutex_; // Guarded by creation_mutex_ -std::map - SharedThreadMergerImpl::shared_merger_instances_; +std::map + SharedThreadMerger::shared_merger_instances_; -const int SharedThreadMergerImpl::kLeaseNotSet = -1; +const int SharedThreadMerger::kLeaseNotSet = -1; -SharedThreadMergerImpl* SharedThreadMergerImpl::GetSharedImpl( +SharedThreadMerger* SharedThreadMerger::GetSharedMerger( fml::TaskQueueId owner, fml::TaskQueueId subsumed) { std::scoped_lock creation(creation_mutex_); @@ -27,25 +27,25 @@ SharedThreadMergerImpl* SharedThreadMergerImpl::GetSharedImpl( if (iter != shared_merger_instances_.end()) { return iter->second; } - auto merger = new SharedThreadMergerImpl(owner, subsumed); + auto merger = new SharedThreadMerger(owner, subsumed); shared_merger_instances_[key] = merger; return merger; } -SharedThreadMergerImpl::SharedThreadMergerImpl(fml::TaskQueueId owner, - fml::TaskQueueId subsumed) +SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner, + fml::TaskQueueId subsumed) : owner_(owner), subsumed_(subsumed), task_queues_(fml::MessageLoopTaskQueues::GetInstance()), lease_term_(kLeaseNotSet) {} -void SharedThreadMergerImpl::RecordMergerCaller(RasterThreadMerger* caller) { +void SharedThreadMerger::RecordMergerCaller(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); // Record current merge caller into callers set. merge_callers_.insert(caller); } -bool SharedThreadMergerImpl::MergeWithLease(int lease_term) { +bool SharedThreadMerger::MergeWithLease(int lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); if (IsMergedUnSafe()) { @@ -58,7 +58,7 @@ bool SharedThreadMergerImpl::MergeWithLease(int lease_term) { return success; } -bool SharedThreadMergerImpl::UnMergeNowUnSafe() { +bool SharedThreadMerger::UnMergeNowUnSafe() { FML_CHECK(lease_term_ == 0) << "lease_term_ must be 0 state before calling UnMergeNowUnSafe()"; bool success = task_queues_->Unmerge(owner_, subsumed_); @@ -66,7 +66,7 @@ bool SharedThreadMergerImpl::UnMergeNowUnSafe() { return success; } -bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger* caller) { +bool SharedThreadMerger::UnMergeNowIfLastOne(RasterThreadMerger* caller) { std::scoped_lock lock(mutex_); merge_callers_.erase(caller); if (!merge_callers_.empty()) { @@ -78,7 +78,7 @@ bool SharedThreadMergerImpl::UnMergeNowIfLastOne(RasterThreadMerger* caller) { return UnMergeNowUnSafe(); } -bool SharedThreadMergerImpl::DecrementLease() { +bool SharedThreadMerger::DecrementLease() { std::scoped_lock lock(mutex_); FML_DCHECK(lease_term_ > 0) << "lease_term should always be positive when merged."; @@ -91,7 +91,7 @@ bool SharedThreadMergerImpl::DecrementLease() { return false; } -void SharedThreadMergerImpl::ExtendLeaseTo(int lease_term) { +void SharedThreadMerger::ExtendLeaseTo(int lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); FML_DCHECK(IsMergedUnSafe()) @@ -101,7 +101,7 @@ void SharedThreadMergerImpl::ExtendLeaseTo(int lease_term) { } } -bool SharedThreadMergerImpl::IsMergedUnSafe() const { +bool SharedThreadMerger::IsMergedUnSafe() const { return lease_term_ > 0; } diff --git a/fml/shared_thread_merger_impl.h b/fml/shared_thread_merger.h similarity index 87% rename from fml/shared_thread_merger_impl.h rename to fml/shared_thread_merger.h index 73ed485ea8e34..fcc25cff7b24f 100644 --- a/fml/shared_thread_merger_impl.h +++ b/fml/shared_thread_merger.h @@ -29,12 +29,12 @@ struct ThreadMergerKey { } }; -class SharedThreadMergerImpl - : public fml::RefCountedThreadSafe { +class SharedThreadMerger + : public fml::RefCountedThreadSafe { public: - SharedThreadMergerImpl(TaskQueueId owner, TaskQueueId subsumed); - static SharedThreadMergerImpl* GetSharedImpl(TaskQueueId owner, - TaskQueueId subsumed); + SharedThreadMerger(TaskQueueId owner, TaskQueueId subsumed); + static SharedThreadMerger* GetSharedMerger(TaskQueueId owner, + TaskQueueId subsumed); // It's called by |RasterThreadMerger::RecordMergerCaller()|. // See the doc of |RasterThreadMerger::RecordMergerCaller()|. void RecordMergerCaller(RasterThreadMerger* caller); @@ -74,7 +74,7 @@ class SharedThreadMergerImpl static std::mutex creation_mutex_; // Guarded by creation_mutex_ - static std::map + static std::map shared_merger_instances_; bool UnMergeNowUnSafe(); }; From 8263643cde3c55b251f3bcff6baab34249e4e67e Mon Sep 17 00:00:00 2001 From: eggfly Date: Tue, 3 Aug 2021 16:51:46 +0800 Subject: [PATCH 21/24] Remove the static map, store the parent merger in shell instead. --- fml/raster_thread_merger.cc | 32 +++++++++++++++++++++++++-- fml/raster_thread_merger.h | 22 ++++++++++++++---- fml/raster_thread_merger_unittests.cc | 15 ++++++++----- fml/shared_thread_merger.cc | 24 ++------------------ fml/shared_thread_merger.h | 27 ++++++---------------- shell/common/rasterizer.cc | 8 +++++-- shell/common/rasterizer.h | 13 ++++++++++- shell/common/rasterizer_unittests.cc | 2 ++ shell/common/shell.cc | 20 +++++++++++++---- shell/common/shell.h | 14 ++++++++++++ 10 files changed, 116 insertions(+), 61 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 26c112977ca88..b9cd75dc99e42 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -12,16 +12,44 @@ namespace fml { RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, fml::TaskQueueId gpu_queue_id) + : RasterThreadMerger( + MakeRefCounted(platform_queue_id, gpu_queue_id), + platform_queue_id, + gpu_queue_id) {} + +RasterThreadMerger::RasterThreadMerger( + fml::RefPtr shared_merger, + fml::TaskQueueId platform_queue_id, + fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - shared_merger_(fml::SharedThreadMerger::GetSharedMerger(platform_queue_id, - gpu_queue_id)), + shared_merger_(shared_merger), enabled_(true) {} void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; } +fml::RefPtr +RasterThreadMerger::GetSharedRasterThreadMerger() const { + return shared_merger_; +} + +fml::RefPtr +RasterThreadMerger::CreateOrShareThreadMerger( + const fml::RefPtr& parent_merger, + TaskQueueId platform_id, + TaskQueueId raster_id) { + if (parent_merger && parent_merger->platform_queue_id_ == platform_id && + parent_merger->gpu_queue_id_ == raster_id) { + auto shared_merger = parent_merger->GetSharedRasterThreadMerger(); + return fml::MakeRefCounted(shared_merger, platform_id, + raster_id); + } else { + return fml::MakeRefCounted(platform_id, raster_id); + } +} + void RasterThreadMerger::MergeWithLease(int lease_term) { std::scoped_lock lock(mutex_); if (TaskQueuesAreSame()) { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index f3da7cd4fdab6..b887707672e0a 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -37,6 +37,16 @@ class RasterThreadMerger // When task queues are statically merged this method becomes no-op. void MergeWithLease(int lease_term); + // Gets the shared merger from current merger object + fml::RefPtr GetSharedRasterThreadMerger() const; + + // Creates a new merger from parent, share the inside shared_merger member + // when the platform_queue_id and raster_queue_id are same. + static fml::RefPtr CreateOrShareThreadMerger( + const fml::RefPtr& parent_merger, + TaskQueueId platform_id, + TaskQueueId raster_id); + // Un-merges the threads now if current caller is the last merge caller, // and it resets the lease term to 0, otherwise it will remove the caller // record and return. The multiple caller records were recorded by the @@ -80,9 +90,6 @@ class RasterThreadMerger // Must run on the platform task runner. void WaitUntilMerged(); - RasterThreadMerger(fml::TaskQueueId platform_queue_id, - fml::TaskQueueId gpu_queue_id); - // Returns true if the current thread owns rasterizing. // When the threads are merged, platform thread owns rasterizing. // When un-merged, raster thread owns rasterizing. @@ -113,7 +120,14 @@ class RasterThreadMerger private: fml::TaskQueueId platform_queue_id_; fml::TaskQueueId gpu_queue_id_; - SharedThreadMerger* shared_merger_; + + RasterThreadMerger(fml::TaskQueueId platform_queue_id, + fml::TaskQueueId gpu_queue_id); + RasterThreadMerger(fml::RefPtr shared_merger, + fml::TaskQueueId platform_queue_id, + fml::TaskQueueId gpu_queue_id); + + const fml::RefPtr shared_merger_; std::condition_variable merged_condition_; std::mutex mutex_; fml::closure merge_unmerge_callback_; diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index ade8f7214a77d..7da34e96e03ca 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -495,9 +495,10 @@ TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger1_ = - fml::MakeRefCounted(qid1, qid2); + fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); const auto raster_thread_merger2_ = - fml::MakeRefCounted(qid1, qid2); + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1_, + qid1, qid2); const int kNumFramesMerged = 5; ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); @@ -532,9 +533,10 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); const auto raster_thread_merger1_ = - fml::MakeRefCounted(qid1, qid2); + fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); const auto raster_thread_merger2_ = - fml::MakeRefCounted(qid1, qid2); + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1_, + qid1, qid2); const int kNumFramesMerged = 5; ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); @@ -578,9 +580,10 @@ TEST(RasterThreadMerger, fml::TaskQueueId qid3 = queue3.GetTaskQueueId(); const auto raster_thread_merger1_ = - fml::MakeRefCounted(qid1, qid2); + fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); const auto raster_thread_merger2_ = - fml::MakeRefCounted(qid1, qid3); + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1_, + qid1, qid3); const int kNumFramesMerged = 5; ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc index c82d6d0e42216..eb42f1a660ab7 100644 --- a/fml/shared_thread_merger.cc +++ b/fml/shared_thread_merger.cc @@ -10,28 +10,8 @@ namespace fml { -std::mutex SharedThreadMerger::creation_mutex_; - -// Guarded by creation_mutex_ -std::map - SharedThreadMerger::shared_merger_instances_; - const int SharedThreadMerger::kLeaseNotSet = -1; -SharedThreadMerger* SharedThreadMerger::GetSharedMerger( - fml::TaskQueueId owner, - fml::TaskQueueId subsumed) { - std::scoped_lock creation(creation_mutex_); - ThreadMergerKey key = {.owner = owner, .subsumed = subsumed}; - auto iter = shared_merger_instances_.find(key); - if (iter != shared_merger_instances_.end()) { - return iter->second; - } - auto merger = new SharedThreadMerger(owner, subsumed); - shared_merger_instances_[key] = merger; - return merger; -} - SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner, fml::TaskQueueId subsumed) : owner_(owner), @@ -39,7 +19,7 @@ SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner, task_queues_(fml::MessageLoopTaskQueues::GetInstance()), lease_term_(kLeaseNotSet) {} -void SharedThreadMerger::RecordMergerCaller(RasterThreadMerger* caller) { +void SharedThreadMerger::RecordMergerCaller(RasterThreadMergerId caller) { std::scoped_lock lock(mutex_); // Record current merge caller into callers set. merge_callers_.insert(caller); @@ -66,7 +46,7 @@ bool SharedThreadMerger::UnMergeNowUnSafe() { return success; } -bool SharedThreadMerger::UnMergeNowIfLastOne(RasterThreadMerger* caller) { +bool SharedThreadMerger::UnMergeNowIfLastOne(RasterThreadMergerId caller) { std::scoped_lock lock(mutex_); merge_callers_.erase(caller); if (!merge_callers_.empty()) { diff --git a/fml/shared_thread_merger.h b/fml/shared_thread_merger.h index fcc25cff7b24f..33c13ea459e6b 100644 --- a/fml/shared_thread_merger.h +++ b/fml/shared_thread_merger.h @@ -17,27 +17,16 @@ namespace fml { class MessageLoopImpl; class RasterThreadMerger; -struct ThreadMergerKey { - TaskQueueId owner; - TaskQueueId subsumed; - bool operator<(const ThreadMergerKey& other) const { - if (owner == other.owner) { - return subsumed < other.subsumed; - } else { - return owner < other.owner; - } - } -}; +typedef void* RasterThreadMergerId; class SharedThreadMerger : public fml::RefCountedThreadSafe { public: SharedThreadMerger(TaskQueueId owner, TaskQueueId subsumed); - static SharedThreadMerger* GetSharedMerger(TaskQueueId owner, - TaskQueueId subsumed); + // It's called by |RasterThreadMerger::RecordMergerCaller()|. // See the doc of |RasterThreadMerger::RecordMergerCaller()|. - void RecordMergerCaller(RasterThreadMerger* caller); + void RecordMergerCaller(RasterThreadMergerId caller); // It's called by |RasterThreadMerger::MergeWithLease()|. // See the doc of |RasterThreadMerger::MergeWithLease()|. @@ -45,7 +34,7 @@ class SharedThreadMerger // It's called by |RasterThreadMerger::UnMergeNowIfLastOne()|. // See the doc of |RasterThreadMerger::UnMergeNowIfLastOne()|. - bool UnMergeNowIfLastOne(RasterThreadMerger* caller); + bool UnMergeNowIfLastOne(RasterThreadMergerId caller); // It's called by |RasterThreadMerger::ExtendLeaseTo()|. // See the doc of |RasterThreadMerger::ExtendLeaseTo()|. @@ -70,13 +59,11 @@ class SharedThreadMerger /// The |RecordMergerCaller| method will record the caller /// into this merge_callers_ set, |UnMergeNowIfLastOne()| /// method will remove the caller from this merge_callers_. - std::set merge_callers_; + std::set merge_callers_; - static std::mutex creation_mutex_; - // Guarded by creation_mutex_ - static std::map - shared_merger_instances_; bool UnMergeNowUnSafe(); + + FML_DISALLOW_COPY_AND_ASSIGN(SharedThreadMerger); }; } // namespace fml diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 8404f2d57ae74..502e04c4416ae 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -68,8 +68,8 @@ void Rasterizer::Setup(std::unique_ptr surface) { delegate_.GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId(); const auto gpu_id = delegate_.GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(); - raster_thread_merger_ = - fml::MakeRefCounted(platform_id, gpu_id); + raster_thread_merger_ = fml::RasterThreadMerger::CreateOrShareThreadMerger( + delegate_.GetParentRasterThreadMerger(), platform_id, gpu_id); } if (raster_thread_merger_) { raster_thread_merger_->SetMergeUnmergeCallback([=]() { @@ -706,6 +706,10 @@ void Rasterizer::SetSnapshotSurfaceProducer( snapshot_surface_producer_ = std::move(producer); } +fml::RefPtr Rasterizer::GetRasterThreadMerger() { + return raster_thread_merger_; +} + void Rasterizer::FireNextFrameCallbackIfPresent() { if (!next_frame_callback_) { return; diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index d6b023acec0e9..15beadf5511a5 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -84,6 +84,10 @@ class Rasterizer final : public SnapshotDelegate { /// Task runners used by the shell. virtual const TaskRunners& GetTaskRunners() const = 0; + /// The raster thread merger from parent shell's rasterizer. + virtual const fml::RefPtr + GetParentRasterThreadMerger() const = 0; + /// Accessor for the shell's GPU sync switch, which determines whether GPU /// operations are allowed on the current thread. /// @@ -371,6 +375,14 @@ class Rasterizer final : public SnapshotDelegate { return compositor_context_.get(); } + //---------------------------------------------------------------------------- + /// @brief Returns the raster thread merger used by this rasterizer. + /// This may be `nullptr`. + /// + /// @return The raster thread merger used by this rasterizer. + /// + fml::RefPtr GetRasterThreadMerger(); + //---------------------------------------------------------------------------- /// @brief Skia has no notion of time. To work around the performance /// implications of this, it may cache GPU resources to reference @@ -451,7 +463,6 @@ class Rasterizer final : public SnapshotDelegate { fml::RefPtr raster_thread_merger_; fml::TaskRunnerAffineWeakPtrFactory weak_factory_; std::shared_ptr external_view_embedder_; - // |SnapshotDelegate| sk_sp MakeRasterSnapshot( std::function draw_callback, diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index cb54fef27a097..3bd3f82465285 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -28,6 +28,8 @@ class MockDelegate : public Rasterizer::Delegate { MOCK_METHOD0(GetFrameBudget, fml::Milliseconds()); MOCK_CONST_METHOD0(GetLatestFrameTargetTime, fml::TimePoint()); MOCK_CONST_METHOD0(GetTaskRunners, const TaskRunners&()); + MOCK_CONST_METHOD0(GetParentRasterThreadMerger, + const fml::RefPtr()); MOCK_CONST_METHOD0(GetIsGpuDisabledSyncSwitch, std::shared_ptr()); MOCK_METHOD0(CreateSnapshotSurface, std::unique_ptr()); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 8a789f70cc133..3d8000651f205 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -149,6 +149,7 @@ std::unique_ptr Shell::Create( } return CreateWithSnapshot(std::move(platform_data), // std::move(task_runners), // + nullptr, // std::move(settings), // std::move(vm), // std::move(isolate_snapshot), // @@ -159,6 +160,7 @@ std::unique_ptr Shell::Create( std::unique_ptr Shell::CreateShellOnPlatformThread( DartVMRef vm, + fml::RefPtr parent_merger, TaskRunners task_runners, const PlatformData& platform_data, Settings settings, @@ -173,7 +175,7 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( } auto shell = std::unique_ptr( - new Shell(std::move(vm), task_runners, settings, + new Shell(std::move(vm), task_runners, parent_merger, settings, std::make_shared( task_runners.GetUITaskRunner(), !settings.skia_deterministic_rendering_on_cpu), @@ -301,6 +303,7 @@ std::unique_ptr Shell::CreateShellOnPlatformThread( std::unique_ptr Shell::CreateWithSnapshot( const PlatformData& platform_data, TaskRunners task_runners, + fml::RefPtr parent_thread_merger, Settings settings, DartVMRef vm, fml::RefPtr isolate_snapshot, @@ -326,6 +329,7 @@ std::unique_ptr Shell::CreateWithSnapshot( fml::MakeCopyable( [&latch, // &shell, // + parent_thread_merger, // task_runners = std::move(task_runners), // platform_data = std::move(platform_data), // settings = std::move(settings), // @@ -337,6 +341,7 @@ std::unique_ptr Shell::CreateWithSnapshot( is_gpu_disabled]() mutable { shell = CreateShellOnPlatformThread( std::move(vm), // + parent_thread_merger, // std::move(task_runners), // std::move(platform_data), // std::move(settings), // @@ -352,10 +357,12 @@ std::unique_ptr Shell::CreateWithSnapshot( Shell::Shell(DartVMRef vm, TaskRunners task_runners, + fml::RefPtr parent_merger, Settings settings, std::shared_ptr volatile_path_tracker, bool is_gpu_disabled) : task_runners_(std::move(task_runners)), + parent_raster_thread_merger_(parent_merger), settings_(std::move(settings)), vm_(std::move(vm)), is_gpu_disabled_sync_switch_(new fml::SyncSwitch(is_gpu_disabled)), @@ -477,9 +484,9 @@ std::unique_ptr Shell::Spawn( FML_DCHECK(task_runners_.IsValid()); auto shell_maker = [&](bool is_gpu_disabled) { std::unique_ptr result(CreateWithSnapshot( - PlatformData{}, task_runners_, GetSettings(), vm_, - vm_->GetVMData()->GetIsolateSnapshot(), on_create_platform_view, - on_create_rasterizer, + PlatformData{}, task_runners_, rasterizer_->GetRasterThreadMerger(), + GetSettings(), vm_, vm_->GetVMData()->GetIsolateSnapshot(), + on_create_platform_view, on_create_rasterizer, [engine = this->engine_.get()]( Engine::Delegate& delegate, const PointerDataDispatcherMaker& dispatcher_maker, DartVM& vm, @@ -662,6 +669,11 @@ const TaskRunners& Shell::GetTaskRunners() const { return task_runners_; } +const fml::RefPtr Shell::GetParentRasterThreadMerger() + const { + return parent_raster_thread_merger_; +} + fml::TaskRunnerAffineWeakPtr Shell::GetRasterizer() const { FML_DCHECK(is_setup_); return weak_rasterizer_; diff --git a/shell/common/shell.h b/shell/common/shell.h index 0ad74798717cc..23754936c4e81 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -229,6 +229,16 @@ class Shell final : public PlatformView::Delegate, /// const TaskRunners& GetTaskRunners() const override; + //------------------------------------------------------------------------------ + /// @brief Getting the raster thread merger from parent shell, it can be + /// a null RefPtr when it's a root Shell or the + /// embedder_->SupportsDynamicThreadMerging() returns false. + /// + /// @return The raster thread merger used by the parent shell. + /// + const fml::RefPtr GetParentRasterThreadMerger() + const override; + //---------------------------------------------------------------------------- /// @brief Rasterizers may only be accessed on the raster task runner. /// @@ -390,6 +400,7 @@ class Shell final : public PlatformView::Delegate, rapidjson::Document*)>; const TaskRunners task_runners_; + const fml::RefPtr parent_raster_thread_merger_; const Settings settings_; DartVMRef vm_; mutable std::mutex time_recorder_mutex_; @@ -453,12 +464,14 @@ class Shell final : public PlatformView::Delegate, Shell(DartVMRef vm, TaskRunners task_runners, + fml::RefPtr parent_merger, Settings settings, std::shared_ptr volatile_path_tracker, bool is_gpu_disabled); static std::unique_ptr CreateShellOnPlatformThread( DartVMRef vm, + fml::RefPtr parent_merger, TaskRunners task_runners, const PlatformData& platform_data, Settings settings, @@ -470,6 +483,7 @@ class Shell final : public PlatformView::Delegate, static std::unique_ptr CreateWithSnapshot( const PlatformData& platform_data, TaskRunners task_runners, + fml::RefPtr parent_thread_merger, Settings settings, DartVMRef vm, fml::RefPtr isolate_snapshot, From 1d48d1574d7e146fa29a742c732ea97b90146031 Mon Sep 17 00:00:00 2001 From: eggfly Date: Thu, 5 Aug 2021 16:04:48 +0800 Subject: [PATCH 22/24] Let every merger has their own lease_term to avoid multi-decrement bug Remove RecordMergeCaller() method. --- fml/raster_thread_merger.cc | 14 ++--- fml/raster_thread_merger.h | 16 +---- fml/raster_thread_merger_unittests.cc | 39 +++++------- fml/shared_thread_merger.cc | 60 +++++++++---------- fml/shared_thread_merger.h | 21 +++---- .../external_view_embedder.cc | 2 - 6 files changed, 64 insertions(+), 88 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index b9cd75dc99e42..f6a4e2c0187b7 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -65,7 +65,7 @@ void RasterThreadMerger::MergeWithLease(int lease_term) { return; } - bool success = shared_merger_->MergeWithLease(lease_term); + bool success = shared_merger_->MergeWithLease(this, lease_term); if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } @@ -106,7 +106,10 @@ void RasterThreadMerger::ExtendLeaseTo(int lease_term) { return; } std::scoped_lock lock(mutex_); - shared_merger_->ExtendLeaseTo(lease_term); + if (!IsEnabledUnSafe()) { + return; + } + shared_merger_->ExtendLeaseTo(this, lease_term); } bool RasterThreadMerger::IsMerged() { @@ -161,7 +164,7 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { if (!IsEnabledUnSafe()) { return RasterThreadStatus::kRemainsMerged; } - bool unmerged_after_decrement = shared_merger_->DecrementLease(); + bool unmerged_after_decrement = shared_merger_->DecrementLease(this); if (unmerged_after_decrement) { if (merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); @@ -172,9 +175,4 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { return RasterThreadStatus::kRemainsMerged; } -void RasterThreadMerger::RecordMergeCaller() { - std::scoped_lock lock(mutex_); - shared_merger_->RecordMergerCaller(this); -} - } // namespace fml diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index b887707672e0a..7fcc45d8ac8d4 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -49,8 +49,8 @@ class RasterThreadMerger // Un-merges the threads now if current caller is the last merge caller, // and it resets the lease term to 0, otherwise it will remove the caller - // record and return. The multiple caller records were recorded by the - // |RecordMergeCaller| method. + // record and return. The multiple caller records were recorded after + // |MergeWithLease| or |ExtendLeaseTo| method. // // Must be executed on the raster task runner. // @@ -70,16 +70,6 @@ class RasterThreadMerger // When task queues are statically merged this method becomes no-op. RasterThreadStatus DecrementLease(); - // Record current merge caller in the set of SharedThreadMerger object. - // This method should be called before multiple merge callers of same - // owner/subsumed pair are going to call |MergeWithLease| method. - // - // And the reason why not putting this method inside |MergeWithLease| is - // |MergeWithLease| should not called when another caller already merged the - // threads and |IsMerged| return true. In this case only recording the - // current caller is needed. - void RecordMergeCaller(); - // The method is locked by current instance, and asks the shared instance of // SharedThreadMerger and the merging state is determined by the // lease_term_ counter. @@ -107,7 +97,7 @@ class RasterThreadMerger // Whether the thread merger is enabled. By default, the thread merger is // enabled. If false, calls to |MergeWithLease| or |UnMergeNowIfLastOne| - // results in a noop. + // or |ExtendLeaseTo| or |DecrementLease| results in a noop. bool IsEnabled(); // Registers a callback that can be used to clean up global state right after diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 7da34e96e03ca..6ffcf538dcb01 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -503,24 +503,32 @@ TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - // Merge using the second merger - raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + // Merge using the first merger + raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); // let there be one more turn till the leases expire. for (int i = 0; i < kNumFramesMerged - 1; i++) { - // Check merge state using the first merger + // Check merge state using the two merger ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); raster_thread_merger1_->DecrementLease(); } - // extend the lease once using the first merger + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); + ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + + // extend the lease once with the first merger raster_thread_merger1_->ExtendLeaseTo(kNumFramesMerged); // we will NOT last for 1 extra turn, we just set it. for (int i = 0; i < kNumFramesMerged; i++) { - // Check merge state using the second merger + // Check merge state using the two merger + ASSERT_TRUE(raster_thread_merger1_->IsMerged()); ASSERT_TRUE(raster_thread_merger2_->IsMerged()); - raster_thread_merger2_->DecrementLease(); + raster_thread_merger1_->DecrementLease(); } ASSERT_FALSE(raster_thread_merger1_->IsMerged()); @@ -541,19 +549,12 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - // Recording mergers themselves is needed. - raster_thread_merger1_->RecordMergeCaller(); - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - raster_thread_merger2_->RecordMergeCaller(); - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - // Merge using the mergers raster_thread_merger1_->MergeWithLease(kNumFramesMerged); ASSERT_TRUE(raster_thread_merger1_->IsMerged()); ASSERT_TRUE(raster_thread_merger2_->IsMerged()); - raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + // Extend the second merger's lease + raster_thread_merger2_->ExtendLeaseTo(kNumFramesMerged); ASSERT_TRUE(raster_thread_merger1_->IsMerged()); ASSERT_TRUE(raster_thread_merger2_->IsMerged()); @@ -612,14 +613,6 @@ TEST(RasterThreadMerger, ASSERT_FALSE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - // test caller records are separated below - raster_thread_merger1_->RecordMergeCaller(); - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - raster_thread_merger2_->RecordMergeCaller(); - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - raster_thread_merger1_->MergeWithLease(kNumFramesMerged); ASSERT_TRUE(raster_thread_merger1_->IsMerged()); ASSERT_FALSE(raster_thread_merger2_->IsMerged()); diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc index eb42f1a660ab7..cf33a5d646113 100644 --- a/fml/shared_thread_merger.cc +++ b/fml/shared_thread_merger.cc @@ -10,22 +10,14 @@ namespace fml { -const int SharedThreadMerger::kLeaseNotSet = -1; - SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner, fml::TaskQueueId subsumed) : owner_(owner), subsumed_(subsumed), - task_queues_(fml::MessageLoopTaskQueues::GetInstance()), - lease_term_(kLeaseNotSet) {} + task_queues_(fml::MessageLoopTaskQueues::GetInstance()) {} -void SharedThreadMerger::RecordMergerCaller(RasterThreadMergerId caller) { - std::scoped_lock lock(mutex_); - // Record current merge caller into callers set. - merge_callers_.insert(caller); -} - -bool SharedThreadMerger::MergeWithLease(int lease_term) { +bool SharedThreadMerger::MergeWithLease(RasterThreadMergerId caller, + int lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); if (IsMergedUnSafe()) { @@ -33,14 +25,15 @@ bool SharedThreadMerger::MergeWithLease(int lease_term) { } bool success = task_queues_->Merge(owner_, subsumed_); FML_CHECK(success) << "Unable to merge the raster and platform threads."; - - lease_term_ = lease_term; + // Save the lease term + lease_term_by_caller_[caller] = lease_term; return success; } bool SharedThreadMerger::UnMergeNowUnSafe() { - FML_CHECK(lease_term_ == 0) - << "lease_term_ must be 0 state before calling UnMergeNowUnSafe()"; + FML_CHECK(IsAllLeaseTermsZeroUnSafe()) + << "all lease term records must be zero before calling " + "UnMergeNowUnSafe()"; bool success = task_queues_->Unmerge(owner_, subsumed_); FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; return success; @@ -48,22 +41,25 @@ bool SharedThreadMerger::UnMergeNowUnSafe() { bool SharedThreadMerger::UnMergeNowIfLastOne(RasterThreadMergerId caller) { std::scoped_lock lock(mutex_); - merge_callers_.erase(caller); - if (!merge_callers_.empty()) { + lease_term_by_caller_.erase(caller); + if (!lease_term_by_caller_.empty()) { return true; } - FML_CHECK(IsMergedUnSafe()) - << "UnMergeNowIfLastOne() must be called only when threads are merged"; - lease_term_ = 0; // mark need unmerge now return UnMergeNowUnSafe(); } -bool SharedThreadMerger::DecrementLease() { +bool SharedThreadMerger::DecrementLease(RasterThreadMergerId caller) { std::scoped_lock lock(mutex_); - FML_DCHECK(lease_term_ > 0) - << "lease_term should always be positive when merged."; - lease_term_--; - if (lease_term_ == 0) { + auto entry = lease_term_by_caller_.find(caller); + bool exist = entry != lease_term_by_caller_.end(); + if (exist) { + std::atomic_int& lease_term_ref = entry->second; + FML_CHECK(lease_term_ref > 0) + << "lease_term should always be positive when merged, lease_term=" + << lease_term_ref; + lease_term_ref--; + } + if (IsAllLeaseTermsZeroUnSafe()) { // Unmerge now because lease_term_ decreased to zero. UnMergeNowUnSafe(); return true; @@ -71,18 +67,22 @@ bool SharedThreadMerger::DecrementLease() { return false; } -void SharedThreadMerger::ExtendLeaseTo(int lease_term) { +void SharedThreadMerger::ExtendLeaseTo(RasterThreadMergerId caller, + int lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); FML_DCHECK(IsMergedUnSafe()) << "should be merged state when calling this method"; - if (lease_term_ != kLeaseNotSet && lease_term > lease_term_) { - lease_term_ = lease_term; - } + lease_term_by_caller_[caller] = lease_term; } bool SharedThreadMerger::IsMergedUnSafe() const { - return lease_term_ > 0; + return !IsAllLeaseTermsZeroUnSafe(); +} + +bool SharedThreadMerger::IsAllLeaseTermsZeroUnSafe() const { + return std::all_of(lease_term_by_caller_.begin(), lease_term_by_caller_.end(), + [&](const auto& item) { return item.second == 0; }); } } // namespace fml diff --git a/fml/shared_thread_merger.h b/fml/shared_thread_merger.h index 33c13ea459e6b..2c4ec1687b167 100644 --- a/fml/shared_thread_merger.h +++ b/fml/shared_thread_merger.h @@ -24,13 +24,9 @@ class SharedThreadMerger public: SharedThreadMerger(TaskQueueId owner, TaskQueueId subsumed); - // It's called by |RasterThreadMerger::RecordMergerCaller()|. - // See the doc of |RasterThreadMerger::RecordMergerCaller()|. - void RecordMergerCaller(RasterThreadMergerId caller); - // It's called by |RasterThreadMerger::MergeWithLease()|. // See the doc of |RasterThreadMerger::MergeWithLease()|. - bool MergeWithLease(int lease_term); + bool MergeWithLease(RasterThreadMergerId caller, int lease_term); // It's called by |RasterThreadMerger::UnMergeNowIfLastOne()|. // See the doc of |RasterThreadMerger::UnMergeNowIfLastOne()|. @@ -38,7 +34,7 @@ class SharedThreadMerger // It's called by |RasterThreadMerger::ExtendLeaseTo()|. // See the doc of |RasterThreadMerger::ExtendLeaseTo()|. - void ExtendLeaseTo(int lease_term); + void ExtendLeaseTo(RasterThreadMergerId caller, int lease_term); // It's called by |RasterThreadMerger::IsMergedUnSafe()|. // See the doc of |RasterThreadMerger::IsMergedUnSafe()|. @@ -46,20 +42,21 @@ class SharedThreadMerger // It's called by |RasterThreadMerger::DecrementLease()|. // See the doc of |RasterThreadMerger::DecrementLease()|. - bool DecrementLease(); + bool DecrementLease(RasterThreadMergerId caller); private: static const int kLeaseNotSet; fml::TaskQueueId owner_; fml::TaskQueueId subsumed_; fml::RefPtr task_queues_; - std::atomic_int lease_term_; std::mutex mutex_; - /// The |RecordMergerCaller| method will record the caller - /// into this merge_callers_ set, |UnMergeNowIfLastOne()| - /// method will remove the caller from this merge_callers_. - std::set merge_callers_; + /// The |MergeWithLease| or |ExtendLeaseTo| method will record the caller + /// into this lease_term_by_caller_ map, |UnMergeNowIfLastOne()| + /// method will remove the caller from this lease_term_by_caller_. + std::map lease_term_by_caller_; + + bool IsAllLeaseTermsZeroUnSafe() const; bool UnMergeNowUnSafe(); diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc index b1c2be3dadad9..070e54fd70551 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -216,8 +216,6 @@ PostPrerollResult AndroidExternalViewEmbedder::PostPrerollAction( if (!FrameHasPlatformLayers()) { return PostPrerollResult::kSuccess; } - // Record merger firstly when frame has platform layers - raster_thread_merger->RecordMergeCaller(); if (!raster_thread_merger->IsMerged()) { // The raster thread merger may be disabled if the rasterizer is being // created or teared down. From 116a022fa0b50590d3f237d0030fe93d503e94d6 Mon Sep 17 00:00:00 2001 From: eggfly Date: Thu, 5 Aug 2021 20:57:34 +0800 Subject: [PATCH 23/24] Remove unused const declaration --- fml/shared_thread_merger.h | 1 - 1 file changed, 1 deletion(-) diff --git a/fml/shared_thread_merger.h b/fml/shared_thread_merger.h index 2c4ec1687b167..80aaa9dea50ea 100644 --- a/fml/shared_thread_merger.h +++ b/fml/shared_thread_merger.h @@ -45,7 +45,6 @@ class SharedThreadMerger bool DecrementLease(RasterThreadMergerId caller); private: - static const int kLeaseNotSet; fml::TaskQueueId owner_; fml::TaskQueueId subsumed_; fml::RefPtr task_queues_; From 9f25cb3f855b8be62682b9c0456847fe0f30cb77 Mon Sep 17 00:00:00 2001 From: eggfly Date: Fri, 6 Aug 2021 16:30:23 +0800 Subject: [PATCH 24/24] Fix some nits and write better doc after code review --- fml/memory/task_runner_checker_unittest.cc | 4 +- fml/memory/weak_ptr_unittest.cc | 4 +- fml/message_loop_task_queues.cc | 29 +- fml/message_loop_task_queues.h | 8 +- fml/message_loop_task_queues_unittests.cc | 4 +- fml/raster_thread_merger.cc | 6 +- fml/raster_thread_merger.h | 16 +- fml/raster_thread_merger_unittests.cc | 345 +++++++++++---------- fml/shared_thread_merger.cc | 11 +- fml/shared_thread_merger.h | 32 +- shell/common/shell.cc | 2 +- 11 files changed, 239 insertions(+), 222 deletions(-) diff --git a/fml/memory/task_runner_checker_unittest.cc b/fml/memory/task_runner_checker_unittest.cc index 90c896d3827d4..6196384869a5a 100644 --- a/fml/memory/task_runner_checker_unittest.cc +++ b/fml/memory/task_runner_checker_unittest.cc @@ -94,14 +94,14 @@ TEST(TaskRunnerCheckerTests, MergedTaskRunnersRunsOnTheSameThread) { fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); - const int kNumFramesMerged = 5; + const size_t kNumFramesMerged = 5; raster_thread_merger_->MergeWithLease(kNumFramesMerged); // merged, running on the same thread EXPECT_EQ(TaskRunnerChecker::RunsOnTheSameThread(qid1, qid2), true); - for (int i = 0; i < kNumFramesMerged; i++) { + for (size_t i = 0; i < kNumFramesMerged; i++) { ASSERT_TRUE(raster_thread_merger_->IsMerged()); raster_thread_merger_->DecrementLease(); } diff --git a/fml/memory/weak_ptr_unittest.cc b/fml/memory/weak_ptr_unittest.cc index e055ad9095409..2ed0b9ae008c5 100644 --- a/fml/memory/weak_ptr_unittest.cc +++ b/fml/memory/weak_ptr_unittest.cc @@ -212,14 +212,14 @@ TEST(TaskRunnerAffineWeakPtrTest, ShouldNotCrashIfRunningOnTheSameTaskRunner) { fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); const auto raster_thread_merger_ = fml::MakeRefCounted(qid1, qid2); - const int kNumFramesMerged = 5; + const size_t kNumFramesMerged = 5; raster_thread_merger_->MergeWithLease(kNumFramesMerged); loop2_task_start_latch.Signal(); loop2_task_finish_latch.Wait(); - for (int i = 0; i < kNumFramesMerged; i++) { + for (size_t i = 0; i < kNumFramesMerged; i++) { ASSERT_TRUE(raster_thread_merger_->IsMerged()); raster_thread_merger_->DecrementLease(); } diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 093ef5190d801..e0146c6ef7c63 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -25,7 +25,7 @@ fml::RefPtr MessageLoopTaskQueues::instance_; namespace { -// iOS prior to version 9 prevents c++11 thread_local and __thread specefier, +// iOS prior to version 9 prevents c++11 thread_local and __thread specifier, // having us resort to boxed enum containers. class TaskSourceGradeHolder { public: @@ -361,12 +361,10 @@ bool MessageLoopTaskQueues::HasPendingTasksUnlocked( } auto& subsumed_set = entry->owner_of; - for (auto& subsumed : subsumed_set) { - if (!queue_entries_.at(subsumed)->task_source->IsEmpty()) { - return true; - } - } - return false; + return std::any_of( + subsumed_set.begin(), subsumed_set.end(), [&](const auto& subsumed) { + return !queue_entries_.at(subsumed)->task_source->IsEmpty(); + }); } fml::TimePoint MessageLoopTaskQueues::GetNextWakeTimeUnlocked( @@ -386,14 +384,15 @@ TaskSource::TopTask MessageLoopTaskQueues::PeekNextTaskUnlocked( // Use optional for the memory of TopTask object. std::optional top_task; - auto top_task_updater = [&top_task](const TaskSource* source) { - if (source && !source->IsEmpty()) { - auto other_task = source->Top(); - if (!top_task.has_value() || top_task->task > other_task.task) { - top_task.emplace(other_task); - } - } - }; + std::function top_task_updater = + [&top_task](const TaskSource* source) { + if (source && !source->IsEmpty()) { + TaskSource::TopTask other_task = source->Top(); + if (!top_task.has_value() || top_task->task > other_task.task) { + top_task.emplace(other_task); + } + } + }; TaskSource* owner_tasks = entry->task_source.get(); top_task_updater(owner_tasks); diff --git a/fml/message_loop_task_queues.h b/fml/message_loop_task_queues.h index 34a62f5eaa9e7..703c6bcec3b58 100644 --- a/fml/message_loop_task_queues.h +++ b/fml/message_loop_task_queues.h @@ -35,12 +35,12 @@ class TaskQueueEntry { TaskObservers task_observers; std::unique_ptr task_source; - // Set of TaskQueueIds and in-turn the TaskQueues owned by this TaskQueue. If - // the set is empty, this TaskQueue does not own any other TaskQueues. + /// Set of the TaskQueueIds which is owned by this TaskQueue. If the set is + /// empty, this TaskQueue does not own any other TaskQueues. std::set owner_of; - // Identifies the TaskQueue that subsumes this TaskQueue. If it is _kUnmerged, - // it indiacates that this TaskQueue is not owned by any other TaskQueue. + /// Identifies the TaskQueue that subsumes this TaskQueue. If it is _kUnmerged + /// it indicates that this TaskQueue is not owned by any other TaskQueue. TaskQueueId subsumed_by; TaskQueueId created_for; diff --git a/fml/message_loop_task_queues_unittests.cc b/fml/message_loop_task_queues_unittests.cc index 830a663dd7e6b..48f6fb871ef90 100644 --- a/fml/message_loop_task_queues_unittests.cc +++ b/fml/message_loop_task_queues_unittests.cc @@ -107,7 +107,7 @@ TEST(MessageLoopTaskQueue, PreserveTaskOrdering) { const auto now = ChronoTicksSinceEpoch(); int expected_value = 1; - for (;;) { + while (true) { fml::closure invocation = task_queue->GetNextTaskToRun(queue_id, now); if (!invocation) { break; @@ -147,7 +147,7 @@ TEST(MessageLoopTaskQueue, RegisterTasksOnMergedQueuesPreserveTaskOrdering) { // "test_val = 0" in raster1_queue // "test_val = 1" in platform_queue // "test_val = 2" in raster2_queue - for (;;) { + while (true) { fml::closure invocation = task_queue->GetNextTaskToRun(platform_queue, now); if (!invocation) { break; diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index f6a4e2c0187b7..f14d64ed8ac28 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -30,7 +30,7 @@ void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; } -fml::RefPtr +const fml::RefPtr& RasterThreadMerger::GetSharedRasterThreadMerger() const { return shared_merger_; } @@ -50,7 +50,7 @@ RasterThreadMerger::CreateOrShareThreadMerger( } } -void RasterThreadMerger::MergeWithLease(int lease_term) { +void RasterThreadMerger::MergeWithLease(size_t lease_term) { std::scoped_lock lock(mutex_); if (TaskQueuesAreSame()) { return; @@ -100,7 +100,7 @@ bool RasterThreadMerger::IsOnRasterizingThread() const { } } -void RasterThreadMerger::ExtendLeaseTo(int lease_term) { +void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; if (TaskQueuesAreSame()) { return; diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 7fcc45d8ac8d4..bcad1904e1513 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -23,6 +23,11 @@ enum class RasterThreadStatus { kUnmergedNow }; +/// This class is a client and proxy between the rasterizer and +/// |SharedThreadMerger|. The multiple |RasterThreadMerger| instances with same +/// owner_queue_id and same subsumed_queue_id share the same +/// |SharedThreadMerger| instance. Whether they share the same inner instance is +/// determined by |RasterThreadMerger::CreateOrShareThreadMerger| method. class RasterThreadMerger : public fml::RefCountedThreadSafe { public: @@ -35,13 +40,14 @@ class RasterThreadMerger // // If the task queues are the same, we consider them statically merged. // When task queues are statically merged this method becomes no-op. - void MergeWithLease(int lease_term); + void MergeWithLease(size_t lease_term); // Gets the shared merger from current merger object - fml::RefPtr GetSharedRasterThreadMerger() const; + const fml::RefPtr& GetSharedRasterThreadMerger() const; - // Creates a new merger from parent, share the inside shared_merger member - // when the platform_queue_id and raster_queue_id are same. + /// Creates a new merger from parent, share the inside shared_merger member + /// when the platform_queue_id and raster_queue_id are same, otherwise create + /// a new shared_merger instance static fml::RefPtr CreateOrShareThreadMerger( const fml::RefPtr& parent_merger, TaskQueueId platform_id, @@ -61,7 +67,7 @@ class RasterThreadMerger // If the task queues are the same, we consider them statically merged. // When task queues are statically merged this method becomes no-op. - void ExtendLeaseTo(int lease_term); + void ExtendLeaseTo(size_t lease_term); // Returns |RasterThreadStatus::kUnmergedNow| if this call resulted in // splitting the raster and platform threads. Reduces the lease term by 1. diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 6ffcf538dcb01..339cd174752dd 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -60,20 +60,20 @@ TEST(RasterThreadMerger, RemainMergedTillLeaseExpires) { TaskQueueWrapper queue2; fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); - const int kNumFramesMerged = 5; + const size_t kNumFramesMerged = 5; - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->MergeWithLease(kNumFramesMerged); + raster_thread_merger->MergeWithLease(kNumFramesMerged); - for (int i = 0; i < kNumFramesMerged; i++) { - ASSERT_TRUE(raster_thread_merger_->IsMerged()); - raster_thread_merger_->DecrementLease(); + for (size_t i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger->IsMerged()); + raster_thread_merger->DecrementLease(); } - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + ASSERT_FALSE(raster_thread_merger->IsMerged()); } TEST(RasterThreadMerger, IsNotOnRasterizingThread) { @@ -100,32 +100,32 @@ TEST(RasterThreadMerger, IsNotOnRasterizingThread) { fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); fml::CountDownLatch pre_merge(2), post_merge(2), post_unmerge(2); loop1->GetTaskRunner()->PostTask([&]() { - ASSERT_FALSE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_TRUE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_FALSE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid1); pre_merge.CountDown(); }); loop2->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_FALSE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid2); pre_merge.CountDown(); }); pre_merge.Wait(); - raster_thread_merger_->MergeWithLease(1); + raster_thread_merger->MergeWithLease(1); loop1->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_TRUE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid1); post_merge.CountDown(); }); @@ -133,26 +133,26 @@ TEST(RasterThreadMerger, IsNotOnRasterizingThread) { loop2->GetTaskRunner()->PostTask([&]() { // this will be false since this is going to be run // on loop1 really. - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_TRUE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid1); post_merge.CountDown(); }); post_merge.Wait(); - raster_thread_merger_->DecrementLease(); + raster_thread_merger->DecrementLease(); loop1->GetTaskRunner()->PostTask([&]() { - ASSERT_FALSE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_TRUE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_FALSE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid1); post_unmerge.CountDown(); }); loop2->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_FALSE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid2); post_unmerge.CountDown(); }); @@ -173,30 +173,30 @@ TEST(RasterThreadMerger, LeaseExtension) { fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); - const int kNumFramesMerged = 5; + const size_t kNumFramesMerged = 5; - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->MergeWithLease(kNumFramesMerged); + raster_thread_merger->MergeWithLease(kNumFramesMerged); // let there be one more turn till the leases expire. - for (int i = 0; i < kNumFramesMerged - 1; i++) { - ASSERT_TRUE(raster_thread_merger_->IsMerged()); - raster_thread_merger_->DecrementLease(); + for (size_t i = 0; i < kNumFramesMerged - 1; i++) { + ASSERT_TRUE(raster_thread_merger->IsMerged()); + raster_thread_merger->DecrementLease(); } // extend the lease once. - raster_thread_merger_->ExtendLeaseTo(kNumFramesMerged); + raster_thread_merger->ExtendLeaseTo(kNumFramesMerged); // we will NOT last for 1 extra turn, we just set it. - for (int i = 0; i < kNumFramesMerged; i++) { - ASSERT_TRUE(raster_thread_merger_->IsMerged()); - raster_thread_merger_->DecrementLease(); + for (size_t i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger->IsMerged()); + raster_thread_merger->DecrementLease(); } - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + ASSERT_FALSE(raster_thread_merger->IsMerged()); } TEST(RasterThreadMerger, WaitUntilMerged) { @@ -217,7 +217,7 @@ TEST(RasterThreadMerger, WaitUntilMerged) { term_platform.Wait(); }); - const int kNumFramesMerged = 5; + const size_t kNumFramesMerged = 5; fml::MessageLoop* loop_raster = nullptr; fml::AutoResetWaitableEvent term_raster; std::thread thread_raster([&]() { @@ -239,7 +239,7 @@ TEST(RasterThreadMerger, WaitUntilMerged) { latch_merged.Wait(); ASSERT_TRUE(raster_thread_merger->IsMerged()); - for (int i = 0; i < kNumFramesMerged; i++) { + for (size_t i = 0; i < kNumFramesMerged; i++) { ASSERT_TRUE(raster_thread_merger->IsMerged()); raster_thread_merger->DecrementLease(); } @@ -256,26 +256,26 @@ TEST(RasterThreadMerger, HandleTaskQueuesAreTheSame) { TaskQueueWrapper queue; fml::TaskQueueId qid1 = queue.GetTaskQueueId(); fml::TaskQueueId qid2 = qid1; - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); // Statically merged. - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + ASSERT_TRUE(raster_thread_merger->IsMerged()); // Test decrement lease and unmerge are both no-ops. // The task queues should be always merged. - const int kNumFramesMerged = 5; - raster_thread_merger_->MergeWithLease(kNumFramesMerged); + const size_t kNumFramesMerged = 5; + raster_thread_merger->MergeWithLease(kNumFramesMerged); - for (int i = 0; i < kNumFramesMerged; i++) { - ASSERT_TRUE(raster_thread_merger_->IsMerged()); - raster_thread_merger_->DecrementLease(); + for (size_t i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger->IsMerged()); + raster_thread_merger->DecrementLease(); } - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + ASSERT_TRUE(raster_thread_merger->IsMerged()); // Wait until merged should also return immediately. - raster_thread_merger_->WaitUntilMerged(); - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + raster_thread_merger->WaitUntilMerged(); + ASSERT_TRUE(raster_thread_merger->IsMerged()); } TEST(RasterThreadMerger, Enable) { @@ -283,21 +283,21 @@ TEST(RasterThreadMerger, Enable) { TaskQueueWrapper queue2; fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); - raster_thread_merger_->Disable(); - raster_thread_merger_->MergeWithLease(1); - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + raster_thread_merger->Disable(); + raster_thread_merger->MergeWithLease(1); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->Enable(); - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + raster_thread_merger->Enable(); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->MergeWithLease(1); - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + raster_thread_merger->MergeWithLease(1); + ASSERT_TRUE(raster_thread_merger->IsMerged()); - raster_thread_merger_->DecrementLease(); - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + raster_thread_merger->DecrementLease(); + ASSERT_FALSE(raster_thread_merger->IsMerged()); } TEST(RasterThreadMerger, Disable) { @@ -305,44 +305,44 @@ TEST(RasterThreadMerger, Disable) { TaskQueueWrapper queue2; fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); - raster_thread_merger_->Disable(); - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + raster_thread_merger->Disable(); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->MergeWithLease(1); - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + raster_thread_merger->MergeWithLease(1); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->Enable(); - raster_thread_merger_->MergeWithLease(1); - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + raster_thread_merger->Enable(); + raster_thread_merger->MergeWithLease(1); + ASSERT_TRUE(raster_thread_merger->IsMerged()); - raster_thread_merger_->Disable(); - raster_thread_merger_->UnMergeNowIfLastOne(); - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + raster_thread_merger->Disable(); + raster_thread_merger->UnMergeNowIfLastOne(); + ASSERT_TRUE(raster_thread_merger->IsMerged()); { - auto decrement_result = raster_thread_merger_->DecrementLease(); + auto decrement_result = raster_thread_merger->DecrementLease(); ASSERT_EQ(fml::RasterThreadStatus::kRemainsMerged, decrement_result); } - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + ASSERT_TRUE(raster_thread_merger->IsMerged()); - raster_thread_merger_->Enable(); - raster_thread_merger_->UnMergeNowIfLastOne(); - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + raster_thread_merger->Enable(); + raster_thread_merger->UnMergeNowIfLastOne(); + ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger_->MergeWithLease(1); + raster_thread_merger->MergeWithLease(1); - ASSERT_TRUE(raster_thread_merger_->IsMerged()); + ASSERT_TRUE(raster_thread_merger->IsMerged()); { - auto decrement_result = raster_thread_merger_->DecrementLease(); + auto decrement_result = raster_thread_merger->DecrementLease(); ASSERT_EQ(fml::RasterThreadStatus::kUnmergedNow, decrement_result); } - ASSERT_FALSE(raster_thread_merger_->IsMerged()); + ASSERT_FALSE(raster_thread_merger->IsMerged()); } TEST(RasterThreadMerger, IsEnabled) { @@ -350,15 +350,15 @@ TEST(RasterThreadMerger, IsEnabled) { TaskQueueWrapper queue2; fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid1, qid2); - ASSERT_TRUE(raster_thread_merger_->IsEnabled()); + ASSERT_TRUE(raster_thread_merger->IsEnabled()); - raster_thread_merger_->Disable(); - ASSERT_FALSE(raster_thread_merger_->IsEnabled()); + raster_thread_merger->Disable(); + ASSERT_FALSE(raster_thread_merger->IsEnabled()); - raster_thread_merger_->Enable(); - ASSERT_TRUE(raster_thread_merger_->IsEnabled()); + raster_thread_merger->Enable(); + ASSERT_TRUE(raster_thread_merger->IsEnabled()); } TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) { @@ -383,21 +383,21 @@ TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) { fml::TaskQueueId qid_raster = loop_raster->GetTaskRunner()->GetTaskQueueId(); fml::CountDownLatch post_merge(2); - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid_platform, qid_raster); loop_raster->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_FALSE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_raster); - raster_thread_merger_->MergeWithLease(1); + raster_thread_merger->MergeWithLease(1); post_merge.CountDown(); }); loop_raster->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_TRUE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_platform); - raster_thread_merger_->DecrementLease(); + raster_thread_merger->DecrementLease(); post_merge.CountDown(); }); @@ -428,10 +428,10 @@ TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskUnMergesThreads) { loop_raster->GetTaskRunner()->GetTaskQueueId(); fml::AutoResetWaitableEvent merge_latch; - const auto raster_thread_merger_ = + const auto raster_thread_merger = fml::MakeRefCounted(qid_platform, qid_raster); loop_raster->GetTaskRunner()->PostTask([&]() { - raster_thread_merger_->MergeWithLease(1); + raster_thread_merger->MergeWithLease(1); merge_latch.Signal(); }); @@ -441,17 +441,17 @@ TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskUnMergesThreads) { // threads should be merged at this point. fml::AutoResetWaitableEvent unmerge_latch; loop_raster->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_TRUE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_platform); - raster_thread_merger_->DecrementLease(); + raster_thread_merger->DecrementLease(); unmerge_latch.Signal(); }); fml::AutoResetWaitableEvent post_unmerge_latch; loop_raster->GetTaskRunner()->PostTask([&]() { - ASSERT_TRUE(raster_thread_merger_->IsOnRasterizingThread()); - ASSERT_FALSE(raster_thread_merger_->IsOnPlatformThread()); + ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread()); + ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread()); ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_raster); post_unmerge_latch.Signal(); }); @@ -494,45 +494,46 @@ TEST(RasterThreadMerger, MultipleMergersCanMergeSameThreadPair) { TaskQueueWrapper queue2; fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger1_ = + // Two mergers will share one same inner merger + const auto raster_thread_merger1 = fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); - const auto raster_thread_merger2_ = - fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1_, + const auto raster_thread_merger2 = + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1, qid1, qid2); - const int kNumFramesMerged = 5; - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + const size_t kNumFramesMerged = 5; + ASSERT_FALSE(raster_thread_merger1->IsMerged()); + ASSERT_FALSE(raster_thread_merger2->IsMerged()); // Merge using the first merger - raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + raster_thread_merger1->MergeWithLease(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); // let there be one more turn till the leases expire. - for (int i = 0; i < kNumFramesMerged - 1; i++) { + for (size_t i = 0; i < kNumFramesMerged - 1; i++) { // Check merge state using the two merger - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); - raster_thread_merger1_->DecrementLease(); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); + raster_thread_merger1->DecrementLease(); } - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); // extend the lease once with the first merger - raster_thread_merger1_->ExtendLeaseTo(kNumFramesMerged); + raster_thread_merger1->ExtendLeaseTo(kNumFramesMerged); // we will NOT last for 1 extra turn, we just set it. - for (int i = 0; i < kNumFramesMerged; i++) { + for (size_t i = 0; i < kNumFramesMerged; i++) { // Check merge state using the two merger - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); - raster_thread_merger1_->DecrementLease(); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); + raster_thread_merger1->DecrementLease(); } - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + ASSERT_FALSE(raster_thread_merger1->IsMerged()); + ASSERT_FALSE(raster_thread_merger2->IsMerged()); } TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { @@ -540,39 +541,42 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { TaskQueueWrapper queue2; fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); - const auto raster_thread_merger1_ = + // Two mergers will share one same inner merger + const auto raster_thread_merger1 = fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); - const auto raster_thread_merger2_ = - fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1_, + const auto raster_thread_merger2 = + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1, qid1, qid2); - const int kNumFramesMerged = 5; - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + const size_t kNumFramesMerged = 5; + ASSERT_FALSE(raster_thread_merger1->IsMerged()); + ASSERT_FALSE(raster_thread_merger2->IsMerged()); // Merge using the mergers - raster_thread_merger1_->MergeWithLease(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + raster_thread_merger1->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); // Extend the second merger's lease - raster_thread_merger2_->ExtendLeaseTo(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + raster_thread_merger2->ExtendLeaseTo(kNumFramesMerged); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); // Two callers state becomes one caller left. - raster_thread_merger1_->UnMergeNowIfLastOne(); + raster_thread_merger1->UnMergeNowIfLastOne(); // Check if still merged - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + ASSERT_TRUE(raster_thread_merger2->IsMerged()); // One caller state becomes no callers left. - raster_thread_merger2_->UnMergeNowIfLastOne(); + raster_thread_merger2->UnMergeNowIfLastOne(); // Check if unmerged - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + ASSERT_FALSE(raster_thread_merger1->IsMerged()); + ASSERT_FALSE(raster_thread_merger2->IsMerged()); } +/// This case tests multiple standalone engines using independent merger to +/// merge two different raster threads into the same platform thread. TEST(RasterThreadMerger, - MultipleMergersCanIndependentlyMergeTwoOrMoreThreadsIntoOne) { + TwoIndependentMergersCanMergeTwoDifferentThreadsIntoSamePlatformThread) { TaskQueueWrapper queue1; TaskQueueWrapper queue2; TaskQueueWrapper queue3; @@ -580,55 +584,56 @@ TEST(RasterThreadMerger, fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); fml::TaskQueueId qid3 = queue3.GetTaskQueueId(); - const auto raster_thread_merger1_ = + // Two mergers will NOT share same inner merger + const auto merger_from_2_to_1 = fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); - const auto raster_thread_merger2_ = - fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1_, + const auto merger_from_3_to_1 = + fml::RasterThreadMerger::CreateOrShareThreadMerger(merger_from_2_to_1, qid1, qid3); - const int kNumFramesMerged = 5; - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + const size_t kNumFramesMerged = 5; + ASSERT_FALSE(merger_from_2_to_1->IsMerged()); + ASSERT_FALSE(merger_from_3_to_1->IsMerged()); // Merge thread2 into thread1 - raster_thread_merger1_->MergeWithLease(kNumFramesMerged); + merger_from_2_to_1->MergeWithLease(kNumFramesMerged); // Merge thread3 into thread1 - raster_thread_merger2_->MergeWithLease(kNumFramesMerged); + merger_from_3_to_1->MergeWithLease(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + ASSERT_TRUE(merger_from_2_to_1->IsMerged()); + ASSERT_TRUE(merger_from_3_to_1->IsMerged()); - for (int i = 0; i < kNumFramesMerged; i++) { - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - raster_thread_merger1_->DecrementLease(); + for (size_t i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(merger_from_2_to_1->IsMerged()); + merger_from_2_to_1->DecrementLease(); } - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + ASSERT_FALSE(merger_from_2_to_1->IsMerged()); + ASSERT_TRUE(merger_from_3_to_1->IsMerged()); - for (int i = 0; i < kNumFramesMerged; i++) { - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); - raster_thread_merger2_->DecrementLease(); + for (size_t i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(merger_from_3_to_1->IsMerged()); + merger_from_3_to_1->DecrementLease(); } - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + ASSERT_FALSE(merger_from_2_to_1->IsMerged()); + ASSERT_FALSE(merger_from_3_to_1->IsMerged()); - raster_thread_merger1_->MergeWithLease(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); - raster_thread_merger2_->MergeWithLease(kNumFramesMerged); - ASSERT_TRUE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + merger_from_2_to_1->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(merger_from_2_to_1->IsMerged()); + ASSERT_FALSE(merger_from_3_to_1->IsMerged()); + merger_from_3_to_1->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(merger_from_2_to_1->IsMerged()); + ASSERT_TRUE(merger_from_3_to_1->IsMerged()); // Can unmerge independently - raster_thread_merger1_->UnMergeNowIfLastOne(); - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_TRUE(raster_thread_merger2_->IsMerged()); + merger_from_2_to_1->UnMergeNowIfLastOne(); + ASSERT_FALSE(merger_from_2_to_1->IsMerged()); + ASSERT_TRUE(merger_from_3_to_1->IsMerged()); // Can unmerge independently - raster_thread_merger2_->UnMergeNowIfLastOne(); - ASSERT_FALSE(raster_thread_merger1_->IsMerged()); - ASSERT_FALSE(raster_thread_merger2_->IsMerged()); + merger_from_3_to_1->UnMergeNowIfLastOne(); + ASSERT_FALSE(merger_from_2_to_1->IsMerged()); + ASSERT_FALSE(merger_from_3_to_1->IsMerged()); } } // namespace testing diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc index cf33a5d646113..3ee0eb69eeca1 100644 --- a/fml/shared_thread_merger.cc +++ b/fml/shared_thread_merger.cc @@ -17,7 +17,7 @@ SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner, task_queues_(fml::MessageLoopTaskQueues::GetInstance()) {} bool SharedThreadMerger::MergeWithLease(RasterThreadMergerId caller, - int lease_term) { + size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); if (IsMergedUnSafe()) { @@ -53,11 +53,16 @@ bool SharedThreadMerger::DecrementLease(RasterThreadMergerId caller) { auto entry = lease_term_by_caller_.find(caller); bool exist = entry != lease_term_by_caller_.end(); if (exist) { - std::atomic_int& lease_term_ref = entry->second; + std::atomic_size_t& lease_term_ref = entry->second; FML_CHECK(lease_term_ref > 0) << "lease_term should always be positive when merged, lease_term=" << lease_term_ref; lease_term_ref--; + } else { + FML_LOG(WARNING) << "The caller does not exist when calling " + "DecrementLease(), ignored. This may happens after " + "caller is erased in UnMergeNowIfLastOne(). caller=" + << caller; } if (IsAllLeaseTermsZeroUnSafe()) { // Unmerge now because lease_term_ decreased to zero. @@ -68,7 +73,7 @@ bool SharedThreadMerger::DecrementLease(RasterThreadMergerId caller) { } void SharedThreadMerger::ExtendLeaseTo(RasterThreadMergerId caller, - int lease_term) { + size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(mutex_); FML_DCHECK(IsMergedUnSafe()) diff --git a/fml/shared_thread_merger.h b/fml/shared_thread_merger.h index 80aaa9dea50ea..30b3d88e1be96 100644 --- a/fml/shared_thread_merger.h +++ b/fml/shared_thread_merger.h @@ -14,34 +14,36 @@ namespace fml { -class MessageLoopImpl; class RasterThreadMerger; typedef void* RasterThreadMergerId; +/// Instance of this class is shared between multiple |RasterThreadMerger| +/// instances, Most of the callings from |RasterThreadMerger| will be redirected +/// to this class with one more caller parameter. class SharedThreadMerger : public fml::RefCountedThreadSafe { public: SharedThreadMerger(TaskQueueId owner, TaskQueueId subsumed); - // It's called by |RasterThreadMerger::MergeWithLease()|. - // See the doc of |RasterThreadMerger::MergeWithLease()|. - bool MergeWithLease(RasterThreadMergerId caller, int lease_term); + // It's called by |RasterThreadMerger::MergeWithLease|. + // See the doc of |RasterThreadMerger::MergeWithLease|. + bool MergeWithLease(RasterThreadMergerId caller, size_t lease_term); - // It's called by |RasterThreadMerger::UnMergeNowIfLastOne()|. - // See the doc of |RasterThreadMerger::UnMergeNowIfLastOne()|. + // It's called by |RasterThreadMerger::UnMergeNowIfLastOne|. + // See the doc of |RasterThreadMerger::UnMergeNowIfLastOne|. bool UnMergeNowIfLastOne(RasterThreadMergerId caller); - // It's called by |RasterThreadMerger::ExtendLeaseTo()|. - // See the doc of |RasterThreadMerger::ExtendLeaseTo()|. - void ExtendLeaseTo(RasterThreadMergerId caller, int lease_term); + // It's called by |RasterThreadMerger::ExtendLeaseTo|. + // See the doc of |RasterThreadMerger::ExtendLeaseTo|. + void ExtendLeaseTo(RasterThreadMergerId caller, size_t lease_term); - // It's called by |RasterThreadMerger::IsMergedUnSafe()|. - // See the doc of |RasterThreadMerger::IsMergedUnSafe()|. + // It's called by |RasterThreadMerger::IsMergedUnSafe|. + // See the doc of |RasterThreadMerger::IsMergedUnSafe|. bool IsMergedUnSafe() const; - // It's called by |RasterThreadMerger::DecrementLease()|. - // See the doc of |RasterThreadMerger::DecrementLease()|. + // It's called by |RasterThreadMerger::DecrementLease|. + // See the doc of |RasterThreadMerger::DecrementLease|. bool DecrementLease(RasterThreadMergerId caller); private: @@ -51,9 +53,9 @@ class SharedThreadMerger std::mutex mutex_; /// The |MergeWithLease| or |ExtendLeaseTo| method will record the caller - /// into this lease_term_by_caller_ map, |UnMergeNowIfLastOne()| + /// into this lease_term_by_caller_ map, |UnMergeNowIfLastOne| /// method will remove the caller from this lease_term_by_caller_. - std::map lease_term_by_caller_; + std::map lease_term_by_caller_; bool IsAllLeaseTermsZeroUnSafe() const; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 3d8000651f205..edc4b6e021e3e 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -149,7 +149,7 @@ std::unique_ptr Shell::Create( } return CreateWithSnapshot(std::move(platform_data), // std::move(task_runners), // - nullptr, // + /*parent_merger=*/nullptr, // std::move(settings), // std::move(vm), // std::move(isolate_snapshot), //