From 629072277dc73e0628afab7ba7142f7d582996ad Mon Sep 17 00:00:00 2001 From: Amir Hardon Date: Mon, 18 Mar 2019 14:13:30 -0700 Subject: [PATCH] Merge only gpu and platform threads for platform views, fix deadlock. (#8045) The reason we didn't merge just the gpu and platform threads from the get go was a deadlock in Shell:OnPlatformViewCreated and Shell:OnPlatformViewDestroyed. The deadlock was caused by the platform thread starting a thread-hopping flow that ends ends up with the gpu thread releasing a latch that the platform thread is waiting on just after starting the cross-thread dance. If the platform and gpu threads are the same, that last task that is posted to the gpu thread will never get executed as the gpu/platform thread is blocked on a latch. This works around the deadlock by having a special case in the code for the scenario where the gpu and platform threads are the same. Fixes: flutter/flutter#23974 --- shell/common/shell.cc | 60 +++++++++++++++++-- shell/common/shell_unittests.cc | 2 +- .../ios/framework/Source/FlutterEngine.mm | 10 ++-- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 3c43ef2d1..6ba709e87 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -437,16 +437,35 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { latch.Signal(); }); + // The normal flow exectued by this method is that the platform thread is + // starting the sequence and waiting on the latch. Later the UI thread posts + // gpu_task to the GPU thread which signals the latch. If the GPU the and + // platform threads are the same this results in a deadlock as the gpu_task + // will never be posted to the plaform/gpu thread that is blocked on a latch. + // To avoid the described deadlock, if the gpu and the platform threads are + // the same, should_post_gpu_task will be false, and then instead of posting a + // task to the gpu thread, the ui thread just signals the latch and the + // platform/gpu thread follows with executing gpu_task. + bool should_post_gpu_task = + task_runners_.GetGPUTaskRunner() != task_runners_.GetPlatformTaskRunner(); + auto ui_task = [engine = engine_->GetWeakPtr(), // gpu_task_runner = task_runners_.GetGPUTaskRunner(), // - gpu_task // + gpu_task, should_post_gpu_task, + &latch // ] { if (engine) { engine->OnOutputSurfaceCreated(); } // Step 2: Next, tell the GPU thread that it should create a surface for its // rasterizer. - fml::TaskRunner::RunNowOrPostTask(gpu_task_runner, gpu_task); + if (should_post_gpu_task) { + fml::TaskRunner::RunNowOrPostTask(gpu_task_runner, gpu_task); + } else { + // See comment on should_post_gpu_task, in this case we just unblock + // the platform thread. + latch.Signal(); + } }; // Threading: Capture platform view by raw pointer and not the weak pointer. @@ -471,6 +490,12 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { fml::TaskRunner::RunNowOrPostTask(task_runners_.GetIOTaskRunner(), io_task); latch.Wait(); + if (!should_post_gpu_task) { + // See comment on should_post_gpu_task, in this case the gpu_task + // wasn't executed, and we just run it here as the platform thread + // is the GPU thread. + gpu_task(); + } } // |shell::PlatformView::Delegate| @@ -504,21 +529,46 @@ void Shell::OnPlatformViewDestroyed() { fml::TaskRunner::RunNowOrPostTask(io_task_runner, io_task); }; + // The normal flow exectued by this method is that the platform thread is + // starting the sequence and waiting on the latch. Later the UI thread posts + // gpu_task to the GPU thread triggers signaling the latch(on the IO thread). + // If the GPU the and platform threads are the same this results in a deadlock + // as the gpu_task will never be posted to the plaform/gpu thread that is + // blocked on a latch. To avoid the described deadlock, if the gpu and the + // platform threads are the same, should_post_gpu_task will be false, and then + // instead of posting a task to the gpu thread, the ui thread just signals the + // latch and the platform/gpu thread follows with executing gpu_task. + bool should_post_gpu_task = + task_runners_.GetGPUTaskRunner() != task_runners_.GetPlatformTaskRunner(); + auto ui_task = [engine = engine_->GetWeakPtr(), - gpu_task_runner = task_runners_.GetGPUTaskRunner(), - gpu_task]() { + gpu_task_runner = task_runners_.GetGPUTaskRunner(), gpu_task, + should_post_gpu_task, &latch]() { if (engine) { engine->OnOutputSurfaceDestroyed(); } // Step 1: Next, tell the GPU thread that its rasterizer should suspend // access to the underlying surface. - fml::TaskRunner::RunNowOrPostTask(gpu_task_runner, gpu_task); + if (should_post_gpu_task) { + fml::TaskRunner::RunNowOrPostTask(gpu_task_runner, gpu_task); + } else { + // See comment on should_post_gpu_task, in this case we just unblock + // the platform thread. + 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_gpu_task) { + // See comment on should_post_gpu_task, in this case the gpu_task + // wasn't executed, and we just run it here as the platform thread + // is the GPU thread. + gpu_task(); + latch.Wait(); + } } // |shell::PlatformView::Delegate| diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 9e09bbc63..85c6b7962 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -193,7 +193,7 @@ TEST(ShellTest, InitializeWithMultipleThreadButCallingThreadAsPlatformThread) { // Reported in Bug: Engine deadlocks when gpu and platforms threads are the same // #21398 (https://github.com/flutter/flutter/issues/21398) -TEST(ShellTest, DISABLED_InitializeWithGPUAndPlatformThreadsTheSame) { +TEST(ShellTest, InitializeWithGPUAndPlatformThreadsTheSame) { blink::Settings settings = {}; settings.task_observer_add = [](intptr_t, fml::closure) {}; settings.task_observer_remove = [](intptr_t) {}; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index a90e49c70..00fead7de 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -355,16 +355,16 @@ // 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 run in a single threaded configuration. - // TODO(amirh/chinmaygarde): merge only the gpu and platform threads. - // https://github.com/flutter/flutter/issues/23974 + // 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 + blink::TaskRunners task_runners(threadLabel.UTF8String, // label fml::MessageLoop::GetCurrent().GetTaskRunner(), // platform fml::MessageLoop::GetCurrent().GetTaskRunner(), // gpu - fml::MessageLoop::GetCurrent().GetTaskRunner(), // ui - fml::MessageLoop::GetCurrent().GetTaskRunner() // io + _threadHost.ui_thread->GetTaskRunner(), // ui + _threadHost.io_thread->GetTaskRunner() // io ); // Create the shell. This is a blocking operation. _shell = shell::Shell::Create(std::move(task_runners), // task runners -- GitLab