From 2d111a4d1d6fea13ca051cc99edc324bb41bb934 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 23 Jun 2023 16:00:26 -0700 Subject: [PATCH 1/8] [Impeller] Give Impeller a dedicated raster priority level worker loop. --- ci/licenses_golden/excluded_files | 1 - .../backend/metal/playground_impl_mtl.mm | 7 ++- .../backend/vulkan/playground_impl_vk.cc | 5 +- .../backend/vulkan/playground_impl_vk.h | 2 - impeller/renderer/backend/metal/context_mtl.h | 8 +--- .../renderer/backend/metal/context_mtl.mm | 46 +++++++++++-------- .../renderer/backend/vulkan/context_vk.cc | 27 ++++++----- impeller/renderer/backend/vulkan/context_vk.h | 3 +- .../backend/vulkan/test/mock_vulkan.cc | 2 - .../common/shell_test_platform_view_metal.mm | 8 ++-- shell/platform/android/BUILD.gn | 1 - .../android_context_vulkan_impeller.cc | 8 +--- .../android/android_context_vulkan_impeller.h | 4 +- ...droid_context_vulkan_impeller_unittests.cc | 31 ------------- .../platform/android/android_shell_holder.cc | 7 ++- .../platform/android/platform_view_android.cc | 7 +-- .../platform/android/platform_view_android.h | 12 ++--- .../FlutterDarwinContextMetalImpeller.h | 4 +- .../FlutterDarwinContextMetalImpeller.mm | 13 ++---- shell/platform/darwin/ios/ios_context.h | 3 -- shell/platform/darwin/ios/ios_context.mm | 4 +- .../darwin/ios/ios_context_metal_impeller.h | 3 +- .../darwin/ios/ios_context_metal_impeller.mm | 4 +- .../platform/darwin/ios/platform_view_ios.mm | 1 - .../embedder_surface_metal_impeller.h | 1 - .../embedder_surface_metal_impeller.mm | 4 +- 26 files changed, 76 insertions(+), 140 deletions(-) delete mode 100644 shell/platform/android/android_context_vulkan_impeller_unittests.cc diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 3be28594a8c7d..8c0100eeeaeb7 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -230,7 +230,6 @@ ../../../flutter/shell/platform/android/.gitignore ../../../flutter/shell/platform/android/android_context_gl_impeller_unittests.cc ../../../flutter/shell/platform/android/android_context_gl_unittests.cc -../../../flutter/shell/platform/android/android_context_vulkan_impeller_unittests.cc ../../../flutter/shell/platform/android/android_shell_holder_unittests.cc ../../../flutter/shell/platform/android/apk_asset_provider_unittests.cc ../../../flutter/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc diff --git a/impeller/playground/backend/metal/playground_impl_mtl.mm b/impeller/playground/backend/metal/playground_impl_mtl.mm index 55e17871f70a5..e9ffbf64f67b3 100644 --- a/impeller/playground/backend/metal/playground_impl_mtl.mm +++ b/impeller/playground/backend/metal/playground_impl_mtl.mm @@ -73,10 +73,9 @@ if (!window) { return; } - auto worker_task_runner = concurrent_loop_->GetTaskRunner(); - auto context = ContextMTL::Create( - ShaderLibraryMappingsForPlayground(), worker_task_runner, - is_gpu_disabled_sync_switch_, "Playground Library"); + auto context = + ContextMTL::Create(ShaderLibraryMappingsForPlayground(), + is_gpu_disabled_sync_switch_, "Playground Library"); if (!context) { return; } diff --git a/impeller/playground/backend/vulkan/playground_impl_vk.cc b/impeller/playground/backend/vulkan/playground_impl_vk.cc index f92192c4a9ba4..7783a8c31ece5 100644 --- a/impeller/playground/backend/vulkan/playground_impl_vk.cc +++ b/impeller/playground/backend/vulkan/playground_impl_vk.cc @@ -52,9 +52,7 @@ void PlaygroundImplVK::DestroyWindowHandle(WindowHandle handle) { } PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches) - : PlaygroundImpl(switches), - concurrent_loop_(fml::ConcurrentMessageLoop::Create()), - handle_(nullptr, &DestroyWindowHandle) { + : PlaygroundImpl(switches), handle_(nullptr, &DestroyWindowHandle) { if (!::glfwVulkanSupported()) { #ifdef TARGET_OS_MAC VALIDATION_LOG << "Attempted to initialize a Vulkan playground on macOS " @@ -86,7 +84,6 @@ PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches) &::glfwGetInstanceProcAddress); context_settings.shader_libraries_data = ShaderLibraryMappingsForPlayground(); context_settings.cache_directory = fml::paths::GetCachesDirectory(); - context_settings.worker_task_runner = concurrent_loop_->GetTaskRunner(); context_settings.enable_validation = switches_.enable_vulkan_validation; auto context = ContextVK::Create(std::move(context_settings)); diff --git a/impeller/playground/backend/vulkan/playground_impl_vk.h b/impeller/playground/backend/vulkan/playground_impl_vk.h index bb0a7842a18c7..0dae50b0c0677 100644 --- a/impeller/playground/backend/vulkan/playground_impl_vk.h +++ b/impeller/playground/backend/vulkan/playground_impl_vk.h @@ -4,7 +4,6 @@ #pragma once -#include "flutter/fml/concurrent_message_loop.h" #include "flutter/fml/macros.h" #include "impeller/playground/playground_impl.h" #include "impeller/renderer/backend/vulkan/vk.h" @@ -18,7 +17,6 @@ class PlaygroundImplVK final : public PlaygroundImpl { ~PlaygroundImplVK(); private: - std::shared_ptr concurrent_loop_; std::shared_ptr context_; // Windows management. diff --git a/impeller/renderer/backend/metal/context_mtl.h b/impeller/renderer/backend/metal/context_mtl.h index 820c0e216e2de..2659b188e6f9f 100644 --- a/impeller/renderer/backend/metal/context_mtl.h +++ b/impeller/renderer/backend/metal/context_mtl.h @@ -35,12 +35,10 @@ class ContextMTL final : public Context, public: static std::shared_ptr Create( const std::vector& shader_library_paths, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch); static std::shared_ptr Create( const std::vector>& shader_libraries_data, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch, const std::string& label); @@ -48,7 +46,6 @@ class ContextMTL final : public Context, id device, id command_queue, const std::vector>& shader_libraries_data, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch, const std::string& label); @@ -86,7 +83,7 @@ class ContextMTL final : public Context, id CreateMTLCommandBuffer() const; - const std::shared_ptr& GetWorkerTaskRunner() const; + const std::shared_ptr GetWorkerTaskRunner() const; std::shared_ptr GetIsGpuDisabledSyncSwitch() const; @@ -98,7 +95,7 @@ class ContextMTL final : public Context, std::shared_ptr sampler_library_; std::shared_ptr resource_allocator_; std::shared_ptr device_capabilities_; - std::shared_ptr worker_task_runner_; + std::shared_ptr raster_message_loop_; std::shared_ptr is_gpu_disabled_sync_switch_; bool is_valid_ = false; @@ -106,7 +103,6 @@ class ContextMTL final : public Context, id device, id command_queue, NSArray>* shader_libraries, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch); std::shared_ptr CreateCommandBufferInQueue( diff --git a/impeller/renderer/backend/metal/context_mtl.mm b/impeller/renderer/backend/metal/context_mtl.mm index cc4ea365cab42..be0e29b38f02e 100644 --- a/impeller/renderer/backend/metal/context_mtl.mm +++ b/impeller/renderer/backend/metal/context_mtl.mm @@ -71,11 +71,9 @@ static bool DeviceSupportsComputeSubgroups(id device) { id device, id command_queue, NSArray>* shader_libraries, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch) : device_(device), command_queue_(command_queue), - worker_task_runner_(std::move(worker_task_runner)), is_gpu_disabled_sync_switch_(std::move(is_gpu_disabled_sync_switch)) { // Validate device. if (!device_) { @@ -83,6 +81,21 @@ static bool DeviceSupportsComputeSubgroups(id device) { return; } + // Worker task runner. + { + raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); + raster_message_loop_->PostTaskToAllWorkers([]() { + [[NSThread currentThread] setThreadPriority:1.0]; + sched_param param; + int policy; + pthread_t thread = pthread_self(); + if (!pthread_getschedparam(thread, &policy, ¶m)) { + param.sched_priority = 50; + pthread_setschedparam(thread, policy, ¶m); + } + }); + } + // Setup the shader library. { if (shader_libraries == nil) { @@ -210,7 +223,6 @@ static bool DeviceSupportsComputeSubgroups(id device) { std::shared_ptr ContextMTL::Create( const std::vector& shader_library_paths, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch) { auto device = CreateMetalDevice(); auto command_queue = CreateMetalCommandQueue(device); @@ -220,7 +232,7 @@ static bool DeviceSupportsComputeSubgroups(id device) { auto context = std::shared_ptr(new ContextMTL( device, command_queue, MTLShaderLibraryFromFilePaths(device, shader_library_paths), - std::move(worker_task_runner), std::move(is_gpu_disabled_sync_switch))); + std::move(is_gpu_disabled_sync_switch))); if (!context->IsValid()) { FML_LOG(ERROR) << "Could not create Metal context."; return nullptr; @@ -230,7 +242,6 @@ static bool DeviceSupportsComputeSubgroups(id device) { std::shared_ptr ContextMTL::Create( const std::vector>& shader_libraries_data, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch, const std::string& library_label) { auto device = CreateMetalDevice(); @@ -238,11 +249,11 @@ static bool DeviceSupportsComputeSubgroups(id device) { if (!command_queue) { return nullptr; } - auto context = std::shared_ptr(new ContextMTL( - device, command_queue, - MTLShaderLibraryFromFileData(device, shader_libraries_data, - library_label), - std::move(worker_task_runner), std::move(is_gpu_disabled_sync_switch))); + auto context = std::shared_ptr( + new ContextMTL(device, command_queue, + MTLShaderLibraryFromFileData(device, shader_libraries_data, + library_label), + std::move(is_gpu_disabled_sync_switch))); if (!context->IsValid()) { FML_LOG(ERROR) << "Could not create Metal context."; return nullptr; @@ -254,14 +265,13 @@ static bool DeviceSupportsComputeSubgroups(id device) { id device, id command_queue, const std::vector>& shader_libraries_data, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch, const std::string& library_label) { - auto context = std::shared_ptr(new ContextMTL( - device, command_queue, - MTLShaderLibraryFromFileData(device, shader_libraries_data, - library_label), - std::move(worker_task_runner), std::move(is_gpu_disabled_sync_switch))); + auto context = std::shared_ptr( + new ContextMTL(device, command_queue, + MTLShaderLibraryFromFileData(device, shader_libraries_data, + library_label), + std::move(is_gpu_disabled_sync_switch))); if (!context->IsValid()) { FML_LOG(ERROR) << "Could not create Metal context."; return nullptr; @@ -301,9 +311,9 @@ static bool DeviceSupportsComputeSubgroups(id device) { return CreateCommandBufferInQueue(command_queue_); } -const std::shared_ptr& +const std::shared_ptr ContextMTL::GetWorkerTaskRunner() const { - return worker_task_runner_; + return raster_message_loop_->GetTaskRunner(); } std::shared_ptr ContextMTL::GetIsGpuDisabledSyncSwitch() diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 74ba94804a0f4..ec1290ed4aa4d 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -4,6 +4,9 @@ #include "impeller/renderer/backend/vulkan/context_vk.h" +#include +#include +#include #include #include #include @@ -120,12 +123,14 @@ void ContextVK::Setup(Settings settings) { return; } - if (!settings.worker_task_runner) { - VALIDATION_LOG - << "Cannot set up a Vulkan context without a worker task runner."; - return; - } - worker_task_runner_ = settings.worker_task_runner; + raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); +#ifdef FML_OS_ANDROID + raster_message_loop_->PostTaskToAllWorkers([]() { + if (::setpriority(PRIO_PROCESS, gettid(), -5) != 0) { + FML_LOG(ERROR) << "Failed to set Workers task runner priority"; + } + }); +#endif // FML_OS_ANDROID auto& dispatcher = VULKAN_HPP_DEFAULT_DISPATCHER; dispatcher.init(settings.proc_address_callback); @@ -335,10 +340,10 @@ void ContextVK::Setup(Settings settings) { /// Setup the pipeline library. /// auto pipeline_library = std::shared_ptr( - new PipelineLibraryVK(device_holder, // - caps, // - std::move(settings.cache_directory), // - worker_task_runner_ // + new PipelineLibraryVK(device_holder, // + caps, // + std::move(settings.cache_directory), // + raster_message_loop_->GetTaskRunner() // )); if (!pipeline_library->IsValid()) { @@ -458,7 +463,7 @@ const vk::Device& ContextVK::GetDevice() const { const std::shared_ptr ContextVK::GetConcurrentWorkerTaskRunner() const { - return worker_task_runner_; + return raster_message_loop_->GetTaskRunner(); } std::unique_ptr ContextVK::AcquireNextSurface() { diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index cadb168ea4b68..567bde9b3d409 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -39,7 +39,6 @@ class ContextVK final : public Context, PFN_vkGetInstanceProcAddr proc_address_callback = nullptr; std::vector> shader_libraries_data; fml::UniqueFD cache_directory; - std::shared_ptr worker_task_runner; bool enable_validation = false; Settings() = default; @@ -159,7 +158,7 @@ class ContextVK final : public Context, std::shared_ptr device_capabilities_; std::shared_ptr fence_waiter_; std::string device_name_; - std::shared_ptr worker_task_runner_; + std::shared_ptr raster_message_loop_; const uint64_t hash_; bool is_valid_ = false; diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index d4faece695e4d..a6f3d08e16dc1 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -433,8 +433,6 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, std::shared_ptr CreateMockVulkanContext(void) { ContextVK::Settings settings; auto message_loop = fml::ConcurrentMessageLoop::Create(); - settings.worker_task_runner = - std::make_shared(message_loop); settings.proc_address_callback = GetMockVulkanProcAddress; return ContextVK::Create(std::move(settings)); } diff --git a/shell/common/shell_test_platform_view_metal.mm b/shell/common/shell_test_platform_view_metal.mm index 598ecd5f87557..d4e5da2127b90 100644 --- a/shell/common/shell_test_platform_view_metal.mm +++ b/shell/common/shell_test_platform_view_metal.mm @@ -35,11 +35,9 @@ explicit DarwinContextMetal(bool impeller, std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch) : context_(impeller ? nil : [[FlutterDarwinContextMetalSkia alloc] initWithDefaultMTLDevice]), - impeller_context_( - impeller ? [[FlutterDarwinContextMetalImpeller alloc] - initWithTaskRunner:std::move(worker_task_runner) - is_gpu_disabled_sync_switch:std::move(is_gpu_disabled_sync_switch)] - : nil), + impeller_context_(impeller ? [[FlutterDarwinContextMetalImpeller alloc] + init:std::move(is_gpu_disabled_sync_switch)] + : nil), offscreen_texture_(CreateOffscreenTexture( impeller ? [impeller_context_ context]->GetMTLDevice() : [context_ device])) {} diff --git a/shell/platform/android/BUILD.gn b/shell/platform/android/BUILD.gn index db8a0c5099f3c..15bfe17524176 100644 --- a/shell/platform/android/BUILD.gn +++ b/shell/platform/android/BUILD.gn @@ -43,7 +43,6 @@ executable("flutter_shell_native_unittests") { sources = [ "android_context_gl_impeller_unittests.cc", "android_context_gl_unittests.cc", - "android_context_vulkan_impeller_unittests.cc", "android_shell_holder_unittests.cc", "apk_asset_provider_unittests.cc", "flutter_shell_native_unittests.cc", diff --git a/shell/platform/android/android_context_vulkan_impeller.cc b/shell/platform/android/android_context_vulkan_impeller.cc index 9d8deaaa1217e..50df30cecb5e1 100644 --- a/shell/platform/android/android_context_vulkan_impeller.cc +++ b/shell/platform/android/android_context_vulkan_impeller.cc @@ -14,7 +14,6 @@ namespace flutter { static std::shared_ptr CreateImpellerContext( const fml::RefPtr& proc_table, - std::shared_ptr worker_task_runner, bool enable_vulkan_validation) { std::vector> shader_mappings = { std::make_shared(impeller_entity_shaders_vk_data, @@ -32,7 +31,6 @@ static std::shared_ptr CreateImpellerContext( settings.proc_address_callback = instance_proc_addr; settings.shader_libraries_data = std::move(shader_mappings); settings.cache_directory = fml::paths::GetCachesDirectory(); - settings.worker_task_runner = std::move(worker_task_runner); settings.enable_validation = enable_vulkan_validation; FML_LOG(ERROR) << "Using the Impeller rendering backend (Vulkan)."; @@ -41,12 +39,10 @@ static std::shared_ptr CreateImpellerContext( } AndroidContextVulkanImpeller::AndroidContextVulkanImpeller( - bool enable_validation, - std::shared_ptr worker_task_runner) + bool enable_validation) : AndroidContext(AndroidRenderingAPI::kVulkan), proc_table_(fml::MakeRefCounted()) { - auto impeller_context = CreateImpellerContext( - proc_table_, std::move(worker_task_runner), enable_validation); + auto impeller_context = CreateImpellerContext(proc_table_, enable_validation); SetImpellerContext(impeller_context); is_valid_ = proc_table_->HasAcquiredMandatoryProcAddresses() && impeller_context; diff --git a/shell/platform/android/android_context_vulkan_impeller.h b/shell/platform/android/android_context_vulkan_impeller.h index 1c990d238eec1..02e9a305cd039 100644 --- a/shell/platform/android/android_context_vulkan_impeller.h +++ b/shell/platform/android/android_context_vulkan_impeller.h @@ -14,9 +14,7 @@ namespace flutter { class AndroidContextVulkanImpeller : public AndroidContext { public: - AndroidContextVulkanImpeller( - bool enable_validation, - std::shared_ptr worker_task_runner); + AndroidContextVulkanImpeller(bool enable_validation); ~AndroidContextVulkanImpeller(); diff --git a/shell/platform/android/android_context_vulkan_impeller_unittests.cc b/shell/platform/android/android_context_vulkan_impeller_unittests.cc deleted file mode 100644 index 6ac09d8e8dd72..0000000000000 --- a/shell/platform/android/android_context_vulkan_impeller_unittests.cc +++ /dev/null @@ -1,31 +0,0 @@ -#include - -#include "flutter/fml/synchronization/waitable_event.h" -#include "flutter/impeller/renderer/backend/vulkan/context_vk.h" -#include "flutter/shell/platform/android/android_context_vulkan_impeller.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -namespace flutter { -namespace testing { - -TEST(AndroidContextVulkanImpeller, DoesNotCreateOwnMessageLoop) { - auto loop = fml::ConcurrentMessageLoop::Create(); - auto context = std::make_unique( - false, loop->GetTaskRunner()); - ASSERT_TRUE(context); - - auto impeller_context = std::static_pointer_cast( - context->GetImpellerContext()); - ASSERT_TRUE(impeller_context); - - fml::AutoResetWaitableEvent latch; - impeller_context->GetConcurrentWorkerTaskRunner()->PostTask([loop, &latch]() { - ASSERT_TRUE(loop->RunsTasksOnCurrentThread()); - latch.Signal(); - }); - latch.Wait(); -} - -} // namespace testing -} // namespace flutter diff --git a/shell/platform/android/android_shell_holder.cc b/shell/platform/android/android_shell_holder.cc index f555c38b53a1c..becc6861a6060 100644 --- a/shell/platform/android/android_shell_holder.cc +++ b/shell/platform/android/android_shell_holder.cc @@ -109,10 +109,9 @@ AndroidShellHolder::AndroidShellHolder( [&jni_facade, &weak_platform_view](Shell& shell) { std::unique_ptr platform_view_android; platform_view_android = std::make_unique( - shell, // delegate - shell.GetTaskRunners(), // task runners - shell.GetConcurrentWorkerTaskRunner(), // worker task runner - jni_facade, // JNI interop + shell, // delegate + shell.GetTaskRunners(), // task runners + jni_facade, // JNI interop shell.GetSettings() .enable_software_rendering, // use software rendering shell.GetSettings().msaa_samples // msaa sample count diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index a1053fbcaaa13..2993c3d8804a3 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -63,7 +63,6 @@ std::unique_ptr AndroidSurfaceFactoryImpl::CreateSurface() { static std::shared_ptr CreateAndroidContext( bool use_software_rendering, const flutter::TaskRunners& task_runners, - const std::shared_ptr& worker_task_runner, uint8_t msaa_samples, bool enable_impeller, const std::optional& impeller_backend, @@ -90,10 +89,10 @@ static std::shared_ptr CreateAndroidContext( std::make_unique()); case AndroidRenderingAPI::kVulkan: return std::make_unique( - enable_vulkan_validation, worker_task_runner); + enable_vulkan_validation); case AndroidRenderingAPI::kAutoselect: { auto vulkan_backend = std::make_unique( - enable_vulkan_validation, worker_task_runner); + enable_vulkan_validation); if (!vulkan_backend->IsValid()) { return std::make_unique( std::make_unique()); @@ -115,7 +114,6 @@ static std::shared_ptr CreateAndroidContext( PlatformViewAndroid::PlatformViewAndroid( PlatformView::Delegate& delegate, const flutter::TaskRunners& task_runners, - const std::shared_ptr& worker_task_runner, const std::shared_ptr& jni_facade, bool use_software_rendering, uint8_t msaa_samples) @@ -126,7 +124,6 @@ PlatformViewAndroid::PlatformViewAndroid( CreateAndroidContext( use_software_rendering, task_runners, - worker_task_runner, msaa_samples, delegate.OnPlatformViewGetSettings().enable_impeller, delegate.OnPlatformViewGetSettings().impeller_backend, diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index 300f3ab9b6c13..069b2f920b3af 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -42,13 +42,11 @@ class PlatformViewAndroid final : public PlatformView { public: static bool Register(JNIEnv* env); - PlatformViewAndroid( - PlatformView::Delegate& delegate, - const flutter::TaskRunners& task_runners, - const std::shared_ptr& worker_task_runner, - const std::shared_ptr& jni_facade, - bool use_software_rendering, - uint8_t msaa_samples); + PlatformViewAndroid(PlatformView::Delegate& delegate, + const flutter::TaskRunners& task_runners, + const std::shared_ptr& jni_facade, + bool use_software_rendering, + uint8_t msaa_samples); //---------------------------------------------------------------------------- /// @brief Creates a new PlatformViewAndroid but using an existing diff --git a/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.h b/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.h index 4a09c6d50e0d6..549723de64679 100644 --- a/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.h +++ b/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.h @@ -25,9 +25,7 @@ NS_ASSUME_NONNULL_BEGIN /** * Initializes a FlutterDarwinContextMetalImpeller. */ -- (instancetype)initWithTaskRunner:(std::shared_ptr)task_runner - is_gpu_disabled_sync_switch: - (std::shared_ptr)is_gpu_disabled_sync_switch; +- (instancetype)init:(std::shared_ptr)is_gpu_disabled_sync_switch; /** * Creates an external texture with the specified ID and contents. diff --git a/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm b/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm index 34cc332233cd0..00c4115698c08 100644 --- a/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm +++ b/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.mm @@ -17,7 +17,6 @@ FLUTTER_ASSERT_ARC static std::shared_ptr CreateImpellerContext( - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch) { std::vector> shader_mappings = { std::make_shared(impeller_entity_shaders_data, @@ -29,9 +28,8 @@ std::make_shared(impeller_framebuffer_blend_shaders_data, impeller_framebuffer_blend_shaders_length), }; - auto context = - impeller::ContextMTL::Create(shader_mappings, std::move(worker_task_runner), - std::move(is_gpu_disabled_sync_switch), "Impeller Library"); + auto context = impeller::ContextMTL::Create( + shader_mappings, std::move(is_gpu_disabled_sync_switch), "Impeller Library"); if (!context) { FML_LOG(ERROR) << "Could not create Metal Impeller Context."; return nullptr; @@ -43,13 +41,10 @@ @implementation FlutterDarwinContextMetalImpeller -- (instancetype)initWithTaskRunner:(std::shared_ptr)task_runner - is_gpu_disabled_sync_switch: - (std::shared_ptr)is_gpu_disabled_sync_switch { +- (instancetype)init:(std::shared_ptr)is_gpu_disabled_sync_switch { self = [super init]; if (self != nil) { - _context = - CreateImpellerContext(std::move(task_runner), std::move(is_gpu_disabled_sync_switch)); + _context = CreateImpellerContext(std::move(is_gpu_disabled_sync_switch)); id device = _context->GetMTLDevice(); if (!device) { FML_DLOG(ERROR) << "Could not acquire Metal device."; diff --git a/shell/platform/darwin/ios/ios_context.h b/shell/platform/darwin/ios/ios_context.h index 35065675750b3..9a4049b581826 100644 --- a/shell/platform/darwin/ios/ios_context.h +++ b/shell/platform/darwin/ios/ios_context.h @@ -53,8 +53,6 @@ class IOSContext { /// @param[in] msaa_samples /// The number of MSAA samples to use. Only supplied to /// Skia, must be either 0, 1, 2, 4, or 8. - /// @param[in] task_runner - /// The engine concurrent task runner. /// /// @return A valid context on success. `nullptr` on failure. /// @@ -62,7 +60,6 @@ class IOSContext { IOSRenderingAPI api, IOSRenderingBackend backend, MsaaSampleCount msaa_samples, - std::shared_ptr task_runner, std::shared_ptr is_gpu_disabled_sync_switch); //---------------------------------------------------------------------------- diff --git a/shell/platform/darwin/ios/ios_context.mm b/shell/platform/darwin/ios/ios_context.mm index fba7071f6d65f..007de7a7105fd 100644 --- a/shell/platform/darwin/ios/ios_context.mm +++ b/shell/platform/darwin/ios/ios_context.mm @@ -22,7 +22,6 @@ IOSRenderingAPI api, IOSRenderingBackend backend, MsaaSampleCount msaa_samples, - std::shared_ptr task_runner, std::shared_ptr is_gpu_disabled_sync_switch) { switch (api) { case IOSRenderingAPI::kSoftware: @@ -33,8 +32,7 @@ case IOSRenderingBackend::kSkia: return std::make_unique(msaa_samples); case IOSRenderingBackend::kImpeller: - return std::make_unique(std::move(task_runner), - std::move(is_gpu_disabled_sync_switch)); + return std::make_unique(std::move(is_gpu_disabled_sync_switch)); } #endif // SHELL_ENABLE_METAL default: diff --git a/shell/platform/darwin/ios/ios_context_metal_impeller.h b/shell/platform/darwin/ios/ios_context_metal_impeller.h index 10136218291fd..1647b3745c97a 100644 --- a/shell/platform/darwin/ios/ios_context_metal_impeller.h +++ b/shell/platform/darwin/ios/ios_context_metal_impeller.h @@ -20,8 +20,7 @@ namespace flutter { class IOSContextMetalImpeller final : public IOSContext { public: - IOSContextMetalImpeller(std::shared_ptr task_runner, - std::shared_ptr is_gpu_disabled_sync_switch); + IOSContextMetalImpeller(std::shared_ptr is_gpu_disabled_sync_switch); ~IOSContextMetalImpeller(); diff --git a/shell/platform/darwin/ios/ios_context_metal_impeller.mm b/shell/platform/darwin/ios/ios_context_metal_impeller.mm index 75f3a531fcd69..d6187221a7436 100644 --- a/shell/platform/darwin/ios/ios_context_metal_impeller.mm +++ b/shell/platform/darwin/ios/ios_context_metal_impeller.mm @@ -9,13 +9,11 @@ namespace flutter { IOSContextMetalImpeller::IOSContextMetalImpeller( - std::shared_ptr task_runner, std::shared_ptr is_gpu_disabled_sync_switch) : IOSContext(MsaaSampleCount::kFour), darwin_context_metal_impeller_(fml::scoped_nsobject{ [[FlutterDarwinContextMetalImpeller alloc] - initWithTaskRunner:std::move(task_runner) - is_gpu_disabled_sync_switch:std::move(is_gpu_disabled_sync_switch)]}) {} + init:std::move(is_gpu_disabled_sync_switch)]}) {} IOSContextMetalImpeller::~IOSContextMetalImpeller() = default; diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 61095c81d71a9..f600f6c0839db 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -65,7 +65,6 @@ new PlatformMessageHandlerIos(task_runners.GetPlatformTaskRunner())) {} delegate.OnPlatformViewGetSettings().enable_impeller ? IOSRenderingBackend::kImpeller : IOSRenderingBackend::kSkia, static_cast(delegate.OnPlatformViewGetSettings().msaa_samples), - worker_task_runner, std::move(is_gpu_disabled_sync_switch)), platform_views_controller, task_runners) {} diff --git a/shell/platform/embedder/embedder_surface_metal_impeller.h b/shell/platform/embedder/embedder_surface_metal_impeller.h index 2ddacd96f26ae..109870aa6f129 100644 --- a/shell/platform/embedder/embedder_surface_metal_impeller.h +++ b/shell/platform/embedder/embedder_surface_metal_impeller.h @@ -41,7 +41,6 @@ class EmbedderSurfaceMetalImpeller final : public EmbedderSurface, MetalDispatchTable metal_dispatch_table_; std::shared_ptr external_view_embedder_; std::shared_ptr context_; - std::shared_ptr concurrent_loop_; // |EmbedderSurface| bool IsValid() const override; diff --git a/shell/platform/embedder/embedder_surface_metal_impeller.mm b/shell/platform/embedder/embedder_surface_metal_impeller.mm index ba5615252d706..139f3e9094081 100644 --- a/shell/platform/embedder/embedder_surface_metal_impeller.mm +++ b/shell/platform/embedder/embedder_surface_metal_impeller.mm @@ -29,8 +29,7 @@ std::shared_ptr external_view_embedder) : GPUSurfaceMetalDelegate(MTLRenderTargetType::kMTLTexture), metal_dispatch_table_(std::move(metal_dispatch_table)), - external_view_embedder_(std::move(external_view_embedder)), - concurrent_loop_(fml::ConcurrentMessageLoop::Create()) { + external_view_embedder_(std::move(external_view_embedder)) { std::vector> shader_mappings = { std::make_shared(impeller_entity_shaders_data, impeller_entity_shaders_length), @@ -45,7 +44,6 @@ (id)device, // device (id)command_queue, // command_queue shader_mappings, // shader_libraries_data - concurrent_loop_->GetTaskRunner(), // worker_task_runner std::make_shared(false), // is_gpu_disabled_sync_switch "Impeller Library" // library_label ); From 57b90e3c1a4c6bdcec467c529724381b2a6d16f3 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 23 Jun 2023 16:35:02 -0700 Subject: [PATCH 2/8] ifdef --- impeller/renderer/backend/vulkan/context_vk.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index ec1290ed4aa4d..8a2abf8ed64ec 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -4,9 +4,12 @@ #include "impeller/renderer/backend/vulkan/context_vk.h" +#ifdef FML_OS_ANDROID #include #include #include +#endif // FML_OS_ANDROID + #include #include #include From 9f2f08c090a53bfff549ddb8b512e15909ec704e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Jun 2023 11:47:46 -0700 Subject: [PATCH 3/8] more cleanup --- shell/common/animator_unittests.cc | 1 - shell/common/shell_test_platform_view.cc | 5 +---- shell/common/shell_test_platform_view.h | 1 - shell/common/shell_test_platform_view_metal.h | 1 - shell/common/shell_test_platform_view_metal.mm | 3 --- shell/common/shell_unittests.cc | 1 - 6 files changed, 1 insertion(+), 11 deletions(-) diff --git a/shell/common/animator_unittests.cc b/shell/common/animator_unittests.cc index cf20635b14203..e15aa1b1c98c0 100644 --- a/shell/common/animator_unittests.cc +++ b/shell/common/animator_unittests.cc @@ -82,7 +82,6 @@ TEST_F(ShellTest, VSyncTargetTime) { return ShellTestPlatformView::Create( shell, shell.GetTaskRunners(), vsync_clock, create_vsync_waiter, ShellTestPlatformView::BackendType::kDefaultBackend, nullptr, - shell.GetConcurrentWorkerTaskRunner(), shell.GetIsGpuDisabledSyncSwitch()); }, [](Shell& shell) { return std::make_unique(shell); }); diff --git a/shell/common/shell_test_platform_view.cc b/shell/common/shell_test_platform_view.cc index b8188c8825150..248448f567c7f 100644 --- a/shell/common/shell_test_platform_view.cc +++ b/shell/common/shell_test_platform_view.cc @@ -27,7 +27,6 @@ std::unique_ptr ShellTestPlatformView::Create( BackendType backend, const std::shared_ptr& shell_test_external_view_embedder, - const std::shared_ptr& worker_task_runner, const std::shared_ptr& is_gpu_disabled_sync_switch) { // TODO(gw280): https://github.com/flutter/flutter/issues/50298 // Make this fully runtime configurable @@ -49,8 +48,7 @@ std::unique_ptr ShellTestPlatformView::Create( case BackendType::kMetalBackend: return std::make_unique( delegate, task_runners, vsync_clock, create_vsync_waiter, - shell_test_external_view_embedder, worker_task_runner, - is_gpu_disabled_sync_switch); + shell_test_external_view_embedder, is_gpu_disabled_sync_switch); #endif // SHELL_ENABLE_METAL default: @@ -84,7 +82,6 @@ std::unique_ptr ShellTestPlatformViewBuilder::operator()( create_vsync_waiter, // config_.rendering_backend, // config_.shell_test_external_view_embedder, // - shell.GetConcurrentWorkerTaskRunner(), // shell.GetIsGpuDisabledSyncSwitch() // ); } diff --git a/shell/common/shell_test_platform_view.h b/shell/common/shell_test_platform_view.h index 3ac0d4352c011..fdbe870fb6039 100644 --- a/shell/common/shell_test_platform_view.h +++ b/shell/common/shell_test_platform_view.h @@ -29,7 +29,6 @@ class ShellTestPlatformView : public PlatformView { BackendType backend, const std::shared_ptr& shell_test_external_view_embedder, - const std::shared_ptr& worker_task_runner, const std::shared_ptr& is_gpu_disabled_sync_switch); diff --git a/shell/common/shell_test_platform_view_metal.h b/shell/common/shell_test_platform_view_metal.h index a20b9c5f3f929..1b9f951d40c80 100644 --- a/shell/common/shell_test_platform_view_metal.h +++ b/shell/common/shell_test_platform_view_metal.h @@ -24,7 +24,6 @@ class ShellTestPlatformViewMetal final : public ShellTestPlatformView, CreateVsyncWaiter create_vsync_waiter, std::shared_ptr shell_test_external_view_embedder, - const std::shared_ptr& worker_task_runner, const std::shared_ptr& is_gpu_disabled_sync_switch); diff --git a/shell/common/shell_test_platform_view_metal.mm b/shell/common/shell_test_platform_view_metal.mm index d4e5da2127b90..4e96c6d1b3aa6 100644 --- a/shell/common/shell_test_platform_view_metal.mm +++ b/shell/common/shell_test_platform_view_metal.mm @@ -32,7 +32,6 @@ class DarwinContextMetal { public: explicit DarwinContextMetal(bool impeller, - std::shared_ptr worker_task_runner, std::shared_ptr is_gpu_disabled_sync_switch) : context_(impeller ? nil : [[FlutterDarwinContextMetalSkia alloc] initWithDefaultMTLDevice]), impeller_context_(impeller ? [[FlutterDarwinContextMetalImpeller alloc] @@ -72,12 +71,10 @@ GPUMTLTextureInfo offscreen_texture_info() const { std::shared_ptr vsync_clock, CreateVsyncWaiter create_vsync_waiter, std::shared_ptr shell_test_external_view_embedder, - const std::shared_ptr& worker_task_runner, const std::shared_ptr& is_gpu_disabled_sync_switch) : ShellTestPlatformView(delegate, task_runners), GPUSurfaceMetalDelegate(MTLRenderTargetType::kMTLTexture), metal_context_(std::make_unique(GetSettings().enable_impeller, - worker_task_runner, is_gpu_disabled_sync_switch)), create_vsync_waiter_(std::move(create_vsync_waiter)), vsync_clock_(std::move(vsync_clock)), diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 0e110aa8540ac..16abef0dbc0f3 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -413,7 +413,6 @@ TEST_F(ShellTest, std::make_unique(task_runners)); }, ShellTestPlatformView::BackendType::kDefaultBackend, nullptr, - shell.GetConcurrentWorkerTaskRunner(), shell.GetIsGpuDisabledSyncSwitch()); }, [](Shell& shell) { return std::make_unique(shell); }); From 6009f8257d0488a3188fbafac06b94907a08d353 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Jun 2023 11:48:11 -0700 Subject: [PATCH 4/8] ++ --- shell/common/shell_test_platform_view_metal.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/shell/common/shell_test_platform_view_metal.h b/shell/common/shell_test_platform_view_metal.h index 1b9f951d40c80..c5c0a8d454d04 100644 --- a/shell/common/shell_test_platform_view_metal.h +++ b/shell/common/shell_test_platform_view_metal.h @@ -17,15 +17,14 @@ class DarwinContextMetal; class ShellTestPlatformViewMetal final : public ShellTestPlatformView, public GPUSurfaceMetalDelegate { public: - ShellTestPlatformViewMetal( - PlatformView::Delegate& delegate, - const TaskRunners& task_runners, - std::shared_ptr vsync_clock, - CreateVsyncWaiter create_vsync_waiter, - std::shared_ptr - shell_test_external_view_embedder, - const std::shared_ptr& - is_gpu_disabled_sync_switch); + ShellTestPlatformViewMetal(PlatformView::Delegate& delegate, + const TaskRunners& task_runners, + std::shared_ptr vsync_clock, + CreateVsyncWaiter create_vsync_waiter, + std::shared_ptr + shell_test_external_view_embedder, + const std::shared_ptr& + is_gpu_disabled_sync_switch); // |ShellTestPlatformView| virtual ~ShellTestPlatformViewMetal() override; From 066d1a764049b347097c780ae392ae820390f496 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Jun 2023 12:28:16 -0700 Subject: [PATCH 5/8] always end encoding --- .../backend/metal/command_buffer_mtl.mm | 58 ++++++++++--------- .../renderer/backend/metal/context_mtl.mm | 2 + 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/impeller/renderer/backend/metal/command_buffer_mtl.mm b/impeller/renderer/backend/metal/command_buffer_mtl.mm index 58aa6b5478148..155a9ed46fa32 100644 --- a/impeller/renderer/backend/metal/command_buffer_mtl.mm +++ b/impeller/renderer/backend/metal/command_buffer_mtl.mm @@ -202,32 +202,38 @@ static bool LogMTLCommandBufferErrorIfPresent(id buffer) { return false; } - auto task = fml::MakeCopyable([render_pass, buffer, render_command_encoder, - weak_context = context_]() { - auto context = weak_context.lock(); - if (!context) { - return; - } - auto is_gpu_disabled_sync_switch = - ContextMTL::Cast(*context).GetIsGpuDisabledSyncSwitch(); - is_gpu_disabled_sync_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse( - [&render_pass, &render_command_encoder, &buffer, &context] { - auto mtl_render_pass = static_cast(render_pass.get()); - if (!mtl_render_pass->label_.empty()) { - [render_command_encoder - setLabel:@(mtl_render_pass->label_.c_str())]; - } - - auto result = mtl_render_pass->EncodeCommands( - context->GetResourceAllocator(), render_command_encoder); - [render_command_encoder endEncoding]; - if (result) { - [buffer commit]; - } else { - VALIDATION_LOG << "Failed to encode command buffer"; - } - })); - }); + auto task = fml::MakeCopyable( + [render_pass, buffer, render_command_encoder, weak_context = context_]() { + auto context = weak_context.lock(); + if (!context) { + return; + } + auto is_gpu_disabled_sync_switch = + ContextMTL::Cast(*context).GetIsGpuDisabledSyncSwitch(); + is_gpu_disabled_sync_switch->Execute( + fml::SyncSwitch::Handlers() + .SetIfFalse([&render_pass, &render_command_encoder, &buffer, + &context] { + auto mtl_render_pass = + static_cast(render_pass.get()); + if (!mtl_render_pass->label_.empty()) { + [render_command_encoder + setLabel:@(mtl_render_pass->label_.c_str())]; + } + + auto result = mtl_render_pass->EncodeCommands( + context->GetResourceAllocator(), render_command_encoder); + [render_command_encoder endEncoding]; + if (result) { + [buffer commit]; + } else { + VALIDATION_LOG << "Failed to encode command buffer"; + } + }) + .SetIfTrue([&render_command_encoder] { + [render_command_encoder endEncoding]; + })); + }); worker_task_runner->PostTask(task); return true; } diff --git a/impeller/renderer/backend/metal/context_mtl.mm b/impeller/renderer/backend/metal/context_mtl.mm index be0e29b38f02e..869b8dc33c3ad 100644 --- a/impeller/renderer/backend/metal/context_mtl.mm +++ b/impeller/renderer/backend/metal/context_mtl.mm @@ -85,6 +85,8 @@ static bool DeviceSupportsComputeSubgroups(id device) { { raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); raster_message_loop_->PostTaskToAllWorkers([]() { + // See https://github.com/flutter/flutter/issues/65752 + // Intentionally opt out of QoS for raster task workloads. [[NSThread currentThread] setThreadPriority:1.0]; sched_param param; int policy; From 21e986b77b4544011382182fdb85607d08aca975 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Jun 2023 14:38:25 -0700 Subject: [PATCH 6/8] ++ --- impeller/renderer/backend/metal/command_buffer_mtl.mm | 1 + impeller/renderer/backend/metal/context_mtl.mm | 3 ++- impeller/renderer/backend/vulkan/context_vk.cc | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/metal/command_buffer_mtl.mm b/impeller/renderer/backend/metal/command_buffer_mtl.mm index 155a9ed46fa32..894f8b1fdce82 100644 --- a/impeller/renderer/backend/metal/command_buffer_mtl.mm +++ b/impeller/renderer/backend/metal/command_buffer_mtl.mm @@ -206,6 +206,7 @@ static bool LogMTLCommandBufferErrorIfPresent(id buffer) { [render_pass, buffer, render_command_encoder, weak_context = context_]() { auto context = weak_context.lock(); if (!context) { + [render_command_encoder endEncoding]; return; } auto is_gpu_disabled_sync_switch = diff --git a/impeller/renderer/backend/metal/context_mtl.mm b/impeller/renderer/backend/metal/context_mtl.mm index 869b8dc33c3ad..a852bcd362441 100644 --- a/impeller/renderer/backend/metal/context_mtl.mm +++ b/impeller/renderer/backend/metal/context_mtl.mm @@ -83,7 +83,8 @@ static bool DeviceSupportsComputeSubgroups(id device) { // Worker task runner. { - raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); + raster_message_loop_ = fml::ConcurrentMessageLoop::Create( + std::min(4u, std::thread::hardware_concurrency())); raster_message_loop_->PostTaskToAllWorkers([]() { // See https://github.com/flutter/flutter/issues/65752 // Intentionally opt out of QoS for raster task workloads. diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 8a2abf8ed64ec..df6323ad120fd 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -126,7 +126,8 @@ void ContextVK::Setup(Settings settings) { return; } - raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); + raster_message_loop_ = fml::ConcurrentMessageLoop::Create( + std::min(4u, std::thread::hardware_concurrency())); #ifdef FML_OS_ANDROID raster_message_loop_->PostTaskToAllWorkers([]() { if (::setpriority(PRIO_PROCESS, gettid(), -5) != 0) { From a70c98c79dcd78f74abf2b27406274ab1cb24626 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 25 Jun 2023 11:10:19 -0700 Subject: [PATCH 7/8] maybe this will work. --- fml/concurrent_message_loop.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/fml/concurrent_message_loop.cc b/fml/concurrent_message_loop.cc index 911e4038516b6..c99de48e7b3c4 100644 --- a/fml/concurrent_message_loop.cc +++ b/fml/concurrent_message_loop.cc @@ -28,8 +28,17 @@ ConcurrentMessageLoop::ConcurrentMessageLoop(size_t worker_count) ConcurrentMessageLoop::~ConcurrentMessageLoop() { Terminate(); - for (auto& worker : workers_) { - worker.join(); + for (auto i = 0u; i < workers_.size(); i++) { + auto& worker = workers_[i]; + auto thread_id = worker_thread_ids_[i]; + if (std::this_thread::get_id() != thread_id) { + FML_DCHECK(worker.joinable()); + worker.join(); + } else { + // The concurrent message loop is being deleted from a worker + // thread. detach this thread to avoid creating a deadlock. + worker.detach(); + } } } From 199655d3e911b3ee9bcbf4d4353fab2c2a45646d Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 25 Jun 2023 14:48:52 -0700 Subject: [PATCH 8/8] add explicit shutdown for testing. --- fml/concurrent_message_loop.cc | 14 +++----------- impeller/playground/playground.cc | 3 +++ impeller/renderer/backend/gles/context_gles.cc | 2 ++ impeller/renderer/backend/gles/context_gles.h | 3 +++ impeller/renderer/backend/metal/context_mtl.h | 3 +++ impeller/renderer/backend/metal/context_mtl.mm | 5 +++++ impeller/renderer/backend/vulkan/context_vk.cc | 4 ++++ impeller/renderer/backend/vulkan/context_vk.h | 3 +++ impeller/renderer/context.h | 4 ++++ impeller/renderer/testing/mocks.h | 2 ++ 10 files changed, 32 insertions(+), 11 deletions(-) diff --git a/fml/concurrent_message_loop.cc b/fml/concurrent_message_loop.cc index c99de48e7b3c4..ace1985c24a15 100644 --- a/fml/concurrent_message_loop.cc +++ b/fml/concurrent_message_loop.cc @@ -28,17 +28,9 @@ ConcurrentMessageLoop::ConcurrentMessageLoop(size_t worker_count) ConcurrentMessageLoop::~ConcurrentMessageLoop() { Terminate(); - for (auto i = 0u; i < workers_.size(); i++) { - auto& worker = workers_[i]; - auto thread_id = worker_thread_ids_[i]; - if (std::this_thread::get_id() != thread_id) { - FML_DCHECK(worker.joinable()); - worker.join(); - } else { - // The concurrent message loop is being deleted from a worker - // thread. detach this thread to avoid creating a deadlock. - worker.detach(); - } + for (auto& worker : workers_) { + FML_DCHECK(worker.joinable()); + worker.join(); } } diff --git a/impeller/playground/playground.cc b/impeller/playground/playground.cc index ce81afab97f0c..447da44b3d8ac 100644 --- a/impeller/playground/playground.cc +++ b/impeller/playground/playground.cc @@ -139,6 +139,9 @@ void Playground::SetupWindow() { } void Playground::TeardownWindow() { + if (context_) { + context_->Shutdown(); + } context_.reset(); renderer_.reset(); impl_.reset(); diff --git a/impeller/renderer/backend/gles/context_gles.cc b/impeller/renderer/backend/gles/context_gles.cc index 47e7256374b18..4f590e2b2d729 100644 --- a/impeller/renderer/backend/gles/context_gles.cc +++ b/impeller/renderer/backend/gles/context_gles.cc @@ -107,6 +107,8 @@ bool ContextGLES::IsValid() const { return is_valid_; } +void ContextGLES::Shutdown() {} + // |Context| std::string ContextGLES::DescribeGpuModel() const { return reactor_->GetProcTable().GetDescription()->GetString(); diff --git a/impeller/renderer/backend/gles/context_gles.h b/impeller/renderer/backend/gles/context_gles.h index 2faea35232a04..37fca72b3c1a1 100644 --- a/impeller/renderer/backend/gles/context_gles.h +++ b/impeller/renderer/backend/gles/context_gles.h @@ -72,6 +72,9 @@ class ContextGLES final : public Context, // |Context| const std::shared_ptr& GetCapabilities() const override; + // |Context| + void Shutdown() override; + FML_DISALLOW_COPY_AND_ASSIGN(ContextGLES); }; diff --git a/impeller/renderer/backend/metal/context_mtl.h b/impeller/renderer/backend/metal/context_mtl.h index 2659b188e6f9f..3a88efc52d9b1 100644 --- a/impeller/renderer/backend/metal/context_mtl.h +++ b/impeller/renderer/backend/metal/context_mtl.h @@ -81,6 +81,9 @@ class ContextMTL final : public Context, // |Context| bool UpdateOffscreenLayerPixelFormat(PixelFormat format) override; + // |Context| + void Shutdown() override; + id CreateMTLCommandBuffer() const; const std::shared_ptr GetWorkerTaskRunner() const; diff --git a/impeller/renderer/backend/metal/context_mtl.mm b/impeller/renderer/backend/metal/context_mtl.mm index a852bcd362441..1da9684800087 100644 --- a/impeller/renderer/backend/metal/context_mtl.mm +++ b/impeller/renderer/backend/metal/context_mtl.mm @@ -314,6 +314,11 @@ new ContextMTL(device, command_queue, return CreateCommandBufferInQueue(command_queue_); } +// |Context| +void ContextMTL::Shutdown() { + raster_message_loop_.reset(); +} + const std::shared_ptr ContextMTL::GetWorkerTaskRunner() const { return raster_message_loop_->GetTaskRunner(); diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index df6323ad120fd..81e700f8bca8a 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -470,6 +470,10 @@ ContextVK::GetConcurrentWorkerTaskRunner() const { return raster_message_loop_->GetTaskRunner(); } +void ContextVK::Shutdown() { + raster_message_loop_->Terminate(); +} + std::unique_ptr ContextVK::AcquireNextSurface() { TRACE_EVENT0("impeller", __FUNCTION__); auto surface = swapchain_ ? swapchain_->AcquireNextDrawable() : nullptr; diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index 567bde9b3d409..25f1ff6890d17 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -77,6 +77,9 @@ class ContextVK final : public Context, // |Context| const std::shared_ptr& GetCapabilities() const override; + // |Context| + void Shutdown() override; + void SetOffscreenFormat(PixelFormat pixel_format); template diff --git a/impeller/renderer/context.h b/impeller/renderer/context.h index 36bf48c6ed16f..554b496893844 100644 --- a/impeller/renderer/context.h +++ b/impeller/renderer/context.h @@ -42,6 +42,10 @@ class Context { virtual std::shared_ptr CreateCommandBuffer() const = 0; + /// @brief Force all pending async work to finish by deleting any owned + /// concurrent message loops. + virtual void Shutdown() = 0; + protected: Context(); diff --git a/impeller/renderer/testing/mocks.h b/impeller/renderer/testing/mocks.h index e845a2a4b1f18..3066523cf8f1b 100644 --- a/impeller/renderer/testing/mocks.h +++ b/impeller/renderer/testing/mocks.h @@ -90,6 +90,8 @@ class MockImpellerContext : public Context { MOCK_CONST_METHOD0(IsValid, bool()); + MOCK_METHOD0(Shutdown, void()); + MOCK_CONST_METHOD0(GetResourceAllocator, std::shared_ptr()); MOCK_CONST_METHOD0(GetShaderLibrary, std::shared_ptr());