From d3f34c823eeee352907874b8d50277f9c37e42c0 Mon Sep 17 00:00:00 2001 From: kinarobin Date: Wed, 14 Apr 2021 20:13:09 +0800 Subject: [PATCH 1/5] remove judgment for gpu task --- shell/common/shell.cc | 73 +++---------------------------------------- 1 file changed, 4 insertions(+), 69 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index e919c1b86a2f2..28c91a5a3bd42 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -716,28 +716,8 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { // Prevent any request to change the thread configuration for raster and // platform queues while the platform view is being created. - // - // This prevents false positives such as this method starts assuming that the - // raster and platform queues have a given thread configuration, but then the - // configuration is changed by a task, and the asumption is not longer true. - // - // This incorrect assumption can lead to dead lock. - // See `should_post_raster_task` for more. rasterizer_->DisableThreadMergerIfNeeded(); - // 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. - const bool should_post_raster_task = - !task_runners_.GetRasterTaskRunner()->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 @@ -764,7 +744,7 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { auto ui_task = [engine = engine_->GetWeakPtr(), // raster_task_runner = task_runners_.GetRasterTaskRunner(), // - raster_task, should_post_raster_task, + raster_task, &latch // ] { if (engine) { @@ -772,13 +752,7 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { } // 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(); - } + fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); }; // Threading: Capture platform view by raw pointer and not the weak pointer. @@ -809,12 +783,6 @@ 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(); - } } // |PlatformView::Delegate| @@ -825,28 +793,8 @@ void Shell::OnPlatformViewDestroyed() { // Prevent any request to change the thread configuration for raster and // platform queues while the platform view is being destroyed. - // - // This prevents false positives such as this method starts assuming that the - // raster and platform queues have a given thread configuration, but then the - // configuration is changed by a task, and the asumption is not longer true. - // - // This incorrect assumption can lead to dead lock. - // See `should_post_raster_task` for more. rasterizer_->DisableThreadMergerIfNeeded(); - // 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. - const bool should_post_raster_task = - !task_runners_.GetRasterTaskRunner()->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 @@ -881,17 +829,11 @@ void Shell::OnPlatformViewDestroyed() { auto ui_task = [engine = engine_->GetWeakPtr(), raster_task_runner = task_runners_.GetRasterTaskRunner(), - raster_task, should_post_raster_task, &latch]() { + raster_task, &latch]() { if (engine) { engine->OnOutputSurfaceDestroyed(); } - 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(); - } + fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); }; // Step 0: Post a task onto the UI thread to tell the engine that its output @@ -899,13 +841,6 @@ void Shell::OnPlatformViewDestroyed() { 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. - raster_task(); - latch.Wait(); - } } // |PlatformView::Delegate| From 30cc9dc5a39a42853104598e4b0280e670f7f463 Mon Sep 17 00:00:00 2001 From: kinarobin Date: Wed, 14 Apr 2021 20:39:22 +0800 Subject: [PATCH 2/5] delete unused parameters --- shell/common/shell.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 28c91a5a3bd42..8f5c568616dc1 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -744,9 +744,7 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { auto ui_task = [engine = engine_->GetWeakPtr(), // raster_task_runner = task_runners_.GetRasterTaskRunner(), // - raster_task, - &latch // - ] { + raster_task]() { if (engine) { engine->OnOutputSurfaceCreated(); } @@ -829,7 +827,7 @@ void Shell::OnPlatformViewDestroyed() { auto ui_task = [engine = engine_->GetWeakPtr(), raster_task_runner = task_runners_.GetRasterTaskRunner(), - raster_task, &latch]() { + raster_task]() { if (engine) { engine->OnOutputSurfaceDestroyed(); } From bd95d3260935d286c1b002e2648e6ea31036c883 Mon Sep 17 00:00:00 2001 From: kinarobin Date: Thu, 15 Apr 2021 10:45:22 +0800 Subject: [PATCH 3/5] remove meaningless test case --- .../embedder/tests/embedder_unittests_gl.cc | 79 ------------------- 1 file changed, 79 deletions(-) diff --git a/shell/platform/embedder/tests/embedder_unittests_gl.cc b/shell/platform/embedder/tests/embedder_unittests_gl.cc index b6744999753eb..edd93c9046996 100644 --- a/shell/platform/embedder/tests/embedder_unittests_gl.cc +++ b/shell/platform/embedder/tests/embedder_unittests_gl.cc @@ -1719,85 +1719,6 @@ TEST_F(EmbedderTest, CanCreateEmbedderWithCustomRenderTaskRunner) { } } -//------------------------------------------------------------------------------ -/// Asserts that the render task runner can be the same as the platform task -/// runner. -/// -TEST_F(EmbedderTest, - CanCreateEmbedderWithCustomRenderTaskRunnerTheSameAsPlatformTaskRunner) { - // A new thread needs to be created for the platform thread because the test - // can't wait for assertions to be completed on the same thread that services - // platform task runner tasks. - auto platform_task_runner = CreateNewThread("platform_thread"); - - static std::mutex engine_mutex; - static UniqueEngine engine; - fml::AutoResetWaitableEvent task_latch; - bool task_executed = false; - EmbedderTestTaskRunner common_task_runner( - platform_task_runner, [&](FlutterTask task) { - std::scoped_lock engine_lock(engine_mutex); - if (engine.is_valid()) { - ASSERT_EQ(FlutterEngineRunTask(engine.get(), &task), kSuccess); - task_executed = true; - task_latch.Signal(); - } - }); - - platform_task_runner->PostTask([&]() { - EmbedderConfigBuilder builder( - GetEmbedderContext(EmbedderTestContextType::kOpenGLContext)); - builder.SetDartEntrypoint("can_render_scene_without_custom_compositor"); - builder.SetOpenGLRendererConfig(SkISize::Make(800, 600)); - builder.SetRenderTaskRunner( - &common_task_runner.GetFlutterTaskRunnerDescription()); - builder.SetPlatformTaskRunner( - &common_task_runner.GetFlutterTaskRunnerDescription()); - - { - std::scoped_lock lock(engine_mutex); - engine = builder.InitializeEngine(); - } - - ASSERT_EQ(FlutterEngineRunInitialized(engine.get()), kSuccess); - - ASSERT_TRUE(engine.is_valid()); - - FlutterWindowMetricsEvent event = {}; - event.struct_size = sizeof(event); - event.width = 800; - event.height = 600; - event.pixel_ratio = 1.0; - ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), - kSuccess); - }); - - task_latch.Wait(); - - // Don't use the task latch because that may be called multiple time - // (including during the shutdown process). - fml::AutoResetWaitableEvent shutdown_latch; - - platform_task_runner->PostTask([&]() { - ASSERT_TRUE(task_executed); - ASSERT_EQ(FlutterEngineDeinitialize(engine.get()), kSuccess); - - { - std::scoped_lock engine_lock(engine_mutex); - engine.reset(); - } - shutdown_latch.Signal(); - }); - - shutdown_latch.Wait(); - - { - std::scoped_lock engine_lock(engine_mutex); - // Engine should have been killed by this point. - ASSERT_FALSE(engine.is_valid()); - } -} - TEST_F(EmbedderTest, CompositorMustBeAbleToRenderKnownScenePixelRatioOnSurface) { auto& context = GetEmbedderContext(EmbedderTestContextType::kOpenGLContext); From 9f9a250908aa416652ad2cc1a6051a61439770a7 Mon Sep 17 00:00:00 2001 From: kinarobin Date: Thu, 15 Apr 2021 14:13:31 +0800 Subject: [PATCH 4/5] remove meaningless test case --- shell/common/shell_unittests.cc | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 0daddce767641..47ef8b04a1ad3 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -345,26 +345,6 @@ TEST_F(ShellTest, InitializeWithDisabledGpu) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, InitializeWithGPUAndPlatformThreadsTheSame) { - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); - Settings settings = CreateSettingsForFixture(); - 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)); - ASSERT_TRUE(DartVMRef::IsInstanceRunning()); - ASSERT_TRUE(ValidateShell(shell.get())); - DestroyShell(std::move(shell), std::move(task_runners)); - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); -} - TEST_F(ShellTest, FixturesAreFunctional) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); auto settings = CreateSettingsForFixture(); From 498f4f9bb2ac766d679a5d345569772de435e96e Mon Sep 17 00:00:00 2001 From: kinarobin Date: Thu, 15 Apr 2021 14:39:06 +0800 Subject: [PATCH 5/5] update test case --- shell/common/rasterizer_unittests.cc | 2 +- shell/common/shell_unittests.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 96321df4eb1a3..47433ff6edc6b 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -219,7 +219,7 @@ TEST( fml::MessageLoop::EnsureInitializedForCurrentThread(); TaskRunners task_runners("test", fml::MessageLoop::GetCurrent().GetTaskRunner(), - fml::MessageLoop::GetCurrent().GetTaskRunner(), + thread_host.raster_thread->GetTaskRunner(), thread_host.ui_thread->GetTaskRunner(), thread_host.io_thread->GetTaskRunner()); diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 47ef8b04a1ad3..6bb221e779c0b 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -1077,7 +1077,7 @@ TEST_F(ShellTest, TaskRunners task_runners( "test", thread_host.platform_thread->GetTaskRunner(), // platform - thread_host.platform_thread->GetTaskRunner(), // raster + thread_host.raster_thread->GetTaskRunner(), // raster thread_host.ui_thread->GetTaskRunner(), // ui thread_host.io_thread->GetTaskRunner() // io );