From f31e62ff46a52a9e6f9b0f2935575030c793394a Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 21 Jul 2020 11:53:26 -0700 Subject: [PATCH 01/16] merge threads when onplatformviewdestroy --- fml/raster_thread_merger.cc | 36 ++++++--- fml/raster_thread_merger.h | 7 +- shell/common/rasterizer.cc | 25 ++++++ shell/common/rasterizer.h | 4 + shell/common/shell.cc | 80 ++++--------------- .../ios/framework/Source/FlutterEngine.mm | 50 +++--------- 6 files changed, 91 insertions(+), 111 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index b424c04b2754e..7b8c373a87679 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -17,23 +17,37 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, gpu_queue_id_(gpu_queue_id), task_queues_(fml::MessageLoopTaskQueues::GetInstance()), lease_term_(kLeaseNotSet) { - is_merged_ = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); + bool is_merged = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); + is_merged_.store(is_merged); + if (is_merged) { + lease_term_ = 1; + } } void RasterThreadMerger::MergeWithLease(size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; - if (!is_merged_) { - is_merged_ = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); + if (!is_merged_.load()) { + FML_DLOG(ERROR) << "--- tm_: merged"; + is_merged_.store(task_queues_->Merge(platform_queue_id_, gpu_queue_id_)); lease_term_ = lease_term; } } +void RasterThreadMerger::UnMergeNow() { + FML_CHECK(IsOnRasterizingThread()); + lease_term_ = 0; + bool success = task_queues_->Unmerge(platform_queue_id_); + FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; + is_merged_.store(false); + FML_DLOG(ERROR) << "--- tm_: unmerged"; +} + bool RasterThreadMerger::IsOnPlatformThread() const { return MessageLoop::GetCurrentTaskQueueId() == platform_queue_id_; } bool RasterThreadMerger::IsOnRasterizingThread() const { - if (is_merged_) { + if (is_merged_.load()) { return IsOnPlatformThread(); } else { return !IsOnPlatformThread(); @@ -48,11 +62,17 @@ void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { } bool RasterThreadMerger::IsMerged() const { - return is_merged_; + return is_merged_.load(); +} + +void RasterThreadMerger::WaitUntilMerged() { + FML_CHECK(MessageLoop::GetCurrentTaskQueueId() != gpu_queue_id_); + while (!is_merged_.load()) + ; } RasterThreadStatus RasterThreadMerger::DecrementLease() { - if (!is_merged_) { + if (!is_merged_.load()) { return RasterThreadStatus::kRemainsUnmerged; } @@ -65,9 +85,7 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { << "lease_term should always be positive when merged."; lease_term_--; if (lease_term_ == 0) { - bool success = task_queues_->Unmerge(platform_queue_id_); - FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; - is_merged_ = false; + UnMergeNow(); return RasterThreadStatus::kUnmergedNow; } diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 7c0318ff77d26..4719fcbea237c 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -8,6 +8,7 @@ #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" #include "flutter/fml/message_loop_task_queues.h" +#include "flutter/fml/synchronization/waitable_event.h" namespace fml { @@ -30,6 +31,8 @@ class RasterThreadMerger // unless an |ExtendLeaseTo| gets called. void MergeWithLease(size_t lease_term); + void UnMergeNow(); + void ExtendLeaseTo(size_t lease_term); // Returns |RasterThreadStatus::kUnmergedNow| if this call resulted in @@ -38,6 +41,8 @@ class RasterThreadMerger bool IsMerged() const; + void WaitUntilMerged(); + RasterThreadMerger(fml::TaskQueueId platform_queue_id, fml::TaskQueueId gpu_queue_id); @@ -55,7 +60,7 @@ class RasterThreadMerger fml::TaskQueueId gpu_queue_id_; fml::RefPtr task_queues_; std::atomic_int lease_term_; - bool is_merged_; + std::atomic is_merged_; FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 9735854f41842..13a4e0a20c020 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -657,6 +657,31 @@ std::optional Rasterizer::GetResourceCacheMaxBytes() const { return std::nullopt; } +bool Rasterizer::EnsureThreadsAreMerged() { + if (raster_thread_merger_.get() == nullptr) { + return false; + } + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), + [weak_this = weak_factory_.GetWeakPtr(), + thread_merger = raster_thread_merger_]() { + if (weak_this->surface_ == nullptr) { + FML_DLOG(ERROR) << "--- no surface_"; + return; + } + FML_DLOG(ERROR) << "--- merge with lease"; + thread_merger->MergeWithLease(10); + }); + raster_thread_merger_->WaitUntilMerged(); + return true; +} + +void Rasterizer::UnMergeNow() { + if (raster_thread_merger_.get() == nullptr) { + return; + } + raster_thread_merger_->UnMergeNow(); +} + Rasterizer::Screenshot::Screenshot() {} Rasterizer::Screenshot::Screenshot(sk_sp p_data, SkISize p_size) diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index c436c3bda91e5..fce40c8414fd8 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -430,6 +430,10 @@ class Rasterizer final : public SnapshotDelegate { /// std::optional GetResourceCacheMaxBytes() const; + bool EnsureThreadsAreMerged(); + + void UnMergeNow(); + private: Delegate& delegate_; TaskRunners task_runners_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 976d1befb37b5..6f1e896303fdc 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -605,10 +605,10 @@ DartVM* Shell::GetDartVM() { // |PlatformView::Delegate| void Shell::OnPlatformViewCreated(std::unique_ptr surface) { + FML_DLOG(ERROR) << "--- OnPlatformViewCreated"; TRACE_EVENT0("flutter", "Shell::OnPlatformViewCreated"); FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - // Note: // This is a synchronous operation because certain platforms depend on // setup/suspension of all activities that may be interacting with the GPU in @@ -630,36 +630,13 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { latch.Signal(); }); - // The normal flow executed by this method is that the platform thread is - // 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 - // 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 executing - // raster_task. - bool should_post_raster_task = task_runners_.GetRasterTaskRunner() != - task_runners_.GetPlatformTaskRunner(); - - auto ui_task = [engine = engine_->GetWeakPtr(), // - raster_task_runner = task_runners_.GetRasterTaskRunner(), // - raster_task, should_post_raster_task, - &latch // + auto ui_task = [engine = engine_->GetWeakPtr(), // + &latch // ] { if (engine) { engine->OnOutputSurfaceCreated(); } - // Step 2: Next, tell the raster thread that it should create a surface for - // its rasterizer. - if (should_post_raster_task) { - fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); - } else { - // See comment on should_post_raster_task, in this case we just unblock - // the platform thread. - latch.Signal(); - } + latch.Signal(); }; // Threading: Capture platform view by raw pointer and not the weak pointer. @@ -682,14 +659,10 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { }; fml::TaskRunner::RunNowOrPostTask(task_runners_.GetIOTaskRunner(), io_task); - latch.Wait(); - if (!should_post_raster_task) { - // See comment on should_post_raster_task, in this case the raster_task - // wasn't executed, and we just run it here as the platform thread - // is the raster thread. - raster_task(); - } + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), + raster_task); + latch.Wait(); } // |PlatformView::Delegate| @@ -726,47 +699,26 @@ void Shell::OnPlatformViewDestroyed() { fml::TaskRunner::RunNowOrPostTask(io_task_runner, io_task); }; - // The normal flow executed by this method is that the platform thread is - // 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 - // 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 - // executing raster_task. - bool should_post_raster_task = task_runners_.GetRasterTaskRunner() != - task_runners_.GetPlatformTaskRunner(); - - auto ui_task = [engine = engine_->GetWeakPtr(), - raster_task_runner = task_runners_.GetRasterTaskRunner(), - raster_task, should_post_raster_task, &latch]() { + auto ui_task = [engine = engine_->GetWeakPtr(), &latch]() { if (engine) { engine->OnOutputSurfaceDestroyed(); } - // Step 1: Next, tell the raster thread that its rasterizer should suspend - // access to the underlying surface. - if (should_post_raster_task) { - fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); - } else { - // See comment on should_post_raster_task, in this case we just unblock - // the platform thread. - latch.Signal(); - } + latch.Signal(); }; // Step 0: Post a task onto the UI thread to tell the engine that its output // surface is about to go away. fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), ui_task); latch.Wait(); - if (!should_post_raster_task) { - // See comment on should_post_raster_task, in this case the raster_task - // wasn't executed, and we just run it here as the platform thread - // is the raster thread. + if (rasterizer_->EnsureThreadsAreMerged()) { raster_task(); - latch.Wait(); + rasterizer_->UnMergeNow(); + } else { + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), + raster_task); } + latch.Wait(); + FML_DLOG(ERROR) << "shell unmerged"; } // |PlatformView::Delegate| diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index 57d531ec7420d..be853fe2ef6ae 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -493,43 +493,19 @@ - (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryURI { shell.GetIsGpuDisabledSyncSwitch()); }; - if (flutter::IsIosEmbeddedViewsPreviewEnabled()) { - // Embedded views requires the gpu and the platform views to be the same. - // The plan is to eventually dynamically merge the threads when there's a - // platform view in the layer tree. - // For now we use a fixed thread configuration with the same thread used as the - // gpu and platform task runner. - // TODO(amirh/chinmaygarde): remove this, and dynamically change the thread configuration. - // https://github.com/flutter/flutter/issues/23975 - - flutter::TaskRunners task_runners(threadLabel.UTF8String, // label - fml::MessageLoop::GetCurrent().GetTaskRunner(), // platform - fml::MessageLoop::GetCurrent().GetTaskRunner(), // raster - _threadHost.ui_thread->GetTaskRunner(), // ui - _threadHost.io_thread->GetTaskRunner() // io - ); - // Create the shell. This is a blocking operation. - _shell = flutter::Shell::Create(std::move(task_runners), // task runners - std::move(windowData), // window data - std::move(settings), // settings - on_create_platform_view, // platform view creation - on_create_rasterizer // rasterzier creation - ); - } else { - flutter::TaskRunners task_runners(threadLabel.UTF8String, // label - fml::MessageLoop::GetCurrent().GetTaskRunner(), // platform - _threadHost.raster_thread->GetTaskRunner(), // raster - _threadHost.ui_thread->GetTaskRunner(), // ui - _threadHost.io_thread->GetTaskRunner() // io - ); - // Create the shell. This is a blocking operation. - _shell = flutter::Shell::Create(std::move(task_runners), // task runners - std::move(windowData), // window data - std::move(settings), // settings - on_create_platform_view, // platform view creation - on_create_rasterizer // rasterzier creation - ); - } + flutter::TaskRunners task_runners(threadLabel.UTF8String, // label + fml::MessageLoop::GetCurrent().GetTaskRunner(), // platform + _threadHost.raster_thread->GetTaskRunner(), // raster + _threadHost.ui_thread->GetTaskRunner(), // ui + _threadHost.io_thread->GetTaskRunner() // io + ); + // Create the shell. This is a blocking operation. + _shell = flutter::Shell::Create(std::move(task_runners), // task runners + std::move(windowData), // window data + std::move(settings), // settings + on_create_platform_view, // platform view creation + on_create_rasterizer // rasterzier creation + ); if (_shell == nullptr) { FML_LOG(ERROR) << "Could not start a shell FlutterEngine with entrypoint: " From 54c84471db2cae7be9ec8c894c778d1e5b1ca290 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 21 Jul 2020 12:11:18 -0700 Subject: [PATCH 02/16] cleanup --- fml/raster_thread_merger.h | 1 - shell/common/rasterizer.cc | 10 +++------- shell/common/rasterizer.h | 2 -- shell/common/shell.cc | 10 +++------- 4 files changed, 6 insertions(+), 17 deletions(-) diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 4719fcbea237c..101eb4b6ee899 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -8,7 +8,6 @@ #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" #include "flutter/fml/message_loop_task_queues.h" -#include "flutter/fml/synchronization/waitable_event.h" namespace fml { diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 13a4e0a20c020..9c82b7e94d3bf 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -93,6 +93,9 @@ void Rasterizer::Teardown() { compositor_context_->OnGrContextDestroyed(); surface_.reset(); last_layer_tree_.reset(); + if (raster_thread_merger_.get() != nullptr) { + raster_thread_merger_->UnMergeNow(); + } } void Rasterizer::NotifyLowMemoryWarning() const { @@ -675,13 +678,6 @@ bool Rasterizer::EnsureThreadsAreMerged() { return true; } -void Rasterizer::UnMergeNow() { - if (raster_thread_merger_.get() == nullptr) { - return; - } - raster_thread_merger_->UnMergeNow(); -} - Rasterizer::Screenshot::Screenshot() {} Rasterizer::Screenshot::Screenshot(sk_sp p_data, SkISize p_size) diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index fce40c8414fd8..ae3123b6b59f3 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -432,8 +432,6 @@ class Rasterizer final : public SnapshotDelegate { bool EnsureThreadsAreMerged(); - void UnMergeNow(); - private: Delegate& delegate_; TaskRunners task_runners_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 6f1e896303fdc..de2c185f3d133 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -710,13 +710,9 @@ void Shell::OnPlatformViewDestroyed() { // surface is about to go away. fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), ui_task); latch.Wait(); - if (rasterizer_->EnsureThreadsAreMerged()) { - raster_task(); - rasterizer_->UnMergeNow(); - } else { - fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), - raster_task); - } + rasterizer_->EnsureThreadsAreMerged(); + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), + raster_task); latch.Wait(); FML_DLOG(ERROR) << "shell unmerged"; } From c1975726782de9b71bf0b5e0dbc501f01220d56b Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 4 Aug 2020 10:52:22 -0700 Subject: [PATCH 03/16] atomic -> atomic_bool --- fml/raster_thread_merger.cc | 19 +++++++++---------- fml/raster_thread_merger.h | 2 +- .../ios/framework/Source/FlutterEngine.mm | 2 +- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index c2454483a412b..dd9a224523167 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -17,18 +17,17 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, gpu_queue_id_(gpu_queue_id), task_queues_(fml::MessageLoopTaskQueues::GetInstance()), lease_term_(kLeaseNotSet) { - bool is_merged = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); - is_merged_.store(is_merged); - if (is_merged) { + is_merged_ = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); + if (is_merged_) { lease_term_ = 1; } } void RasterThreadMerger::MergeWithLease(size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; - if (!is_merged_.load()) { + if (!is_merged_) { FML_DLOG(ERROR) << "--- tm_: merged"; - is_merged_.store(task_queues_->Merge(platform_queue_id_, gpu_queue_id_)); + is_merged_ = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); lease_term_ = lease_term; } } @@ -38,7 +37,7 @@ void RasterThreadMerger::UnMergeNow() { lease_term_ = 0; bool success = task_queues_->Unmerge(platform_queue_id_); FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; - is_merged_.store(false); + is_merged_ = false; FML_DLOG(ERROR) << "--- tm_: unmerged"; } @@ -47,7 +46,7 @@ bool RasterThreadMerger::IsOnPlatformThread() const { } bool RasterThreadMerger::IsOnRasterizingThread() const { - if (is_merged_.load()) { + if (is_merged_) { return IsOnPlatformThread(); } else { return !IsOnPlatformThread(); @@ -63,17 +62,17 @@ void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { } bool RasterThreadMerger::IsMerged() const { - return is_merged_.load(); + return is_merged_; } void RasterThreadMerger::WaitUntilMerged() { FML_CHECK(MessageLoop::GetCurrentTaskQueueId() != gpu_queue_id_); - while (!is_merged_.load()) + while (!is_merged_) ; } RasterThreadStatus RasterThreadMerger::DecrementLease() { - if (!is_merged_.load()) { + if (!is_merged_) { return RasterThreadStatus::kRemainsUnmerged; } diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 101eb4b6ee899..95af30f734f13 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -59,7 +59,7 @@ class RasterThreadMerger fml::TaskQueueId gpu_queue_id_; fml::RefPtr task_queues_; std::atomic_int lease_term_; - std::atomic is_merged_; + std::atomic_bool is_merged_; FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index b5c115e0efdca..a51cb717c587a 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -502,7 +502,7 @@ - (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryURI { ); // Create the shell. This is a blocking operation. _shell = flutter::Shell::Create(std::move(task_runners), // task runners - std::move(windowData), // window data + std::move(platformData), // window data std::move(settings), // settings on_create_platform_view, // platform view creation on_create_rasterizer // rasterzier creation From edd4dfd6509f87a35c1cfe4689a238b32bb6e068 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 4 Aug 2020 10:53:05 -0700 Subject: [PATCH 04/16] wait until merged only on platform thread --- fml/raster_thread_merger.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index dd9a224523167..3177c4579d57a 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -66,7 +66,7 @@ bool RasterThreadMerger::IsMerged() const { } void RasterThreadMerger::WaitUntilMerged() { - FML_CHECK(MessageLoop::GetCurrentTaskQueueId() != gpu_queue_id_); + FML_CHECK(IsOnPlatformThread()); while (!is_merged_) ; } From 11e014cfee147fbf71cc2db0fccd207331ad8931 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 7 Aug 2020 11:43:02 -0700 Subject: [PATCH 05/16] unit tests --- fml/raster_thread_merger.cc | 7 +- fml/raster_thread_merger.h | 4 + shell/common/rasterizer.cc | 6 + shell/common/shell.cc | 1 - shell/common/shell_test.cc | 10 + shell/common/shell_test.h | 2 + .../shell_test_external_view_embedder.cc | 15 +- .../shell_test_external_view_embedder.h | 11 +- shell/common/shell_unittests.cc | 320 ++++++++++++++++-- .../ios/framework/Source/FlutterEngine.mm | 2 +- 10 files changed, 344 insertions(+), 34 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 3177c4579d57a..b31c12d7ff69b 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -30,6 +30,7 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { is_merged_ = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); lease_term_ = lease_term; } + merged_condition_.notify_one(); } void RasterThreadMerger::UnMergeNow() { @@ -66,9 +67,11 @@ bool RasterThreadMerger::IsMerged() const { } void RasterThreadMerger::WaitUntilMerged() { + FML_DLOG(ERROR) << "--- wait until merged start"; FML_CHECK(IsOnPlatformThread()); - while (!is_merged_) - ; + std::unique_lock lock(merged_mutex_); + merged_condition_.wait(lock, [&] { return IsMerged(); }); + FML_DLOG(ERROR) << "--- wait until merged end"; } RasterThreadStatus RasterThreadMerger::DecrementLease() { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 95af30f734f13..e8e8e24e85e1d 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -5,6 +5,8 @@ #ifndef FML_SHELL_COMMON_TASK_RUNNER_MERGER_H_ #define FML_SHELL_COMMON_TASK_RUNNER_MERGER_H_ +#include +#include #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" #include "flutter/fml/message_loop_task_queues.h" @@ -60,6 +62,8 @@ class RasterThreadMerger fml::RefPtr task_queues_; std::atomic_int lease_term_; std::atomic_bool is_merged_; + std::condition_variable merged_condition_; + std::mutex merged_mutex_; FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 6264959ca535e..780f168b4000c 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -97,6 +97,7 @@ void Rasterizer::Teardown() { last_layer_tree_.reset(); if (raster_thread_merger_.get() != nullptr) { raster_thread_merger_->UnMergeNow(); + FML_DLOG(ERROR) << "Rasterizer::Teardown unmerged"; } } @@ -151,6 +152,7 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { bool result = front_continuation.Complete(std::move(resubmitted_layer_tree_)); if (result) { + FML_DLOG(ERROR) << "Draw resubmit more available"; consume_result = PipelineConsumeResult::MoreAvailable; } } else if (raster_status == RasterStatus::kEnqueuePipeline) { @@ -168,12 +170,15 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { // Consume as many pipeline items as possible. But yield the event loop // between successive tries. switch (consume_result) { + FML_DLOG(ERROR) << "Draw resubmit more available post task"; case PipelineConsumeResult::MoreAvailable: { task_runners_.GetRasterTaskRunner()->PostTask( [weak_this = weak_factory_.GetWeakPtr(), pipeline]() { + FML_DLOG(ERROR) << "Draw resubmit draw again"; if (weak_this) { weak_this->Draw(pipeline); } + FML_DLOG(ERROR) << "Draw resubmit draw again finished"; }); break; } @@ -304,6 +309,7 @@ RasterStatus Rasterizer::DoDraw( if (raster_status == RasterStatus::kSuccess) { last_layer_tree_ = std::move(layer_tree); } else if (raster_status == RasterStatus::kResubmit) { + FML_DLOG(ERROR) << "DoDraw resubmit"; resubmitted_layer_tree_ = std::move(layer_tree); return raster_status; } diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 66b4b7a5dd16a..a90965cfb1d23 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -719,7 +719,6 @@ void Shell::OnPlatformViewDestroyed() { fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), raster_task); latch.Wait(); - FML_DLOG(ERROR) << "shell unmerged"; } // |PlatformView::Delegate| diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 075f4adb3769e..8ce3d50e0ac01 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -49,6 +49,16 @@ void ShellTest::PlatformViewNotifyCreated(Shell* shell) { latch.Wait(); } +void ShellTest::PlatformViewNotifyDestroyed(Shell* shell) { + fml::AutoResetWaitableEvent latch; + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetPlatformTaskRunner(), [shell, &latch]() { + shell->GetPlatformView()->NotifyDestroyed(); + latch.Signal(); + }); + latch.Wait(); +} + void ShellTest::RunEngine(Shell* shell, RunConfiguration configuration) { fml::AutoResetWaitableEvent latch; fml::TaskRunner::RunNowOrPostTask( diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index e296ba118c73e..2a10d9a8814b0 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -50,6 +50,8 @@ class ShellTest : public FixtureTest { static void PlatformViewNotifyCreated( Shell* shell); // This creates the surface + static void PlatformViewNotifyDestroyed( + Shell* shell); // This destroys the surface static void RunEngine(Shell* shell, RunConfiguration configuration); static void RestartEngine(Shell* shell, RunConfiguration configuration); diff --git a/shell/common/shell_test_external_view_embedder.cc b/shell/common/shell_test_external_view_embedder.cc index f3a5a0a37d27d..329653436e298 100644 --- a/shell/common/shell_test_external_view_embedder.cc +++ b/shell/common/shell_test_external_view_embedder.cc @@ -2,6 +2,15 @@ namespace flutter { +void ShellTestExternalViewEmbedder::UpdatePostPrerollResult( + PostPrerollResult post_preroll_result) { + post_preroll_result_ = post_preroll_result; +} + +void ShellTestExternalViewEmbedder::SetResubmitOnce() { + resubmit_once_ = true; +} + // |ExternalViewEmbedder| void ShellTestExternalViewEmbedder::CancelFrame() {} @@ -21,6 +30,10 @@ void ShellTestExternalViewEmbedder::PrerollCompositeEmbeddedView( PostPrerollResult ShellTestExternalViewEmbedder::PostPrerollAction( fml::RefPtr raster_thread_merger) { FML_DCHECK(raster_thread_merger); + if (resubmit_once_) { + resubmit_once_ = false; + return PostPrerollResult::kResubmitFrame; + } return post_preroll_result_; } @@ -45,7 +58,7 @@ void ShellTestExternalViewEmbedder::SubmitFrame( void ShellTestExternalViewEmbedder::EndFrame( bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { - end_frame_call_back_(should_resubmit_frame); + end_frame_call_back_(should_resubmit_frame, raster_thread_merger); } // |ExternalViewEmbedder| diff --git a/shell/common/shell_test_external_view_embedder.h b/shell/common/shell_test_external_view_embedder.h index c67772df74f9a..5473569628b04 100644 --- a/shell/common/shell_test_external_view_embedder.h +++ b/shell/common/shell_test_external_view_embedder.h @@ -15,7 +15,8 @@ namespace flutter { /// class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { public: - using EndFrameCallBack = std::function; + using EndFrameCallBack = + std::function)>; ShellTestExternalViewEmbedder(const EndFrameCallBack& end_frame_call_back, PostPrerollResult post_preroll_result) @@ -24,6 +25,11 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { ~ShellTestExternalViewEmbedder() = default; + void UpdatePostPrerollResult(PostPrerollResult post_preroll_result); + + // Resubmit the layer tree to trigger thread merging onces + void SetResubmitOnce(); + private: // |ExternalViewEmbedder| void CancelFrame() override; @@ -63,7 +69,8 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { SkCanvas* GetRootCanvas() override; const EndFrameCallBack end_frame_call_back_; - const PostPrerollResult post_preroll_result_; + PostPrerollResult post_preroll_result_; + bool resubmit_once_; FML_DISALLOW_COPY_AND_ASSIGN(ShellTestExternalViewEmbedder); }; diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index f7e68679a9de2..232c7ec8844f7 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -63,6 +63,32 @@ static bool ValidateShell(Shell* shell) { return true; } +static bool RasterizerHasLayerTree(Shell* shell) { + fml::AutoResetWaitableEvent latch; + bool has_layer_tree = false; + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetRasterTaskRunner(), + [shell, &latch, &has_layer_tree]() { + has_layer_tree = shell->GetRasterizer()->GetLastLayerTree() != nullptr; + latch.Signal(); + }); + latch.Wait(); + return has_layer_tree; +} + +static void ValidateDestroyPlatformView(Shell* shell) { + ASSERT_TRUE(shell != nullptr); + ASSERT_TRUE(shell->IsSetup()); + + // To validate destroy platform view, we must ensure the rasterizer has a + // layer tree before the platform view is destroyed. + ASSERT_TRUE(RasterizerHasLayerTree(shell)); + + ShellTest::PlatformViewNotifyDestroyed(shell); + // Validate the layer tree is destroyed + ASSERT_FALSE(RasterizerHasLayerTree(shell)); +} + TEST_F(ShellTest, InitializeWithInvalidThreads) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); @@ -310,8 +336,8 @@ TEST_F(ShellTest, NoNeedToReportTimingsByDefault) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); ASSERT_FALSE(GetNeedsReportTimings(shell.get())); // This assertion may or may not be the direct result of needs_report_timings_ @@ -338,8 +364,8 @@ TEST_F(ShellTest, NeedsReportTimingsIsSetWithCallback) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("dummyReportTimingsMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); ASSERT_TRUE(GetNeedsReportTimings(shell.get())); DestroyShell(std::move(shell)); } @@ -392,8 +418,8 @@ TEST_F(ShellTest, DISABLED_ReportTimingsIsCalled) { // Pump many frames so we can trigger the report quickly instead of waiting // for the 1 second threshold. + PumpOneFrame(shell.get()); for (int i = 0; i < 200; i += 1) { - PumpOneFrame(shell.get()); } reportLatch.Wait(); @@ -453,7 +479,6 @@ TEST_F(ShellTest, FrameRasterizedCallbackIsCalled) { CREATE_NATIVE_ENTRY(nativeOnBeginFrame)); RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get()); // Check that timing is properly set. This implies that @@ -479,11 +504,13 @@ TEST_F(ShellTest, auto settings = CreateSettingsForFixture(); fml::AutoResetWaitableEvent endFrameLatch; bool end_frame_called = false; - auto end_frame_callback = [&](bool should_resubmit_frame) { - ASSERT_TRUE(should_resubmit_frame); - end_frame_called = true; - endFrameLatch.Signal(); - }; + auto end_frame_callback = + [&](bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) { + ASSERT_TRUE(should_resubmit_frame); + end_frame_called = true; + endFrameLatch.Signal(); + }; auto external_view_embedder = std::make_shared( end_frame_callback, PostPrerollResult::kResubmitFrame); auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(), @@ -511,14 +538,255 @@ TEST_F(ShellTest, flutter::SkiaGPUObject({sk_picture, queue}), false, false); root->Add(picture_layer); }; - PumpOneFrame(shell.get(), 100, 100, builder); + endFrameLatch.Wait(); ASSERT_TRUE(end_frame_called); DestroyShell(std::move(shell)); } + +TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { + const size_t ThreadMergingLease = 10; + auto settings = CreateSettingsForFixture(); + fml::AutoResetWaitableEvent endFrameLatch; + auto end_frame_callback = + [&](bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) { + if (should_resubmit_frame && !raster_thread_merger->IsMerged()) { + raster_thread_merger->MergeWithLease(ThreadMergingLease); + } + endFrameLatch.Signal(); + }; + auto external_view_embedder = std::make_shared( + end_frame_callback, PostPrerollResult::kSuccess); + // Set resubmit once to trigger thread merging. + external_view_embedder->SetResubmitOnce(); + auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(), + false, external_view_embedder); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + LayerTreeBuilder builder = [&](std::shared_ptr root) { + SkPictureRecorder recorder; + SkCanvas* recording_canvas = + recorder.beginRecording(SkRect::MakeXYWH(0, 0, 80, 80)); + recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), + SkPaint(SkColor4f::FromColor(SK_ColorRED))); + auto sk_picture = recorder.finishRecordingAsPicture(); + fml::RefPtr queue = fml::MakeRefCounted( + this->GetCurrentTaskRunner(), fml::TimeDelta::FromSeconds(0)); + auto picture_layer = std::make_shared( + SkPoint::Make(10, 10), + flutter::SkiaGPUObject({sk_picture, queue}), false, false); + root->Add(picture_layer); + }; + + PumpOneFrame(shell.get(), 100, 100, builder); + // Pump one frame to trigger thread merging. + endFrameLatch.Wait(); + // Pump another frame to ensure threads are merged and a regular layer tree is + // submitted. + PumpOneFrame(shell.get(), 100, 100, builder); + // Threads are merged here. PlatformViewNotifyDestroy should be executed + // successfully. + ASSERT_TRUE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + ValidateDestroyPlatformView(shell.get()); + + // Ensure threads are unmerged after platform view destroy + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + + // Validate the platform view can be recreated and destroyed again + ValidateShell(shell.get()); + + DestroyShell(std::move(shell)); +} + +TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { + const size_t ThreadMergingLease = 10; + auto settings = CreateSettingsForFixture(); + fml::AutoResetWaitableEvent endFrameLatch; + auto end_frame_callback = + [&](bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) { + FML_DLOG(ERROR) << "END FRAME called"; + if (should_resubmit_frame && !raster_thread_merger->IsMerged()) { + FML_DLOG(ERROR) << "END FRAME merge threads"; + raster_thread_merger->MergeWithLease(ThreadMergingLease); + } + endFrameLatch.Signal(); + }; + // Start with a regular layer tree with `PostPrerollResult::kSuccess` so we + // can later check if the rasterizer is tore down using + // |ValidateDestroyPlatformView| + auto external_view_embedder = std::make_shared( + end_frame_callback, PostPrerollResult::kSuccess); + + auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(), + false, external_view_embedder); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + LayerTreeBuilder builder = [&](std::shared_ptr root) { + SkPictureRecorder recorder; + SkCanvas* recording_canvas = + recorder.beginRecording(SkRect::MakeXYWH(0, 0, 80, 80)); + recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), + SkPaint(SkColor4f::FromColor(SK_ColorRED))); + auto sk_picture = recorder.finishRecordingAsPicture(); + fml::RefPtr queue = fml::MakeRefCounted( + this->GetCurrentTaskRunner(), fml::TimeDelta::FromSeconds(0)); + auto picture_layer = std::make_shared( + SkPoint::Make(10, 10), + flutter::SkiaGPUObject({sk_picture, queue}), false, false); + root->Add(picture_layer); + }; + + PumpOneFrame(shell.get(), 100, 100, builder); + // Pump one frame and threads aren't merged + endFrameLatch.Wait(); + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + + // Pump a frame with `PostPrerollResult::kResubmitFrame` to start merging + // threads + external_view_embedder->SetResubmitOnce(); + PumpOneFrame(shell.get(), 100, 100, builder); + + // Now destroy the platform view immediately. + // Two things can happen here: + // 1. Threads haven't merged. 2. Threads has already merged. + // |Shell:OnPlatformViewDestroy| should be able to handle both cases. + ValidateDestroyPlatformView(shell.get()); + + // Ensure threads are unmerged after platform view destroy + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + + // Validate the platform view can be recreated and destroyed again + ValidateShell(shell.get()); + + DestroyShell(std::move(shell)); +} + +TEST_F(ShellTest, + OnPlatformViewDestroyWithThreadMergerWhileThreadsAreUnmerged) { + auto settings = CreateSettingsForFixture(); + + auto end_frame_callback = + [&](bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) {}; + auto external_view_embedder = std::make_shared( + end_frame_callback, PostPrerollResult::kSuccess); + auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(), + false, external_view_embedder); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + LayerTreeBuilder builder = [&](std::shared_ptr root) { + SkPictureRecorder recorder; + SkCanvas* recording_canvas = + recorder.beginRecording(SkRect::MakeXYWH(0, 0, 80, 80)); + recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), + SkPaint(SkColor4f::FromColor(SK_ColorRED))); + auto sk_picture = recorder.finishRecordingAsPicture(); + fml::RefPtr queue = fml::MakeRefCounted( + this->GetCurrentTaskRunner(), fml::TimeDelta::FromSeconds(0)); + auto picture_layer = std::make_shared( + SkPoint::Make(10, 10), + flutter::SkiaGPUObject({sk_picture, queue}), false, false); + root->Add(picture_layer); + }; + PumpOneFrame(shell.get(), 100, 100, builder); + + // Threads should not be merged. + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + ValidateDestroyPlatformView(shell.get()); + + // Ensure threads are unmerged after platform view destroy + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + + // Validate the platform view can be recreated and destroyed again + ValidateShell(shell.get()); + + DestroyShell(std::move(shell)); +} + +TEST_F(ShellTest, OnPlatformViewDestroyWithoutRasterThreadMerger) { + auto settings = CreateSettingsForFixture(); + + auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(), + false, nullptr); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + LayerTreeBuilder builder = [&](std::shared_ptr root) { + SkPictureRecorder recorder; + SkCanvas* recording_canvas = + recorder.beginRecording(SkRect::MakeXYWH(0, 0, 80, 80)); + recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), + SkPaint(SkColor4f::FromColor(SK_ColorRED))); + auto sk_picture = recorder.finishRecordingAsPicture(); + fml::RefPtr queue = fml::MakeRefCounted( + this->GetCurrentTaskRunner(), fml::TimeDelta::FromSeconds(0)); + auto picture_layer = std::make_shared( + SkPoint::Make(10, 10), + flutter::SkiaGPUObject({sk_picture, queue}), false, false); + root->Add(picture_layer); + }; + PumpOneFrame(shell.get(), 100, 100, builder); + + // Threads should not be merged. + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + ValidateDestroyPlatformView(shell.get()); + + // Ensure threads are unmerged after platform view destroy + ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( + shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), + shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); + + // Validate the platform view can be recreated and destroyed again + ValidateShell(shell.get()); + + DestroyShell(std::move(shell)); +} #endif TEST(SettingsTest, FrameTimingSetsAndGetsProperly) { @@ -568,8 +836,8 @@ TEST_F(ShellTest, ReportTimingsIsCalledSoonerInNonReleaseMode) { AddNativeCallback("NativeReportTimingsCallback", CREATE_NATIVE_ENTRY(nativeTimingCallback)); RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get()); + PumpOneFrame(shell.get()); reportLatch.Wait(); @@ -612,8 +880,8 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) { CREATE_NATIVE_ENTRY(nativeTimingCallback)); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); for (int i = 0; i < 10; i += 1) { - PumpOneFrame(shell.get()); } reportLatch.Wait(); @@ -664,8 +932,8 @@ TEST_F(ShellTest, WaitForFirstFrame) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_TRUE(result.ok()); @@ -683,8 +951,8 @@ TEST_F(ShellTest, WaitForFirstFrameZeroSizeFrame) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get(), {1.0, 0.0, 0.0}); + RunEngine(shell.get(), std::move(configuration)); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_FALSE(result.ok()); @@ -722,8 +990,8 @@ TEST_F(ShellTest, WaitForFirstFrameMultiple) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_TRUE(result.ok()); @@ -751,8 +1019,8 @@ TEST_F(ShellTest, WaitForFirstFrameInlined) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); fml::AutoResetWaitableEvent event; task_runner->PostTask([&shell, &event] { fml::Status result = @@ -794,8 +1062,8 @@ TEST_F(ShellTest, SetResourceCacheSize) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), static_cast(24 * (1 << 20))); @@ -803,8 +1071,8 @@ TEST_F(ShellTest, SetResourceCacheSize) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 400, 200}); + PumpOneFrame(shell.get()); }); - PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), 3840000U); @@ -815,15 +1083,15 @@ TEST_F(ShellTest, SetResourceCacheSize) { std::vector data(request_json.begin(), request_json.end()); auto platform_message = fml::MakeRefCounted( "flutter/skia", std::move(data), nullptr); - SendEnginePlatformMessage(shell.get(), std::move(platform_message)); PumpOneFrame(shell.get()); + SendEnginePlatformMessage(shell.get(), std::move(platform_message)); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), 10000U); fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 800, 400}); + PumpOneFrame(shell.get()); }); - PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), 10000U); DestroyShell(std::move(shell), std::move(task_runners)); @@ -840,8 +1108,8 @@ TEST_F(ShellTest, SetResourceCacheSizeEarly) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 400, 200}); + PumpOneFrame(shell.get()); }); - PumpOneFrame(shell.get()); // Create the surface needed by rasterizer PlatformViewNotifyCreated(shell.get()); @@ -849,8 +1117,8 @@ TEST_F(ShellTest, SetResourceCacheSizeEarly) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), static_cast(3840000U)); @@ -868,8 +1136,8 @@ TEST_F(ShellTest, SetResourceCacheSizeNotifiesDart) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 400, 200}); + PumpOneFrame(shell.get()); }); - PumpOneFrame(shell.get()); // Create the surface needed by rasterizer PlatformViewNotifyCreated(shell.get()); @@ -885,8 +1153,8 @@ TEST_F(ShellTest, SetResourceCacheSizeNotifiesDart) { latch.Signal(); })); - RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + RunEngine(shell.get(), std::move(configuration)); latch.Wait(); @@ -1074,8 +1342,8 @@ TEST_F(ShellTest, Screenshot) { flutter::SkiaGPUObject({sk_picture, queue}), false, false); root->Add(picture_layer); }; - PumpOneFrame(shell.get(), 100, 100, builder); + firstFrameLatch.Wait(); std::promise screenshot_promise; @@ -1290,7 +1558,6 @@ TEST_F(ShellTest, RasterizerScreenshot) { RunEngine(shell.get(), std::move(configuration)); auto latch = std::make_shared(); - PumpOneFrame(shell.get()); fml::TaskRunner::RunNowOrPostTask( @@ -1321,7 +1588,6 @@ TEST_F(ShellTest, RasterizerMakeRasterSnapshot) { RunEngine(shell.get(), std::move(configuration)); auto latch = std::make_shared(); - PumpOneFrame(shell.get()); fml::TaskRunner::RunNowOrPostTask( diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index a51cb717c587a..5f56f424ec26e 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -502,7 +502,7 @@ - (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryURI { ); // Create the shell. This is a blocking operation. _shell = flutter::Shell::Create(std::move(task_runners), // task runners - std::move(platformData), // window data + std::move(platformData), // window data std::move(settings), // settings on_create_platform_view, // platform view creation on_create_rasterizer // rasterzier creation From cdbee3dd519d39b212a9bf2b334b394d8d0c976c Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 7 Aug 2020 12:11:16 -0700 Subject: [PATCH 06/16] cleanup --- shell/common/shell_unittests.cc | 38 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 232c7ec8844f7..5f3e8aefaac95 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -336,8 +336,8 @@ TEST_F(ShellTest, NoNeedToReportTimingsByDefault) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); ASSERT_FALSE(GetNeedsReportTimings(shell.get())); // This assertion may or may not be the direct result of needs_report_timings_ @@ -364,8 +364,8 @@ TEST_F(ShellTest, NeedsReportTimingsIsSetWithCallback) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("dummyReportTimingsMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); ASSERT_TRUE(GetNeedsReportTimings(shell.get())); DestroyShell(std::move(shell)); } @@ -418,8 +418,8 @@ TEST_F(ShellTest, DISABLED_ReportTimingsIsCalled) { // Pump many frames so we can trigger the report quickly instead of waiting // for the 1 second threshold. - PumpOneFrame(shell.get()); for (int i = 0; i < 200; i += 1) { + PumpOneFrame(shell.get()); } reportLatch.Wait(); @@ -538,8 +538,8 @@ TEST_F(ShellTest, flutter::SkiaGPUObject({sk_picture, queue}), false, false); root->Add(picture_layer); }; - PumpOneFrame(shell.get(), 100, 100, builder); + PumpOneFrame(shell.get(), 100, 100, builder); endFrameLatch.Wait(); ASSERT_TRUE(end_frame_called); @@ -836,8 +836,8 @@ TEST_F(ShellTest, ReportTimingsIsCalledSoonerInNonReleaseMode) { AddNativeCallback("NativeReportTimingsCallback", CREATE_NATIVE_ENTRY(nativeTimingCallback)); RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get()); + PumpOneFrame(shell.get()); PumpOneFrame(shell.get()); reportLatch.Wait(); @@ -932,8 +932,8 @@ TEST_F(ShellTest, WaitForFirstFrame) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_TRUE(result.ok()); @@ -951,8 +951,8 @@ TEST_F(ShellTest, WaitForFirstFrameZeroSizeFrame) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get(), {1.0, 0.0, 0.0}); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get(), {1.0, 0.0, 0.0}); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_FALSE(result.ok()); @@ -990,8 +990,8 @@ TEST_F(ShellTest, WaitForFirstFrameMultiple) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_TRUE(result.ok()); @@ -1019,8 +1019,8 @@ TEST_F(ShellTest, WaitForFirstFrameInlined) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); fml::AutoResetWaitableEvent event; task_runner->PostTask([&shell, &event] { fml::Status result = @@ -1062,8 +1062,8 @@ TEST_F(ShellTest, SetResourceCacheSize) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), static_cast(24 * (1 << 20))); @@ -1071,8 +1071,8 @@ TEST_F(ShellTest, SetResourceCacheSize) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 400, 200}); - PumpOneFrame(shell.get()); }); + PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), 3840000U); @@ -1083,15 +1083,15 @@ TEST_F(ShellTest, SetResourceCacheSize) { std::vector data(request_json.begin(), request_json.end()); auto platform_message = fml::MakeRefCounted( "flutter/skia", std::move(data), nullptr); - PumpOneFrame(shell.get()); SendEnginePlatformMessage(shell.get(), std::move(platform_message)); + PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), 10000U); fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 800, 400}); - PumpOneFrame(shell.get()); }); + PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), 10000U); DestroyShell(std::move(shell), std::move(task_runners)); @@ -1108,8 +1108,8 @@ TEST_F(ShellTest, SetResourceCacheSizeEarly) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 400, 200}); - PumpOneFrame(shell.get()); }); + PumpOneFrame(shell.get()); // Create the surface needed by rasterizer PlatformViewNotifyCreated(shell.get()); @@ -1117,8 +1117,8 @@ TEST_F(ShellTest, SetResourceCacheSizeEarly) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), static_cast(3840000U)); @@ -1136,8 +1136,8 @@ TEST_F(ShellTest, SetResourceCacheSizeNotifiesDart) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { shell->GetPlatformView()->SetViewportMetrics({1.0, 400, 200}); - PumpOneFrame(shell.get()); }); + PumpOneFrame(shell.get()); // Create the surface needed by rasterizer PlatformViewNotifyCreated(shell.get()); @@ -1153,8 +1153,8 @@ TEST_F(ShellTest, SetResourceCacheSizeNotifiesDart) { latch.Signal(); })); - PumpOneFrame(shell.get()); RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); latch.Wait(); @@ -1342,8 +1342,8 @@ TEST_F(ShellTest, Screenshot) { flutter::SkiaGPUObject({sk_picture, queue}), false, false); root->Add(picture_layer); }; - PumpOneFrame(shell.get(), 100, 100, builder); + PumpOneFrame(shell.get(), 100, 100, builder); firstFrameLatch.Wait(); std::promise screenshot_promise; @@ -1558,6 +1558,7 @@ TEST_F(ShellTest, RasterizerScreenshot) { RunEngine(shell.get(), std::move(configuration)); auto latch = std::make_shared(); + PumpOneFrame(shell.get()); fml::TaskRunner::RunNowOrPostTask( @@ -1588,6 +1589,7 @@ TEST_F(ShellTest, RasterizerMakeRasterSnapshot) { RunEngine(shell.get(), std::move(configuration)); auto latch = std::make_shared(); + PumpOneFrame(shell.get()); fml::TaskRunner::RunNowOrPostTask( From 292a647729334c53a545480fc4fb3b15f8601700 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 7 Aug 2020 12:21:46 -0700 Subject: [PATCH 07/16] fix some tests --- .../shell_test_external_view_embedder.cc | 8 ++++++ .../shell_test_external_view_embedder.h | 4 +-- shell/common/shell_unittests.cc | 25 +++++++++++-------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/shell/common/shell_test_external_view_embedder.cc b/shell/common/shell_test_external_view_embedder.cc index 329653436e298..bbefe391a334f 100644 --- a/shell/common/shell_test_external_view_embedder.cc +++ b/shell/common/shell_test_external_view_embedder.cc @@ -2,6 +2,14 @@ namespace flutter { +ShellTestExternalViewEmbedder::ShellTestExternalViewEmbedder( + const EndFrameCallBack& end_frame_call_back, + PostPrerollResult post_preroll_result) + : end_frame_call_back_(end_frame_call_back), + post_preroll_result_(post_preroll_result) { + resubmit_once_ = false; +} + void ShellTestExternalViewEmbedder::UpdatePostPrerollResult( PostPrerollResult post_preroll_result) { post_preroll_result_ = post_preroll_result; diff --git a/shell/common/shell_test_external_view_embedder.h b/shell/common/shell_test_external_view_embedder.h index 5473569628b04..c532aea676c01 100644 --- a/shell/common/shell_test_external_view_embedder.h +++ b/shell/common/shell_test_external_view_embedder.h @@ -19,9 +19,7 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { std::function)>; ShellTestExternalViewEmbedder(const EndFrameCallBack& end_frame_call_back, - PostPrerollResult post_preroll_result) - : end_frame_call_back_(end_frame_call_back), - post_preroll_result_(post_preroll_result) {} + PostPrerollResult post_preroll_result); ~ShellTestExternalViewEmbedder() = default; diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 5f3e8aefaac95..72eb5c9dec5aa 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -502,14 +502,14 @@ TEST_F(ShellTest, FrameRasterizedCallbackIsCalled) { TEST_F(ShellTest, ExternalEmbedderEndFrameIsCalledWhenPostPrerollResultIsResubmit) { auto settings = CreateSettingsForFixture(); - fml::AutoResetWaitableEvent endFrameLatch; + fml::AutoResetWaitableEvent end_frame_latch; bool end_frame_called = false; auto end_frame_callback = [&](bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { ASSERT_TRUE(should_resubmit_frame); end_frame_called = true; - endFrameLatch.Signal(); + end_frame_latch.Signal(); }; auto external_view_embedder = std::make_shared( end_frame_callback, PostPrerollResult::kResubmitFrame); @@ -540,7 +540,7 @@ TEST_F(ShellTest, }; PumpOneFrame(shell.get(), 100, 100, builder); - endFrameLatch.Wait(); + end_frame_latch.Wait(); ASSERT_TRUE(end_frame_called); @@ -550,14 +550,14 @@ TEST_F(ShellTest, TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { const size_t ThreadMergingLease = 10; auto settings = CreateSettingsForFixture(); - fml::AutoResetWaitableEvent endFrameLatch; + fml::AutoResetWaitableEvent end_frame_latch; auto end_frame_callback = [&](bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { if (should_resubmit_frame && !raster_thread_merger->IsMerged()) { raster_thread_merger->MergeWithLease(ThreadMergingLease); } - endFrameLatch.Signal(); + end_frame_latch.Signal(); }; auto external_view_embedder = std::make_shared( end_frame_callback, PostPrerollResult::kSuccess); @@ -591,7 +591,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { PumpOneFrame(shell.get(), 100, 100, builder); // Pump one frame to trigger thread merging. - endFrameLatch.Wait(); + end_frame_latch.Wait(); // Pump another frame to ensure threads are merged and a regular layer tree is // submitted. PumpOneFrame(shell.get(), 100, 100, builder); @@ -616,7 +616,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { const size_t ThreadMergingLease = 10; auto settings = CreateSettingsForFixture(); - fml::AutoResetWaitableEvent endFrameLatch; + fml::AutoResetWaitableEvent end_frame_latch; auto end_frame_callback = [&](bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { @@ -625,7 +625,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { FML_DLOG(ERROR) << "END FRAME merge threads"; raster_thread_merger->MergeWithLease(ThreadMergingLease); } - endFrameLatch.Signal(); + end_frame_latch.Signal(); }; // Start with a regular layer tree with `PostPrerollResult::kSuccess` so we // can later check if the rasterizer is tore down using @@ -661,7 +661,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { PumpOneFrame(shell.get(), 100, 100, builder); // Pump one frame and threads aren't merged - endFrameLatch.Wait(); + end_frame_latch.Wait(); ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( shell->GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(), shell->GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId())); @@ -691,10 +691,12 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { TEST_F(ShellTest, OnPlatformViewDestroyWithThreadMergerWhileThreadsAreUnmerged) { auto settings = CreateSettingsForFixture(); - + fml::AutoResetWaitableEvent end_frame_latch; auto end_frame_callback = [&](bool should_resubmit_frame, - fml::RefPtr raster_thread_merger) {}; + fml::RefPtr raster_thread_merger) { + end_frame_latch.Signal(); + }; auto external_view_embedder = std::make_shared( end_frame_callback, PostPrerollResult::kSuccess); auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(), @@ -723,6 +725,7 @@ TEST_F(ShellTest, root->Add(picture_layer); }; PumpOneFrame(shell.get(), 100, 100, builder); + end_frame_latch.Wait(); // Threads should not be merged. ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( From 986398585400aa171a2b805a885f12cd4ddea35c Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 7 Aug 2020 12:25:22 -0700 Subject: [PATCH 08/16] fix typo --- shell/common/shell_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 72eb5c9dec5aa..b292e761117ec 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -883,8 +883,8 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) { CREATE_NATIVE_ENTRY(nativeTimingCallback)); RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get()); for (int i = 0; i < 10; i += 1) { + PumpOneFrame(shell.get()); } reportLatch.Wait(); From 0c6e1c3adae2a56549add06cd7c58d55054fb747 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 7 Aug 2020 17:19:15 -0700 Subject: [PATCH 09/16] Rasterizer::EnsureThreadsAreMerged works with no surface --- shell/common/rasterizer.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 780f168b4000c..4c78c921e1839 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -95,7 +95,8 @@ void Rasterizer::Teardown() { compositor_context_->OnGrContextDestroyed(); surface_.reset(); last_layer_tree_.reset(); - if (raster_thread_merger_.get() != nullptr) { + if (raster_thread_merger_.get() != nullptr && + raster_thread_merger_.get()->IsMerged()) { raster_thread_merger_->UnMergeNow(); FML_DLOG(ERROR) << "Rasterizer::Teardown unmerged"; } @@ -680,7 +681,8 @@ std::optional Rasterizer::GetResourceCacheMaxBytes() const { } bool Rasterizer::EnsureThreadsAreMerged() { - if (raster_thread_merger_.get() == nullptr) { + if (surface_ == nullptr || raster_thread_merger_.get() == nullptr) { + FML_DLOG(ERROR) << "--- no surface_"; return false; } fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), From b4a37cb62ca900d43a3702fb541c470e3c3848a0 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Sat, 8 Aug 2020 11:19:06 -0700 Subject: [PATCH 10/16] support static thread merging --- shell/common/rasterizer.cc | 11 ++++++- shell/common/shell.cc | 40 +++++++++++++++++++---- shell/common/shell_unittests.cc | 56 +++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 7 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 4c78c921e1839..0eaa7701dcb28 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -81,7 +81,11 @@ void Rasterizer::Setup(std::unique_ptr surface) { #if !defined(OS_FUCHSIA) // TODO(sanjayc77): https://github.com/flutter/flutter/issues/53179. Add // support for raster thread merger for Fuchsia. - if (surface_->GetExternalViewEmbedder()) { + if (surface_->GetExternalViewEmbedder() && + // Don't create raster_thread_merger if platform and raster task runners + // are the same. + task_runners_.GetRasterTaskRunner() != + task_runners_.GetPlatformTaskRunner()) { const auto platform_id = task_runners_.GetPlatformTaskRunner()->GetTaskQueueId(); const auto gpu_id = task_runners_.GetRasterTaskRunner()->GetTaskQueueId(); @@ -681,6 +685,11 @@ std::optional Rasterizer::GetResourceCacheMaxBytes() const { } bool Rasterizer::EnsureThreadsAreMerged() { + // If threads are merged statically, always return true. + if (task_runners_.GetRasterTaskRunner() == + task_runners_.GetPlatformTaskRunner()) { + return true; + } if (surface_ == nullptr || raster_thread_merger_.get() == nullptr) { FML_DLOG(ERROR) << "--- no surface_"; return false; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a90965cfb1d23..abdf9e9e6d98e 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -614,6 +614,7 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { TRACE_EVENT0("flutter", "Shell::OnPlatformViewCreated"); FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); + // Note: // This is a synchronous operation because certain platforms depend on // setup/suspension of all activities that may be interacting with the GPU in @@ -635,13 +636,36 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { latch.Signal(); }); - auto ui_task = [engine = engine_->GetWeakPtr(), // - &latch // + // The normal flow executed by this method is that the platform thread is + // 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 + // 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 executing + // raster_task. + bool should_post_raster_task = task_runners_.GetRasterTaskRunner() != + task_runners_.GetPlatformTaskRunner(); + + auto ui_task = [engine = engine_->GetWeakPtr(), // + raster_task_runner = task_runners_.GetRasterTaskRunner(), // + raster_task, should_post_raster_task, + &latch // ] { if (engine) { engine->OnOutputSurfaceCreated(); } - latch.Signal(); + // Step 2: Next, tell the raster thread that it should create a surface for + // its rasterizer. + if (should_post_raster_task) { + fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); + } else { + // See comment on should_post_raster_task, in this case we just unblock + // the platform thread. + latch.Signal(); + } }; // Threading: Capture platform view by raw pointer and not the weak pointer. @@ -664,10 +688,14 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { }; fml::TaskRunner::RunNowOrPostTask(task_runners_.GetIOTaskRunner(), io_task); + latch.Wait(); - fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), - raster_task); - latch.Wait(); + if (!should_post_raster_task) { + // See comment on should_post_raster_task, in this case the raster_task + // wasn't executed, and we just run it here as the platform thread + // is the raster thread. + raster_task(); + } } // |PlatformView::Delegate| diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index b292e761117ec..1a669cb62172d 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -792,6 +792,62 @@ TEST_F(ShellTest, OnPlatformViewDestroyWithoutRasterThreadMerger) { } #endif +TEST_F(ShellTest, OnPlatformViewDestroyWithStaticThreadMerging) { + auto settings = CreateSettingsForFixture(); + fml::AutoResetWaitableEvent end_frame_latch; + auto end_frame_callback = + [&](bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) { + end_frame_latch.Signal(); + }; + auto external_view_embedder = std::make_shared( + end_frame_callback, PostPrerollResult::kSuccess); + ThreadHost thread_host( + "io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform | ThreadHost::Type::IO | ThreadHost::Type::UI); + TaskRunners task_runners( + "test", + thread_host.platform_thread->GetTaskRunner(), // platform + thread_host.platform_thread->GetTaskRunner(), // raster + thread_host.ui_thread->GetTaskRunner(), // ui + thread_host.io_thread->GetTaskRunner() // io + ); + auto shell = CreateShell(std::move(settings), std::move(task_runners), false, + external_view_embedder); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + LayerTreeBuilder builder = [&](std::shared_ptr root) { + SkPictureRecorder recorder; + SkCanvas* recording_canvas = + recorder.beginRecording(SkRect::MakeXYWH(0, 0, 80, 80)); + recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), + SkPaint(SkColor4f::FromColor(SK_ColorRED))); + auto sk_picture = recorder.finishRecordingAsPicture(); + fml::RefPtr queue = fml::MakeRefCounted( + this->GetCurrentTaskRunner(), fml::TimeDelta::FromSeconds(0)); + auto picture_layer = std::make_shared( + SkPoint::Make(10, 10), + flutter::SkiaGPUObject({sk_picture, queue}), false, false); + root->Add(picture_layer); + }; + PumpOneFrame(shell.get(), 100, 100, builder); + end_frame_latch.Wait(); + + ValidateDestroyPlatformView(shell.get()); + + // Validate the platform view can be recreated and destroyed again + ValidateShell(shell.get()); + + DestroyShell(std::move(shell), std::move(task_runners)); +} + TEST(SettingsTest, FrameTimingSetsAndGetsProperly) { // Ensure that all phases are in kPhases. ASSERT_EQ(sizeof(FrameTiming::kPhases), From 566d3da1276af3add7bc5e989027b980509fe1c3 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Sat, 8 Aug 2020 11:33:05 -0700 Subject: [PATCH 11/16] remove all debugging logs and add doc/comments --- fml/raster_thread_merger.cc | 4 ---- fml/raster_thread_merger.h | 6 ++++++ shell/common/rasterizer.cc | 9 --------- shell/common/rasterizer.h | 14 ++++++++++++++ shell/common/shell.cc | 1 - shell/common/shell_test_external_view_embedder.h | 5 ++++- shell/common/shell_unittests.cc | 2 -- 7 files changed, 24 insertions(+), 17 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index b31c12d7ff69b..8f673402de105 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -26,7 +26,6 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, void RasterThreadMerger::MergeWithLease(size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; if (!is_merged_) { - FML_DLOG(ERROR) << "--- tm_: merged"; is_merged_ = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); lease_term_ = lease_term; } @@ -39,7 +38,6 @@ void RasterThreadMerger::UnMergeNow() { bool success = task_queues_->Unmerge(platform_queue_id_); FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; is_merged_ = false; - FML_DLOG(ERROR) << "--- tm_: unmerged"; } bool RasterThreadMerger::IsOnPlatformThread() const { @@ -67,11 +65,9 @@ bool RasterThreadMerger::IsMerged() const { } void RasterThreadMerger::WaitUntilMerged() { - FML_DLOG(ERROR) << "--- wait until merged start"; FML_CHECK(IsOnPlatformThread()); std::unique_lock lock(merged_mutex_); merged_condition_.wait(lock, [&] { return IsMerged(); }); - FML_DLOG(ERROR) << "--- wait until merged end"; } RasterThreadStatus RasterThreadMerger::DecrementLease() { diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index e8e8e24e85e1d..df95cfc2a4533 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -32,6 +32,9 @@ class RasterThreadMerger // unless an |ExtendLeaseTo| gets called. void MergeWithLease(size_t lease_term); + // Unmerge the threads now, resets the lease term to 0. + // + // Must be executed on the raster task runner. void UnMergeNow(); void ExtendLeaseTo(size_t lease_term); @@ -42,6 +45,9 @@ class RasterThreadMerger bool IsMerged() const; + // Wait until the threads are merged. + // + // Must run on the platform task runner. void WaitUntilMerged(); RasterThreadMerger(fml::TaskQueueId platform_queue_id, diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 0eaa7701dcb28..103c0c1b65ba3 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -102,7 +102,6 @@ void Rasterizer::Teardown() { if (raster_thread_merger_.get() != nullptr && raster_thread_merger_.get()->IsMerged()) { raster_thread_merger_->UnMergeNow(); - FML_DLOG(ERROR) << "Rasterizer::Teardown unmerged"; } } @@ -157,7 +156,6 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { bool result = front_continuation.Complete(std::move(resubmitted_layer_tree_)); if (result) { - FML_DLOG(ERROR) << "Draw resubmit more available"; consume_result = PipelineConsumeResult::MoreAvailable; } } else if (raster_status == RasterStatus::kEnqueuePipeline) { @@ -175,15 +173,12 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { // Consume as many pipeline items as possible. But yield the event loop // between successive tries. switch (consume_result) { - FML_DLOG(ERROR) << "Draw resubmit more available post task"; case PipelineConsumeResult::MoreAvailable: { task_runners_.GetRasterTaskRunner()->PostTask( [weak_this = weak_factory_.GetWeakPtr(), pipeline]() { - FML_DLOG(ERROR) << "Draw resubmit draw again"; if (weak_this) { weak_this->Draw(pipeline); } - FML_DLOG(ERROR) << "Draw resubmit draw again finished"; }); break; } @@ -314,7 +309,6 @@ RasterStatus Rasterizer::DoDraw( if (raster_status == RasterStatus::kSuccess) { last_layer_tree_ = std::move(layer_tree); } else if (raster_status == RasterStatus::kResubmit) { - FML_DLOG(ERROR) << "DoDraw resubmit"; resubmitted_layer_tree_ = std::move(layer_tree); return raster_status; } @@ -691,17 +685,14 @@ bool Rasterizer::EnsureThreadsAreMerged() { return true; } if (surface_ == nullptr || raster_thread_merger_.get() == nullptr) { - FML_DLOG(ERROR) << "--- no surface_"; return false; } fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), [weak_this = weak_factory_.GetWeakPtr(), thread_merger = raster_thread_merger_]() { if (weak_this->surface_ == nullptr) { - FML_DLOG(ERROR) << "--- no surface_"; return; } - FML_DLOG(ERROR) << "--- merge with lease"; thread_merger->MergeWithLease(10); }); raster_thread_merger_->WaitUntilMerged(); diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index 39e0a87c80a6e..2c69bab3551e6 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -430,6 +430,20 @@ class Rasterizer final : public SnapshotDelegate { /// std::optional GetResourceCacheMaxBytes() const; + //---------------------------------------------------------------------------- + /// @brief Makes sure the raster task runner and the platform task runner + /// are merged + /// + /// @attention If raster and platform task runners are not the same or not + /// merged. This method + /// will try to merge the task runners and might block the current + /// thread and wait until the 2 task runners are merged. + /// + /// @return `true` if raster and platform task runners are the same. + /// `true` if/when raster and platform task runners are merged. + /// `false` if the surface or the RasterThreadMerger has not been + /// initialized. + /// bool EnsureThreadsAreMerged(); private: diff --git a/shell/common/shell.cc b/shell/common/shell.cc index abdf9e9e6d98e..0c6405e4a5a12 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -610,7 +610,6 @@ DartVM* Shell::GetDartVM() { // |PlatformView::Delegate| void Shell::OnPlatformViewCreated(std::unique_ptr surface) { - FML_DLOG(ERROR) << "--- OnPlatformViewCreated"; TRACE_EVENT0("flutter", "Shell::OnPlatformViewCreated"); FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); diff --git a/shell/common/shell_test_external_view_embedder.h b/shell/common/shell_test_external_view_embedder.h index c532aea676c01..8f1ac1447a33b 100644 --- a/shell/common/shell_test_external_view_embedder.h +++ b/shell/common/shell_test_external_view_embedder.h @@ -23,9 +23,12 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { ~ShellTestExternalViewEmbedder() = default; + // Updates the post preroll result so the |PostPrerollAction| after always + // returns the new `post_preroll_result`. void UpdatePostPrerollResult(PostPrerollResult post_preroll_result); - // Resubmit the layer tree to trigger thread merging onces + // Update the post preroll result to PostPrerollResult::kResubmitFrame for + // only the next frame. void SetResubmitOnce(); private: diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 1a669cb62172d..3f98806dbee46 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -620,9 +620,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { auto end_frame_callback = [&](bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { - FML_DLOG(ERROR) << "END FRAME called"; if (should_resubmit_frame && !raster_thread_merger->IsMerged()) { - FML_DLOG(ERROR) << "END FRAME merge threads"; raster_thread_merger->MergeWithLease(ThreadMergingLease); } end_frame_latch.Signal(); From b104fcd351db2f5e2e54d299d3acf929a1fdb4f8 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Mon, 10 Aug 2020 10:19:35 -0700 Subject: [PATCH 12/16] add comments --- fml/raster_thread_merger.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 8f673402de105..d1b332dae6f5a 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -19,6 +19,7 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, lease_term_(kLeaseNotSet) { is_merged_ = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); if (is_merged_) { + // lease_term_ must be > 0 if the threads are merged. lease_term_ = 1; } } From 72049f3eb69ae4feda4c72b9e8524bc61da4e4f6 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 11 Aug 2020 20:11:33 -0700 Subject: [PATCH 13/16] reviews --- fml/raster_thread_merger.cc | 4 +++- shell/common/rasterizer.cc | 1 + shell/common/rasterizer.h | 8 ++++---- shell/common/shell_test_external_view_embedder.h | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index d1b332dae6f5a..882e224606461 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -19,7 +19,9 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, lease_term_(kLeaseNotSet) { is_merged_ = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); if (is_merged_) { - // lease_term_ must be > 0 if the threads are merged. + // lease_term_ indicates how many frames before the |UnMergeNow| happens. + // See: |DecrementLease|. If the threads have been merged already, we should + // set the lease_term_ to at least 1 so |DecrementLease| won't crash. lease_term_ = 1; } } diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 103c0c1b65ba3..2c7d37d7caa3f 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -696,6 +696,7 @@ bool Rasterizer::EnsureThreadsAreMerged() { thread_merger->MergeWithLease(10); }); raster_thread_merger_->WaitUntilMerged(); + FML_DCHECK(raster_thread_merger_->IsMerged()); return true; } diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index 2c69bab3551e6..8906be814beec 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -432,12 +432,12 @@ class Rasterizer final : public SnapshotDelegate { //---------------------------------------------------------------------------- /// @brief Makes sure the raster task runner and the platform task runner - /// are merged + /// are merged. /// /// @attention If raster and platform task runners are not the same or not - /// merged. This method - /// will try to merge the task runners and might block the current - /// thread and wait until the 2 task runners are merged. + /// merged. This method will try to merge the task runners and + /// might block the current thread and wait until the 2 task + /// runners are merged. /// /// @return `true` if raster and platform task runners are the same. /// `true` if/when raster and platform task runners are merged. diff --git a/shell/common/shell_test_external_view_embedder.h b/shell/common/shell_test_external_view_embedder.h index 8f1ac1447a33b..0164aab3a0e50 100644 --- a/shell/common/shell_test_external_view_embedder.h +++ b/shell/common/shell_test_external_view_embedder.h @@ -27,7 +27,7 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { // returns the new `post_preroll_result`. void UpdatePostPrerollResult(PostPrerollResult post_preroll_result); - // Update the post preroll result to PostPrerollResult::kResubmitFrame for + // Updates the post preroll result to `PostPrerollResult::kResubmitFrame` for // only the next frame. void SetResubmitOnce(); From bbb0210be5f0ae9c26c737b2ea8106971cd8a306 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Wed, 12 Aug 2020 13:03:13 -0700 Subject: [PATCH 14/16] review & more unit tests for raster_thread_merger --- fml/raster_thread_merger.cc | 41 +++++----- fml/raster_thread_merger.h | 11 ++- fml/raster_thread_merger_unittests.cc | 106 ++++++++++++++++++++++++++ shell/common/rasterizer.h | 10 +-- 4 files changed, 134 insertions(+), 34 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 882e224606461..d26023c6e6d41 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -17,38 +17,33 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, gpu_queue_id_(gpu_queue_id), task_queues_(fml::MessageLoopTaskQueues::GetInstance()), lease_term_(kLeaseNotSet) { - is_merged_ = task_queues_->Owns(platform_queue_id_, gpu_queue_id_); - if (is_merged_) { - // lease_term_ indicates how many frames before the |UnMergeNow| happens. - // See: |DecrementLease|. If the threads have been merged already, we should - // set the lease_term_ to at least 1 so |DecrementLease| won't crash. - lease_term_ = 1; - } + FML_CHECK(!task_queues_->Owns(platform_queue_id_, gpu_queue_id_)); } void RasterThreadMerger::MergeWithLease(size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; - if (!is_merged_) { - is_merged_ = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); + std::scoped_lock lock(lease_term_mutex_); + if (lease_term_ <= 0) { + bool success = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); + FML_CHECK(success) << "Unable to merge the raster and platform threads."; lease_term_ = lease_term; } merged_condition_.notify_one(); } void RasterThreadMerger::UnMergeNow() { - FML_CHECK(IsOnRasterizingThread()); + std::scoped_lock lock(lease_term_mutex_); lease_term_ = 0; bool success = task_queues_->Unmerge(platform_queue_id_); FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; - is_merged_ = false; } bool RasterThreadMerger::IsOnPlatformThread() const { return MessageLoop::GetCurrentTaskQueueId() == platform_queue_id_; } -bool RasterThreadMerger::IsOnRasterizingThread() const { - if (is_merged_) { +bool RasterThreadMerger::IsOnRasterizingThread() { + if (lease_term_ > 0) { return IsOnPlatformThread(); } else { return !IsOnPlatformThread(); @@ -56,6 +51,7 @@ bool RasterThreadMerger::IsOnRasterizingThread() const { } void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { + std::scoped_lock lock(lease_term_mutex_); FML_DCHECK(lease_term > 0) << "lease_term should be positive."; if (lease_term_ != kLeaseNotSet && static_cast(lease_term) > lease_term_) { @@ -63,23 +59,20 @@ void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { } } -bool RasterThreadMerger::IsMerged() const { - return is_merged_; +bool RasterThreadMerger::IsMerged() { + std::scoped_lock lock(lease_term_mutex_); + return lease_term_ > 0; } void RasterThreadMerger::WaitUntilMerged() { FML_CHECK(IsOnPlatformThread()); - std::unique_lock lock(merged_mutex_); - merged_condition_.wait(lock, [&] { return IsMerged(); }); + std::unique_lock lock(lease_term_mutex_); + merged_condition_.wait(lock, [&] { return lease_term_ > 0; }); } RasterThreadStatus RasterThreadMerger::DecrementLease() { - if (!is_merged_) { - return RasterThreadStatus::kRemainsUnmerged; - } - - // we haven't been set to merge. - if (lease_term_ == kLeaseNotSet) { + std::unique_lock lock(lease_term_mutex_); + if (lease_term_ <= 0) { return RasterThreadStatus::kRemainsUnmerged; } @@ -87,6 +80,8 @@ RasterThreadStatus RasterThreadMerger::DecrementLease() { << "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(); return RasterThreadStatus::kUnmergedNow; } diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index df95cfc2a4533..1518d09f89e57 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -32,7 +32,7 @@ class RasterThreadMerger // unless an |ExtendLeaseTo| gets called. void MergeWithLease(size_t lease_term); - // Unmerge the threads now, resets the lease term to 0. + // Un-merges the threads now, and resets the lease term to 0. // // Must be executed on the raster task runner. void UnMergeNow(); @@ -43,9 +43,9 @@ class RasterThreadMerger // splitting the raster and platform threads. Reduces the lease term by 1. RasterThreadStatus DecrementLease(); - bool IsMerged() const; + bool IsMerged(); - // Wait until the threads are merged. + // Waits until the threads are merged. // // Must run on the platform task runner. void WaitUntilMerged(); @@ -56,7 +56,7 @@ class RasterThreadMerger // Returns true if the current thread owns rasterizing. // When the threads are merged, platform thread owns rasterizing. // When un-merged, raster thread owns rasterizing. - bool IsOnRasterizingThread() const; + bool IsOnRasterizingThread(); // Returns true if the current thread is the platform thread. bool IsOnPlatformThread() const; @@ -67,9 +67,8 @@ class RasterThreadMerger fml::TaskQueueId gpu_queue_id_; fml::RefPtr task_queues_; std::atomic_int lease_term_; - std::atomic_bool is_merged_; std::condition_variable merged_condition_; - std::mutex merged_mutex_; + std::mutex lease_term_mutex_; FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index a182723a79191..fe13c175bdd1b 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -208,3 +208,109 @@ TEST(RasterThreadMerger, LeaseExtension) { thread1.join(); thread2.join(); } + +TEST(RasterThreadMerger, WaitUntilMerged) { + fml::RefPtr raster_thread_merger; + + fml::AutoResetWaitableEvent create_thread_merger_latch; + fml::MessageLoop* loop_platform = nullptr; + fml::AutoResetWaitableEvent latch_platform; + fml::AutoResetWaitableEvent term_platform; + fml::AutoResetWaitableEvent latch_merged; + std::thread thread_platform([&]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop_platform = &fml::MessageLoop::GetCurrent(); + latch_platform.Signal(); + create_thread_merger_latch.Wait(); + raster_thread_merger->WaitUntilMerged(); + latch_merged.Signal(); + term_platform.Wait(); + }); + + const int kNumFramesMerged = 5; + fml::MessageLoop* loop_raster = nullptr; + fml::AutoResetWaitableEvent term_raster; + std::thread thread_raster([&]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop_raster = &fml::MessageLoop::GetCurrent(); + latch_platform.Wait(); + fml::TaskQueueId qid_platform = + loop_platform->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid_raster = + loop_raster->GetTaskRunner()->GetTaskQueueId(); + raster_thread_merger = + fml::MakeRefCounted(qid_platform, qid_raster); + ASSERT_FALSE(raster_thread_merger->IsMerged()); + create_thread_merger_latch.Signal(); + raster_thread_merger->MergeWithLease(kNumFramesMerged); + term_raster.Wait(); + }); + + latch_merged.Wait(); + ASSERT_TRUE(raster_thread_merger->IsMerged()); + + for (int i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger->IsMerged()); + raster_thread_merger->DecrementLease(); + } + + ASSERT_FALSE(raster_thread_merger->IsMerged()); + + term_platform.Signal(); + term_raster.Signal(); + thread_platform.join(); + thread_raster.join(); +} + +TEST(RasterThreadMerger, WaitUntilMergedReturnsIfAlreadyMerged) { + fml::RefPtr raster_thread_merger; + + fml::AutoResetWaitableEvent thread_merged_latch; + fml::MessageLoop* loop_platform = nullptr; + fml::AutoResetWaitableEvent latch_platform; + fml::AutoResetWaitableEvent term_platform; + fml::AutoResetWaitableEvent latch_wait_until_merged; + std::thread thread_platform([&]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop_platform = &fml::MessageLoop::GetCurrent(); + latch_platform.Signal(); + thread_merged_latch.Wait(); + raster_thread_merger->WaitUntilMerged(); + latch_wait_until_merged.Signal(); + term_platform.Wait(); + }); + + const int kNumFramesMerged = 5; + fml::MessageLoop* loop_raster = nullptr; + fml::AutoResetWaitableEvent term_raster; + std::thread thread_raster([&]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop_raster = &fml::MessageLoop::GetCurrent(); + latch_platform.Wait(); + fml::TaskQueueId qid_platform = + loop_platform->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid_raster = + loop_raster->GetTaskRunner()->GetTaskQueueId(); + raster_thread_merger = + fml::MakeRefCounted(qid_platform, qid_raster); + ASSERT_FALSE(raster_thread_merger->IsMerged()); + raster_thread_merger->MergeWithLease(kNumFramesMerged); + thread_merged_latch.Signal(); + term_raster.Wait(); + }); + + latch_wait_until_merged.Wait(); + ASSERT_TRUE(raster_thread_merger->IsMerged()); + + for (int i = 0; i < kNumFramesMerged; i++) { + ASSERT_TRUE(raster_thread_merger->IsMerged()); + raster_thread_merger->DecrementLease(); + } + + ASSERT_FALSE(raster_thread_merger->IsMerged()); + + term_platform.Signal(); + term_raster.Signal(); + thread_platform.join(); + thread_raster.join(); +} diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index 8906be814beec..5e57f1bb99b80 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -435,14 +435,14 @@ class Rasterizer final : public SnapshotDelegate { /// are merged. /// /// @attention If raster and platform task runners are not the same or not - /// merged. This method will try to merge the task runners and - /// might block the current thread and wait until the 2 task - /// runners are merged. + /// merged, this method will try to merge the task runners, + /// blocking the current thread until the 2 task runners are + /// merged. /// /// @return `true` if raster and platform task runners are the same. /// `true` if/when raster and platform task runners are merged. - /// `false` if the surface or the RasterThreadMerger has not been - /// initialized. + /// `false` if the surface or the |RasterThreadMerger| has not + /// been initialized. /// bool EnsureThreadsAreMerged(); From 5d60a2e851c6cfbecaf3c96efe6b87872acaa505 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 13 Aug 2020 10:47:52 -0700 Subject: [PATCH 15/16] refactor --- fml/raster_thread_merger.cc | 14 +++++++++----- fml/raster_thread_merger.h | 2 ++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index d26023c6e6d41..667c326ad5565 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -23,7 +23,7 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, void RasterThreadMerger::MergeWithLease(size_t lease_term) { FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(lease_term_mutex_); - if (lease_term_ <= 0) { + if (!IsMergedUnSafe()) { bool success = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); FML_CHECK(success) << "Unable to merge the raster and platform threads."; lease_term_ = lease_term; @@ -43,7 +43,7 @@ bool RasterThreadMerger::IsOnPlatformThread() const { } bool RasterThreadMerger::IsOnRasterizingThread() { - if (lease_term_ > 0) { + if (IsMergedUnSafe()) { return IsOnPlatformThread(); } else { return !IsOnPlatformThread(); @@ -52,7 +52,7 @@ bool RasterThreadMerger::IsOnRasterizingThread() { void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { std::scoped_lock lock(lease_term_mutex_); - FML_DCHECK(lease_term > 0) << "lease_term should be positive."; + FML_DCHECK(IsMergedUnSafe()) << "lease_term should be positive."; if (lease_term_ != kLeaseNotSet && static_cast(lease_term) > lease_term_) { lease_term_ = lease_term; @@ -61,18 +61,22 @@ void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { bool RasterThreadMerger::IsMerged() { std::scoped_lock lock(lease_term_mutex_); + return IsMergedUnSafe(); +}\ + +bool RasterThreadMerger::IsMergedUnSafe() { return lease_term_ > 0; } void RasterThreadMerger::WaitUntilMerged() { FML_CHECK(IsOnPlatformThread()); std::unique_lock lock(lease_term_mutex_); - merged_condition_.wait(lock, [&] { return lease_term_ > 0; }); + merged_condition_.wait(lock, [&] { return IsMergedUnSafe(); }); } RasterThreadStatus RasterThreadMerger::DecrementLease() { std::unique_lock lock(lease_term_mutex_); - if (lease_term_ <= 0) { + if (!IsMergedUnSafe()) { return RasterThreadStatus::kRemainsUnmerged; } diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 1518d09f89e57..71c4ee03befcb 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -70,6 +70,8 @@ class RasterThreadMerger std::condition_variable merged_condition_; std::mutex lease_term_mutex_; + bool IsMergedUnSafe(); + FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); FML_DISALLOW_COPY_AND_ASSIGN(RasterThreadMerger); From f0aa0a6b032d43cd71a390eebeee691b9cf6b0db Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 18 Aug 2020 10:49:26 -0700 Subject: [PATCH 16/16] make raster_thraed_merger handle static thread merging --- fml/message_loop_task_queues.cc | 2 +- fml/message_loop_task_queues_unittests.cc | 7 +++ fml/raster_thread_merger.cc | 23 +++++++- fml/raster_thread_merger.h | 15 +++++ fml/raster_thread_merger_unittests.cc | 71 +++++++++-------------- shell/common/rasterizer.cc | 11 +--- 6 files changed, 75 insertions(+), 54 deletions(-) diff --git a/fml/message_loop_task_queues.cc b/fml/message_loop_task_queues.cc index 8df08fa1b244d..1d5a9091083f3 100644 --- a/fml/message_loop_task_queues.cc +++ b/fml/message_loop_task_queues.cc @@ -246,7 +246,7 @@ bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner) { bool MessageLoopTaskQueues::Owns(TaskQueueId owner, TaskQueueId subsumed) const { std::lock_guard guard(queue_mutex_); - return subsumed == queue_entries_.at(owner)->owner_of || owner == subsumed; + return subsumed == queue_entries_.at(owner)->owner_of; } // Subsumed queues will never have pending tasks. diff --git a/fml/message_loop_task_queues_unittests.cc b/fml/message_loop_task_queues_unittests.cc index a1c2df0a47f6c..1086bb28b8013 100644 --- a/fml/message_loop_task_queues_unittests.cc +++ b/fml/message_loop_task_queues_unittests.cc @@ -173,6 +173,13 @@ TEST(MessageLoopTaskQueue, NotifyObserversWhileCreatingQueues) { before_second_observer.Signal(); notify_observers.join(); } + +TEST(MessageLoopTaskQueue, QueueDoNotOwnItself) { + auto task_queue = fml::MessageLoopTaskQueues::GetInstance(); + auto queue_id = task_queue->CreateTaskQueue(); + ASSERT_FALSE(task_queue->Owns(queue_id, queue_id)); +} + // TODO(chunhtai): This unit-test is flaky and sometimes fails asynchronizely // after the test has finished. // https://github.com/flutter/flutter/issues/43858 diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 5b48d1e435dbb..62b696db70aa4 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -21,6 +21,10 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, } void RasterThreadMerger::MergeWithLease(size_t lease_term) { + if (TaskQueuesAreSame()) { + return; + } + FML_DCHECK(lease_term > 0) << "lease_term should be positive."; std::scoped_lock lock(lease_term_mutex_); if (!IsMergedUnSafe()) { @@ -32,6 +36,10 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { } void RasterThreadMerger::UnMergeNow() { + if (TaskQueuesAreSame()) { + return; + } + std::scoped_lock lock(lease_term_mutex_); lease_term_ = 0; bool success = task_queues_->Unmerge(platform_queue_id_); @@ -51,6 +59,9 @@ bool RasterThreadMerger::IsOnRasterizingThread() { } void RasterThreadMerger::ExtendLeaseTo(size_t lease_term) { + if (TaskQueuesAreSame()) { + return; + } std::scoped_lock lock(lease_term_mutex_); FML_DCHECK(IsMergedUnSafe()) << "lease_term should be positive."; if (lease_term_ != kLeaseNotSet && @@ -65,16 +76,26 @@ bool RasterThreadMerger::IsMerged() { } bool RasterThreadMerger::IsMergedUnSafe() { - return lease_term_ > 0; + return lease_term_ > 0 || TaskQueuesAreSame(); +} + +bool RasterThreadMerger::TaskQueuesAreSame() { + return platform_queue_id_ == gpu_queue_id_; } void RasterThreadMerger::WaitUntilMerged() { + if (TaskQueuesAreSame()) { + return; + } FML_CHECK(IsOnPlatformThread()); std::unique_lock lock(lease_term_mutex_); merged_condition_.wait(lock, [&] { return IsMergedUnSafe(); }); } RasterThreadStatus RasterThreadMerger::DecrementLease() { + if (TaskQueuesAreSame()) { + return RasterThreadStatus::kRemainsMerged; + } std::unique_lock lock(lease_term_mutex_); if (!IsMergedUnSafe()) { return RasterThreadStatus::kRemainsUnmerged; diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 71c4ee03befcb..b01cf76a11436 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -30,17 +30,29 @@ class RasterThreadMerger // When the caller merges with a lease term of say 2. The threads // are going to remain merged until 2 invocations of |DecreaseLease|, // unless an |ExtendLeaseTo| gets called. + // + // 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); // Un-merges the threads now, and resets the lease term to 0. // // Must be executed on the raster task runner. + // + // 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(); + // 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); // Returns |RasterThreadStatus::kUnmergedNow| if this call resulted in // splitting the raster and platform threads. Reduces the lease term by 1. + // + // If the task queues are the same, we consider them statically merged. + // When task queues are statically merged this method becomes no-op. RasterThreadStatus DecrementLease(); bool IsMerged(); @@ -71,6 +83,9 @@ class RasterThreadMerger std::mutex lease_term_mutex_; bool IsMergedUnSafe(); + // 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(); FML_FRIEND_REF_COUNTED_THREAD_SAFE(RasterThreadMerger); FML_FRIEND_MAKE_REF_COUNTED(RasterThreadMerger); diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index fe13c175bdd1b..f3df1c1bb2259 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -262,55 +262,42 @@ TEST(RasterThreadMerger, WaitUntilMerged) { thread_raster.join(); } -TEST(RasterThreadMerger, WaitUntilMergedReturnsIfAlreadyMerged) { - fml::RefPtr raster_thread_merger; - - fml::AutoResetWaitableEvent thread_merged_latch; - fml::MessageLoop* loop_platform = nullptr; - fml::AutoResetWaitableEvent latch_platform; - fml::AutoResetWaitableEvent term_platform; - fml::AutoResetWaitableEvent latch_wait_until_merged; - std::thread thread_platform([&]() { +TEST(RasterThreadMerger, HandleTaskQueuesAreTheSame) { + fml::MessageLoop* loop1 = nullptr; + fml::AutoResetWaitableEvent latch1; + fml::AutoResetWaitableEvent term1; + std::thread thread1([&loop1, &latch1, &term1]() { fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop_platform = &fml::MessageLoop::GetCurrent(); - latch_platform.Signal(); - thread_merged_latch.Wait(); - raster_thread_merger->WaitUntilMerged(); - latch_wait_until_merged.Signal(); - term_platform.Wait(); + loop1 = &fml::MessageLoop::GetCurrent(); + latch1.Signal(); + term1.Wait(); }); - const int kNumFramesMerged = 5; - fml::MessageLoop* loop_raster = nullptr; - fml::AutoResetWaitableEvent term_raster; - std::thread thread_raster([&]() { - fml::MessageLoop::EnsureInitializedForCurrentThread(); - loop_raster = &fml::MessageLoop::GetCurrent(); - latch_platform.Wait(); - fml::TaskQueueId qid_platform = - loop_platform->GetTaskRunner()->GetTaskQueueId(); - fml::TaskQueueId qid_raster = - loop_raster->GetTaskRunner()->GetTaskQueueId(); - raster_thread_merger = - fml::MakeRefCounted(qid_platform, qid_raster); - ASSERT_FALSE(raster_thread_merger->IsMerged()); - raster_thread_merger->MergeWithLease(kNumFramesMerged); - thread_merged_latch.Signal(); - term_raster.Wait(); - }); + latch1.Wait(); - latch_wait_until_merged.Wait(); - ASSERT_TRUE(raster_thread_merger->IsMerged()); + fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid2 = qid1; + const auto raster_thread_merger_ = + fml::MakeRefCounted(qid1, qid2); + // Statically merged. + 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); for (int i = 0; i < kNumFramesMerged; i++) { - ASSERT_TRUE(raster_thread_merger->IsMerged()); - raster_thread_merger->DecrementLease(); + ASSERT_TRUE(raster_thread_merger_->IsMerged()); + raster_thread_merger_->DecrementLease(); } - ASSERT_FALSE(raster_thread_merger->IsMerged()); + ASSERT_TRUE(raster_thread_merger_->IsMerged()); - term_platform.Signal(); - term_raster.Signal(); - thread_platform.join(); - thread_raster.join(); + // Wait until merged should also return immediately. + raster_thread_merger_->WaitUntilMerged(); + ASSERT_TRUE(raster_thread_merger_->IsMerged()); + + term1.Signal(); + thread1.join(); } diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 7f2b82b384a6d..292de2e234f18 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -62,11 +62,7 @@ void Rasterizer::Setup(std::unique_ptr surface) { // TODO(sanjayc77): https://github.com/flutter/flutter/issues/53179. Add // support for raster thread merger for Fuchsia. if (surface_->GetExternalViewEmbedder() && - surface_->GetExternalViewEmbedder()->SupportsDynamicThreadMerging() && - // Don't create raster_thread_merger if platform and raster task runners - // are the same. - delegate_.GetTaskRunners().GetRasterTaskRunner() != - delegate_.GetTaskRunners().GetPlatformTaskRunner()) { + surface_->GetExternalViewEmbedder()->SupportsDynamicThreadMerging()) { const auto platform_id = delegate_.GetTaskRunners().GetPlatformTaskRunner()->GetTaskQueueId(); const auto gpu_id = @@ -668,11 +664,6 @@ std::optional Rasterizer::GetResourceCacheMaxBytes() const { } bool Rasterizer::EnsureThreadsAreMerged() { - // If threads are merged statically, always return true. - if (delegate_.GetTaskRunners().GetRasterTaskRunner() == - delegate_.GetTaskRunners().GetPlatformTaskRunner()) { - return true; - } if (surface_ == nullptr || raster_thread_merger_.get() == nullptr) { return false; }