From 04b8acca77b7537c43c9c9fbd9d535acc30059d6 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Wed, 12 Jul 2023 09:08:35 -0700 Subject: [PATCH 1/5] draftr --- fml/raster_thread_merger.cc | 11 ++++++++--- fml/raster_thread_merger.h | 2 ++ fml/shared_thread_merger.cc | 1 + impeller/renderer/renderer.cc | 1 - 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 7eb108faf9110..333513ca67531 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -17,7 +17,9 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, : RasterThreadMerger( MakeRefCounted(platform_queue_id, gpu_queue_id), platform_queue_id, - gpu_queue_id) {} + gpu_queue_id) { + id_ = id_count++; +} RasterThreadMerger::RasterThreadMerger( fml::RefPtr shared_merger, @@ -25,7 +27,9 @@ RasterThreadMerger::RasterThreadMerger( fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - shared_merger_(std::move(shared_merger)) {} + shared_merger_(std::move(shared_merger)) { + id_ = id_count++; +} void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; @@ -68,9 +72,9 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { bool success = shared_merger_->MergeWithLease(this, lease_term); if (success && merge_unmerge_callback_ != nullptr) { + FML_DLOG(ERROR) << id_ << " merged"; merge_unmerge_callback_(); } - merged_condition_.notify_one(); } @@ -87,6 +91,7 @@ void RasterThreadMerger::UnMergeNowIfLastOne() { if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } + FML_DLOG(ERROR) << id_ << "un-merged"; } bool RasterThreadMerger::IsOnPlatformThread() const { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 3ad4a4e27da0c..564ea8881d245 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -16,6 +16,7 @@ namespace fml { class MessageLoopImpl; +static int id_count = 0; enum class RasterThreadStatus { kRemainsMerged, @@ -135,6 +136,7 @@ class RasterThreadMerger // The platform_queue_id and gpu_queue_id are exactly the same. // We consider the threads are always merged and cannot be unmerged. bool TaskQueuesAreSame() const; + int id_ = 0; FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc index 15fac3e2fa0c2..1753c166029bd 100644 --- a/fml/shared_thread_merger.cc +++ b/fml/shared_thread_merger.cc @@ -66,6 +66,7 @@ bool SharedThreadMerger::DecrementLease(RasterThreadMergerId caller) { "caller is erased in UnMergeNowIfLastOne(). caller=" << caller; } + FML_DLOG(ERROR) << "lease term " << entry->first << " " << entry->second; if (IsAllLeaseTermsZeroUnSafe()) { // Unmerge now because lease_term_ decreased to zero. UnMergeNowUnSafe(); diff --git a/impeller/renderer/renderer.cc b/impeller/renderer/renderer.cc index d1fda89cf726f..0522b35e4a75d 100644 --- a/impeller/renderer/renderer.cc +++ b/impeller/renderer/renderer.cc @@ -56,7 +56,6 @@ bool Renderer::Render(std::unique_ptr surface, const auto present_result = surface->Present(); frames_in_flight_sema_->Signal(); - return present_result; } From 1bc3599d47899c490277b2b955788fe089ea244a Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 13 Jul 2023 11:49:50 -0700 Subject: [PATCH 2/5] fix --- fml/raster_thread_merger.cc | 10 ++-------- fml/raster_thread_merger.h | 10 ++++------ fml/shared_thread_merger.cc | 7 +++---- 3 files changed, 9 insertions(+), 18 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 333513ca67531..68476ebcbd1f8 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -17,9 +17,7 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, : RasterThreadMerger( MakeRefCounted(platform_queue_id, gpu_queue_id), platform_queue_id, - gpu_queue_id) { - id_ = id_count++; -} + gpu_queue_id) {} RasterThreadMerger::RasterThreadMerger( fml::RefPtr shared_merger, @@ -27,9 +25,7 @@ RasterThreadMerger::RasterThreadMerger( fml::TaskQueueId gpu_queue_id) : platform_queue_id_(platform_queue_id), gpu_queue_id_(gpu_queue_id), - shared_merger_(std::move(shared_merger)) { - id_ = id_count++; -} + shared_merger_(std::move(shared_merger)) {} void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { merge_unmerge_callback_ = callback; @@ -72,7 +68,6 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { bool success = shared_merger_->MergeWithLease(this, lease_term); if (success && merge_unmerge_callback_ != nullptr) { - FML_DLOG(ERROR) << id_ << " merged"; merge_unmerge_callback_(); } merged_condition_.notify_one(); @@ -91,7 +86,6 @@ void RasterThreadMerger::UnMergeNowIfLastOne() { if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } - FML_DLOG(ERROR) << id_ << "un-merged"; } bool RasterThreadMerger::IsOnPlatformThread() const { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 564ea8881d245..98aa7e6293055 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -16,7 +16,6 @@ namespace fml { class MessageLoopImpl; -static int id_count = 0; enum class RasterThreadStatus { kRemainsMerged, @@ -54,10 +53,10 @@ class RasterThreadMerger 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 after - // |MergeWithLease| or |ExtendLeaseTo| method. + // Un-merges the threads now if one of current caller is the last merge caller + // that is merged, and it resets the lease term to 0, otherwise it will remove + // the caller record and return. The multiple caller records were recorded + // after |MergeWithLease| or |ExtendLeaseTo| method. // // Must be executed on the raster task runner. // @@ -136,7 +135,6 @@ class RasterThreadMerger // The platform_queue_id and gpu_queue_id are exactly the same. // We consider the threads are always merged and cannot be unmerged. bool TaskQueuesAreSame() const; - int id_ = 0; FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc index 1753c166029bd..0b56b229d4834 100644 --- a/fml/shared_thread_merger.cc +++ b/fml/shared_thread_merger.cc @@ -44,10 +44,10 @@ bool SharedThreadMerger::UnMergeNowUnSafe() { bool SharedThreadMerger::UnMergeNowIfLastOne(RasterThreadMergerId caller) { std::scoped_lock lock(mutex_); lease_term_by_caller_.erase(caller); - if (!lease_term_by_caller_.empty()) { - return true; + if (lease_term_by_caller_.empty() || IsAllLeaseTermsZeroUnSafe()) { + return UnMergeNowUnSafe(); } - return UnMergeNowUnSafe(); + return true; } bool SharedThreadMerger::DecrementLease(RasterThreadMergerId caller) { @@ -66,7 +66,6 @@ bool SharedThreadMerger::DecrementLease(RasterThreadMergerId caller) { "caller is erased in UnMergeNowIfLastOne(). caller=" << caller; } - FML_DLOG(ERROR) << "lease term " << entry->first << " " << entry->second; if (IsAllLeaseTermsZeroUnSafe()) { // Unmerge now because lease_term_ decreased to zero. UnMergeNowUnSafe(); From 3f13c1c1d6df3999ce4dfc8a7533735e85f1a708 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 13 Jul 2023 12:04:12 -0700 Subject: [PATCH 3/5] test --- fml/raster_thread_merger_unittests.cc | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 43beb63947e1f..d366386a72a9d 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -594,6 +594,42 @@ TEST(RasterThreadMerger, TheLastCallerOfMultipleMergersCanUnmergeNow) { ASSERT_FALSE(raster_thread_merger2->IsMerged()); } +TEST(RasterThreadMerger, TheLastMergedCallerOfMultipleMergersCanUnmergeNow) { + TaskQueueWrapper queue1; + TaskQueueWrapper queue2; + fml::TaskQueueId qid1 = queue1.GetTaskQueueId(); + fml::TaskQueueId qid2 = queue2.GetTaskQueueId(); + // Two mergers will share one same inner merger + const auto raster_thread_merger1 = + fml::RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2); + + const size_t kNumFramesMerged = 1; + ASSERT_FALSE(raster_thread_merger1->IsMerged()); + + // Merge using the mergers + raster_thread_merger1->MergeWithLease(kNumFramesMerged); + ASSERT_TRUE(raster_thread_merger1->IsMerged()); + + for (size_t i = 0; i < kNumFramesMerged; i ++) { + // Un-merge thread merger 1. + raster_thread_merger1->DecrementLease(); + } + ASSERT_FALSE(raster_thread_merger1->IsMerged()); + + const auto raster_thread_merger2 = + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1, + qid1, qid2); + ASSERT_FALSE(raster_thread_merger2->IsMerged()); + raster_thread_merger2->MergeWithLease(kNumFramesMerged); + 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()); +} + /// This case tests multiple standalone engines using independent merger to /// merge two different raster threads into the same platform thread. TEST(RasterThreadMerger, From 6449e105e069a3e7b67f841fc6aef9fdba514af2 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 13 Jul 2023 12:15:10 -0700 Subject: [PATCH 4/5] clean --- fml/raster_thread_merger.cc | 1 + fml/raster_thread_merger_unittests.cc | 6 +++--- impeller/renderer/renderer.cc | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 68476ebcbd1f8..7eb108faf9110 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -70,6 +70,7 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { if (success && merge_unmerge_callback_ != nullptr) { merge_unmerge_callback_(); } + merged_condition_.notify_one(); } diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index d366386a72a9d..b4b9621d23196 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -610,15 +610,15 @@ TEST(RasterThreadMerger, TheLastMergedCallerOfMultipleMergersCanUnmergeNow) { raster_thread_merger1->MergeWithLease(kNumFramesMerged); ASSERT_TRUE(raster_thread_merger1->IsMerged()); - for (size_t i = 0; i < kNumFramesMerged; i ++) { + for (size_t i = 0; i < kNumFramesMerged; i++) { // Un-merge thread merger 1. raster_thread_merger1->DecrementLease(); } ASSERT_FALSE(raster_thread_merger1->IsMerged()); const auto raster_thread_merger2 = - fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1, - qid1, qid2); + fml::RasterThreadMerger::CreateOrShareThreadMerger(raster_thread_merger1, + qid1, qid2); ASSERT_FALSE(raster_thread_merger2->IsMerged()); raster_thread_merger2->MergeWithLease(kNumFramesMerged); ASSERT_TRUE(raster_thread_merger2->IsMerged()); diff --git a/impeller/renderer/renderer.cc b/impeller/renderer/renderer.cc index 0522b35e4a75d..d1fda89cf726f 100644 --- a/impeller/renderer/renderer.cc +++ b/impeller/renderer/renderer.cc @@ -56,6 +56,7 @@ bool Renderer::Render(std::unique_ptr surface, const auto present_result = surface->Present(); frames_in_flight_sema_->Signal(); + return present_result; } From 38c7c53686c718f0bf5cc25c7916050c9463877d Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 13 Jul 2023 12:17:15 -0700 Subject: [PATCH 5/5] update comment --- fml/raster_thread_merger.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 98aa7e6293055..2fd0bc22e3ab5 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -53,8 +53,8 @@ class RasterThreadMerger TaskQueueId platform_id, TaskQueueId raster_id); - // Un-merges the threads now if one of current caller is the last merge caller - // that is merged, and it resets the lease term to 0, otherwise it will remove + // Un-merges the threads now if current caller is the last merged caller, + // and it resets the lease term to 0, otherwise it will remove // the caller record and return. The multiple caller records were recorded // after |MergeWithLease| or |ExtendLeaseTo| method. //