From 641765e283cf4ca3c45ffe05d4f98d9788099114 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Tue, 25 Feb 2025 13:33:27 +0100 Subject: [PATCH] [Windows] Use enum to configure UI thread policy (#163727) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/flutter/flutter/pull/162935 added a boolean setting to allow merged platform and UI thread. Using boolean doesn't allow for opting out of the behavior once we enable it by default, which is something we will likely want to allow, at least for a period of time. This PR replaces the boolean with following enum: ```cpp // Configures the thread policy for running the UI isolate. typedef enum { // Default value. Currently will run the UI isolate on separate thread, // later will be changed to running the UI isolate on platform thread. Default, // Run the UI isolate on platform thread. RunOnPlatformThread, // Run the UI isolate on a separate thread. RunOnSeparateThread, } FlutterDesktopUIThreadPolicy; ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> --- .../windows/client_wrapper/flutter_engine.cc | 5 +-- .../include/flutter/dart_project.h | 30 ++++++++++------- .../shell/platform/windows/fixtures/main.dart | 5 +++ .../windows/flutter_project_bundle.cc | 3 +- .../platform/windows/flutter_project_bundle.h | 14 +++++--- .../windows/flutter_windows_engine.cc | 4 ++- .../flutter_windows_engine_unittests.cc | 33 +++++++++++++++++++ .../platform/windows/public/flutter_windows.h | 15 +++++++-- .../testing/windows_test_config_builder.cc | 6 ++++ .../testing/windows_test_config_builder.h | 6 ++++ 10 files changed, 100 insertions(+), 21 deletions(-) diff --git a/engine/src/flutter/shell/platform/windows/client_wrapper/flutter_engine.cc b/engine/src/flutter/shell/platform/windows/client_wrapper/flutter_engine.cc index 63b790bfd7..2287d41d11 100644 --- a/engine/src/flutter/shell/platform/windows/client_wrapper/flutter_engine.cc +++ b/engine/src/flutter/shell/platform/windows/client_wrapper/flutter_engine.cc @@ -8,6 +8,7 @@ #include #include "binary_messenger_impl.h" +#include "flutter_windows.h" namespace flutter { @@ -19,8 +20,8 @@ FlutterEngine::FlutterEngine(const DartProject& project) { c_engine_properties.dart_entrypoint = project.dart_entrypoint().c_str(); c_engine_properties.gpu_preference = static_cast(project.gpu_preference()); - c_engine_properties.merged_platform_ui_thread = - project.merged_platform_ui_thread(); + c_engine_properties.ui_thread_policy = + static_cast(project.ui_thread_policy()); const std::vector& entrypoint_args = project.dart_entrypoint_arguments(); diff --git a/engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h b/engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h index 3aed8c5eab..8d18146314 100644 --- a/engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h +++ b/engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h @@ -20,6 +20,17 @@ enum class GpuPreference { LowPowerPreference, }; +// Configures the thread policy for running the UI isolate. +enum class UIThreadPolicy { + // Default value. Currently will run the UI isolate on separate thread, + // later will be changed to running the UI isolate on platform thread. + Default, + // Run the UI isolate on platform thread. + RunOnPlatformThread, + // Run the UI isolate on a separate thread. + RunOnSeparateThread, +}; + // A set of Flutter and Dart assets used to initialize a Flutter engine. class DartProject { public: @@ -90,17 +101,14 @@ class DartProject { // Defaults to NoPreference. GpuPreference gpu_preference() const { return gpu_preference_; } - // Sets whether the UI isolate should run on the platform thread. - // In a future release, this setting will become a no-op when - // Flutter Windows requires merged platform and UI threads. - void set_merged_platform_ui_thread(bool merged_platform_ui_thread) { - merged_platform_ui_thread_ = merged_platform_ui_thread; + // Sets the thread policy for UI isolate. + void set_ui_thread_policy(UIThreadPolicy policy) { + ui_thread_policy_ = policy; } - // Returns whether the UI isolate should run on the platform thread. - // Defaults to false. In a future release, this setting will default - // to true. - bool merged_platform_ui_thread() const { return merged_platform_ui_thread_; } + // Returns the policy for UI isolate. + // Defaults to UIThreadPolicy::Default. + UIThreadPolicy ui_thread_policy() const { return ui_thread_policy_; } private: // Accessors for internals are private, so that they can be changed if more @@ -128,8 +136,8 @@ class DartProject { std::vector dart_entrypoint_arguments_; // The preference for GPU to be used by flutter engine. GpuPreference gpu_preference_ = GpuPreference::NoPreference; - // Whether the UI isolate should run on the platform thread. - bool merged_platform_ui_thread_ = false; + // Thread policy for UI isolate. + UIThreadPolicy ui_thread_policy_ = UIThreadPolicy::Default; }; } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/fixtures/main.dart b/engine/src/flutter/shell/platform/windows/fixtures/main.dart index 5430e86020..f35e9bf356 100644 --- a/engine/src/flutter/shell/platform/windows/fixtures/main.dart +++ b/engine/src/flutter/shell/platform/windows/fixtures/main.dart @@ -391,3 +391,8 @@ void onMetricsChangedSignalViewIds() { signal(); } + +@pragma('vm:entry-point') +void mergedUIThread() { + signal(); +} diff --git a/engine/src/flutter/shell/platform/windows/flutter_project_bundle.cc b/engine/src/flutter/shell/platform/windows/flutter_project_bundle.cc index 4971a99776..87cd0b4a32 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_project_bundle.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_project_bundle.cc @@ -32,7 +32,8 @@ FlutterProjectBundle::FlutterProjectBundle( gpu_preference_ = static_cast(properties.gpu_preference); - merged_platform_ui_thread_ = properties.merged_platform_ui_thread; + ui_thread_policy_ = + static_cast(properties.ui_thread_policy); // Resolve any relative paths. if (assets_path_.is_relative() || icu_path_.is_relative() || diff --git a/engine/src/flutter/shell/platform/windows/flutter_project_bundle.h b/engine/src/flutter/shell/platform/windows/flutter_project_bundle.h index 34892be3ab..8fa12bc8b9 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_project_bundle.h +++ b/engine/src/flutter/shell/platform/windows/flutter_project_bundle.h @@ -22,6 +22,12 @@ enum class FlutterGpuPreference { LowPowerPreference, }; +enum class FlutterUIThreadPolicy { + Default, + RunOnPlatformThread, + RunOnSeparateThread, +}; + // The data associated with a Flutter project needed to run it in an engine. class FlutterProjectBundle { public: @@ -67,8 +73,8 @@ class FlutterProjectBundle { // Returns the app's GPU preference. FlutterGpuPreference gpu_preference() const { return gpu_preference_; } - // Whether the UI isolate should be running on the platform thread. - bool merged_platform_ui_thread() const { return merged_platform_ui_thread_; } + // Returns thread policy for running the UI isolate. + FlutterUIThreadPolicy ui_thread_policy() { return ui_thread_policy_; } private: std::filesystem::path assets_path_; @@ -89,8 +95,8 @@ class FlutterProjectBundle { // App's GPU preference. FlutterGpuPreference gpu_preference_; - // Whether the UI isolate should be running on the platform thread. - bool merged_platform_ui_thread_; + // Thread policy for running the UI isolate. + FlutterUIThreadPolicy ui_thread_policy_; }; } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc index 6500bd87c8..c78540bcde 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc @@ -26,6 +26,7 @@ #include "flutter/shell/platform/windows/system_utils.h" #include "flutter/shell/platform/windows/task_runner.h" #include "flutter/third_party/accessibility/ax/ax_node.h" +#include "shell/platform/windows/flutter_project_bundle.h" // winbase.h defines GetCurrentTime as a macro. #undef GetCurrentTime @@ -294,7 +295,8 @@ bool FlutterWindowsEngine::Run(std::string_view entrypoint) { custom_task_runners.thread_priority_setter = &WindowsPlatformThreadPrioritySetter; - if (project_->merged_platform_ui_thread()) { + if (project_->ui_thread_policy() == + FlutterUIThreadPolicy::RunOnPlatformThread) { FML_LOG(WARNING) << "Running with merged platform and UI thread. Experimental."; custom_task_runners.ui_task_runner = &platform_task_runner; diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc index a7b45f7490..94e8452d4b 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #include "flutter/shell/platform/windows/flutter_windows_engine.h" #include "flutter/fml/logging.h" @@ -27,6 +28,18 @@ // winbase.h defines GetCurrentTime as a macro. #undef GetCurrentTime +namespace { +// Process the next win32 message if there is one. This can be used to +// pump the Windows platform thread task runner. +void PumpMessage() { + ::MSG msg; + if (::GetMessage(&msg, nullptr, 0, 0)) { + ::TranslateMessage(&msg); + ::DispatchMessage(&msg); + } +} +} // namespace + namespace flutter { namespace testing { @@ -1313,5 +1326,25 @@ TEST_F(FlutterWindowsEngineTest, RemoveViewFailureDoesNotHang) { "FlutterEngineRemoveView returned an unexpected result"); } +TEST_F(FlutterWindowsEngineTest, MergedUIThread) { + auto& context = GetContext(); + WindowsConfigBuilder builder{context}; + builder.SetDartEntrypoint("mergedUIThread"); + builder.SetUIThreadPolicy(FlutterDesktopUIThreadPolicy::RunOnPlatformThread); + + std::optional ui_thread_id; + + auto native_entry = CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { + ui_thread_id = std::this_thread::get_id(); + }); + context.AddNativeFunction("Signal", native_entry); + + EnginePtr engine{builder.RunHeadless()}; + while (!ui_thread_id) { + PumpMessage(); + } + ASSERT_EQ(*ui_thread_id, std::this_thread::get_id()); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/public/flutter_windows.h b/engine/src/flutter/shell/platform/windows/public/flutter_windows.h index 6f48798958..5d2bff132b 100644 --- a/engine/src/flutter/shell/platform/windows/public/flutter_windows.h +++ b/engine/src/flutter/shell/platform/windows/public/flutter_windows.h @@ -45,6 +45,17 @@ typedef enum { LowPowerPreference, } FlutterDesktopGpuPreference; +// Configures the thread policy for running the UI isolate. +typedef enum { + // Default value. Currently will run the UI isolate on separate thread, + // later will be changed to running the UI isolate on platform thread. + Default, + // Run the UI isolate on platform thread. + RunOnPlatformThread, + // Run the UI isolate on a separate thread. + RunOnSeparateThread, +} FlutterDesktopUIThreadPolicy; + // Properties for configuring a Flutter engine instance. typedef struct { // The path to the flutter_assets folder for the application to be run. @@ -81,8 +92,8 @@ typedef struct { // GPU choice preference FlutterDesktopGpuPreference gpu_preference; - // Whether the UI isolate should run on the platform thread. - bool merged_platform_ui_thread; + // Policy for the thread that runs UI isolate. + FlutterDesktopUIThreadPolicy ui_thread_policy; } FlutterDesktopEngineProperties; // ========== View Controller ========== diff --git a/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.cc b/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.cc index 6456b9add5..96b984cf0b 100644 --- a/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.cc +++ b/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.cc @@ -30,6 +30,11 @@ void WindowsConfigBuilder::SetDartEntrypoint(std::string_view entrypoint) { dart_entrypoint_ = entrypoint; } +void WindowsConfigBuilder::SetUIThreadPolicy( + FlutterDesktopUIThreadPolicy policy) { + ui_thread_policy_ = policy; +} + void WindowsConfigBuilder::AddDartEntrypointArgument(std::string_view arg) { if (arg.empty()) { return; @@ -69,6 +74,7 @@ FlutterDesktopEngineProperties WindowsConfigBuilder::GetEngineProperties() } engine_properties.gpu_preference = gpu_preference_; + engine_properties.ui_thread_policy = ui_thread_policy_; return engine_properties; } diff --git a/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.h b/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.h index 5cfb87af0b..07da63b245 100644 --- a/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.h +++ b/engine/src/flutter/shell/platform/windows/testing/windows_test_config_builder.h @@ -58,6 +58,10 @@ class WindowsConfigBuilder { // must be decorated with `@pragma('vm:entry-point')`. void SetDartEntrypoint(std::string_view entrypoint); + // Set the UI Thread policy for the engine. + // If not set defaults to FlutterDesktopUIThreadPolicy::Default; + void SetUIThreadPolicy(FlutterDesktopUIThreadPolicy policy); + // Adds an argument to the Dart entrypoint arguments List. void AddDartEntrypointArgument(std::string_view arg); @@ -86,6 +90,8 @@ class WindowsConfigBuilder { WindowsTestContext& context_; std::string dart_entrypoint_; std::vector dart_entrypoint_arguments_; + FlutterDesktopUIThreadPolicy ui_thread_policy_ = + FlutterDesktopUIThreadPolicy::Default; FlutterDesktopGpuPreference gpu_preference_ = FlutterDesktopGpuPreference::NoPreference;