diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index f14d64ed8ac28..6110e683bfbe2 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -23,8 +23,7 @@ RasterThreadMerger::RasterThreadMerger( fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - shared_merger_(shared_merger), - enabled_(true) {} + shared_merger_(shared_merger) {} void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; @@ -119,12 +118,12 @@ bool RasterThreadMerger::IsMerged() { void RasterThreadMerger::Enable() { std::scoped_lock lock(mutex_); - enabled_ = true; + shared_merger_->SetEnabledUnSafe(true); } void RasterThreadMerger::Disable() { std::scoped_lock lock(mutex_); - enabled_ = false; + shared_merger_->SetEnabledUnSafe(false); } bool RasterThreadMerger::IsEnabled() { @@ -133,7 +132,7 @@ bool RasterThreadMerger::IsEnabled() { } bool RasterThreadMerger::IsEnabledUnSafe() const { - return enabled_; + return shared_merger_->IsEnabledUnSafe(); } bool RasterThreadMerger::IsMergedUnSafe() const { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index bcad1904e1513..d3babbfe5d116 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -127,7 +127,6 @@ class RasterThreadMerger std::condition_variable merged_condition_; std::mutex mutex_; fml::closure merge_unmerge_callback_; - bool enabled_; bool IsMergedUnSafe() const; diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 339cd174752dd..43beb63947e1f 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -361,6 +361,27 @@ TEST(RasterThreadMerger, IsEnabled) { ASSERT_TRUE(raster_thread_merger->IsEnabled()); } +TEST(RasterThreadMerger, TwoMergersWithSameThreadPairShareEnabledState) { + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); + const auto merger1 = + RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); + const auto merger2 = + RasterThreadMerger::CreateOrShareThreadMerger(merger1, qid1, qid2); + ASSERT_TRUE(merger1->IsEnabled()); + ASSERT_TRUE(merger2->IsEnabled()); + + merger1->Disable(); + ASSERT_FALSE(merger1->IsEnabled()); + ASSERT_FALSE(merger2->IsEnabled()); + + merger2->Enable(); + ASSERT_TRUE(merger1->IsEnabled()); + ASSERT_TRUE(merger2->IsEnabled()); +} + TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) { fml::MessageLoop* loop_platform = nullptr; fml::AutoResetWaitableEvent latch1; diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc index 3ee0eb69eeca1..71a6bf313b9e2 100644 --- a/fml/shared_thread_merger.cc +++ b/fml/shared_thread_merger.cc @@ -14,7 +14,8 @@ SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner, fml::TaskQueueId subsumed) : owner_(owner), subsumed_(subsumed), - task_queues_(fml::MessageLoopTaskQueues::GetInstance()) {} + task_queues_(fml::MessageLoopTaskQueues::GetInstance()), + enabled_(true) {} bool SharedThreadMerger::MergeWithLease(RasterThreadMergerId caller, size_t lease_term) { @@ -85,6 +86,14 @@ bool SharedThreadMerger::IsMergedUnSafe() const { return !IsAllLeaseTermsZeroUnSafe(); } +bool SharedThreadMerger::IsEnabledUnSafe() const { + return enabled_; +} + +void SharedThreadMerger::SetEnabledUnSafe(bool enabled) { + enabled_ = enabled; +} + 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; }); diff --git a/fml/shared_thread_merger.h b/fml/shared_thread_merger.h index 30b3d88e1be96..b278db4e93d99 100644 --- a/fml/shared_thread_merger.h +++ b/fml/shared_thread_merger.h @@ -42,6 +42,14 @@ class SharedThreadMerger // See the doc of |RasterThreadMerger::IsMergedUnSafe|. bool IsMergedUnSafe() const; + // It's called by |RasterThreadMerger::IsEnabledUnSafe|. + // See the doc of |RasterThreadMerger::IsEnabledUnSafe|. + bool IsEnabledUnSafe() const; + + // It's called by |RasterThreadMerger::Enable| or + // |RasterThreadMerger::Disable|. + void SetEnabledUnSafe(bool enabled); + // It's called by |RasterThreadMerger::DecrementLease|. // See the doc of |RasterThreadMerger::DecrementLease|. bool DecrementLease(RasterThreadMergerId caller); @@ -51,6 +59,7 @@ class SharedThreadMerger fml::TaskQueueId subsumed_; fml::RefPtr task_queues_; std::mutex mutex_; + bool enabled_; /// The |MergeWithLease| or |ExtendLeaseTo| method will record the caller /// into this lease_term_by_caller_ map, |UnMergeNowIfLastOne| diff --git a/shell/common/shell.cc b/shell/common/shell.cc index edc4b6e021e3e..6eba4d7547622 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -709,9 +709,9 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { // // This prevents false positives such as this method starts assuming that the // raster and platform queues have a given thread configuration, but then the - // configuration is changed by a task, and the asumption is not longer true. + // configuration is changed by a task, and the assumption is no longer true. // - // This incorrect assumption can lead to dead lock. + // This incorrect assumption can lead to deadlock. // See `should_post_raster_task` for more. rasterizer_->DisableThreadMergerIfNeeded(); @@ -719,7 +719,7 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { // starting the sequence and waiting on the latch. Later the UI thread posts // raster_task to the raster thread which signals the latch. If the raster and // the platform threads are the same this results in a deadlock as the - // raster_task will never be posted to the plaform/raster thread that is + // raster_task will never be posted to the platform/raster thread that is // blocked on a latch. To avoid the described deadlock, if the raster and the // platform threads are the same, should_post_raster_task will be false, and // then instead of posting a task to the raster thread, the ui thread just @@ -818,9 +818,9 @@ void Shell::OnPlatformViewDestroyed() { // // This prevents false positives such as this method starts assuming that the // raster and platform queues have a given thread configuration, but then the - // configuration is changed by a task, and the asumption is not longer true. + // configuration is changed by a task, and the assumption is no longer true. // - // This incorrect assumption can lead to dead lock. + // This incorrect assumption can lead to deadlock. // See `should_post_raster_task` for more. rasterizer_->DisableThreadMergerIfNeeded(); @@ -828,8 +828,8 @@ void Shell::OnPlatformViewDestroyed() { // starting the sequence and waiting on the latch. Later the UI thread posts // raster_task to the raster thread triggers signaling the latch(on the IO // thread). If the raster and the platform threads are the same this results - // in a deadlock as the raster_task will never be posted to the plaform/raster - // thread that is blocked on a latch. To avoid the described deadlock, if the + // in a deadlock as the raster_task will never be posted to platform/raster + // thread that is blocked on a latch. To avoid the described deadlock, if the // raster and the platform threads are the same, should_post_raster_task will // be false, and then instead of posting a task to the raster thread, the ui // thread just signals the latch and the platform/raster thread follows with