[windows] Implement merged UI and platform thread by knopp · Pull Request #162935 · flutter/flutter

knopp · 2025-02-08T13:45:36Z

Original issue: #150525

Thread merging is currently disabled by default.
It is controlled from the application through DartProject:

flutter::DartProject project(L"data");
// Enables running UI isolate on platform thread
project.set_merged_platform_ui_thread(true);

Required changes to windows embedder:

Resize synchronization no longer blocks on condition variable, instead it polls the Flutter run loop (ignoring other windows messages) until the frame is available. This way resize synchronization works with both thread merging enabled and disabled.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

engine/src/flutter/shell/common/engine.h

engine/src/flutter/fml/task_queue_id.h

engine/src/flutter/shell/platform/embedder/embedder.h

engine/src/flutter/shell/platform/windows/client_wrapper/include/flutter/dart_project.h

engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

engine/src/flutter/shell/platform/windows/flutter_windows_view.cc

engine/src/flutter/shell/platform/windows/task_runner_window.cc

loic-sharma · 2025-02-11T00:07:21Z

Did you check if the FlutterWindowsEngine::CreateView and FlutterWindowsEngine::RemoveView need to be updated? See:

flutter/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

Lines 543 to 558 in 0be3f8f

    
           FlutterEngineResult result = embedder_api_.AddView(engine_, &info); 
        
           if (result != kSuccess) { 
        
             FML_LOG(ERROR) 
        
                 << "Starting the add view operation failed. FlutterEngineAddView " 
        
                    "returned an unexpected result: " 
        
                 << result << ". This indicates a bug in the Windows embedder."; 
        
             FML_DCHECK(false); 
        
             return nullptr; 
        
           } 
        
           // Block the platform thread until the engine has added the view. 
        
           // TODO(loicsharma): This blocks the platform thread eagerly and can 
        
           // cause unnecessary delay in input processing. Instead, this should block 
        
           // lazily only when the app does an operation which needs the view. 
        
           // https://github.com/flutter/flutter/issues/146248 
        
           captures.latch.Wait();

flutter/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

Lines 591 to 616 in 0be3f8f

    
           info.remove_view_callback = [](const FlutterRemoveViewResult* result) { 
        
             // This is invoked on an engine thread. If 
        
             // |FlutterRemoveViewResult.removed| is `true`, the engine guarantees the 
        
             // view won't be presented. 
        
             Captures* captures = reinterpret_cast<Captures*>(result->user_data); 
        
             captures->removed = result->removed; 
        
             captures->latch.Signal(); 
        
           }; 
        
           FlutterEngineResult result = embedder_api_.RemoveView(engine_, &info); 
        
           if (result != kSuccess) { 
        
             FML_LOG(ERROR) << "Starting the remove view operation failed. " 
        
                               "FlutterEngineRemoveView " 
        
                               "returned an unexpected result: " 
        
                            << result 
        
                            << ". This indicates a bug in the Windows embedder."; 
        
             FML_DCHECK(false); 
        
             return; 
        
           } 
        
           // Block the platform thread until the engine has removed the view. 
        
           // TODO(loicsharma): This blocks the platform thread eagerly and can 
        
           // cause unnecessary delay in input processing. Instead, this should block 
        
           // lazily only when an operation needs the view. 
        
           // https://github.com/flutter/flutter/issues/146248 
        
           captures.latch.Wait();

I think the current logic still works with merged threads if UI tasks are executed immediately on the platform thread (as the embedder's signal will be triggered before the embedder waits on the signal). Could you confirm that's correct? Also, could you update the comments to explain the behavior when threads are merged since it won't be obvious.

cc @hbatagelo who's working on multi-window

engine/src/flutter/shell/platform/windows/flutter_windows_view.cc

knopp · 2025-02-11T01:29:19Z

Please don't review the embedder changes :) I'll rebase this PR as soon as #162944 lands (tree is red) and everything embedder related will be gone from this PR.

loic-sharma · 2025-02-11T02:17:25Z

Please don't review the embedder changes :)

Sorry about that, I didn't realize this PR was stacked on another one until after I finished reviewing this one.

knopp · 2025-02-11T12:50:50Z

Did you check if the FlutterWindowsEngine::CreateView and FlutterWindowsEngine::RemoveView need to be updated? See:

flutter/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

Lines 543 to 558 in 0be3f8f

FlutterEngineResult result = embedder_api_.AddView(engine_, &info);

if (result != kSuccess) {

FML_LOG(ERROR)

<< "Starting the add view operation failed. FlutterEngineAddView "

"returned an unexpected result: "

<< result << ". This indicates a bug in the Windows embedder.";

FML_DCHECK(false);

return nullptr;

}

// Block the platform thread until the engine has added the view.

// TODO(loicsharma): This blocks the platform thread eagerly and can

// cause unnecessary delay in input processing. Instead, this should block

// lazily only when the app does an operation which needs the view.

// https://github.com/flutter/flutter/issues/146248

captures.latch.Wait();

flutter/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc

Lines 591 to 616 in 0be3f8f

info.remove_view_callback = [](const FlutterRemoveViewResult* result) {

// This is invoked on an engine thread. If

// |FlutterRemoveViewResult.removed| is `true`, the engine guarantees the

// view won't be presented.

Captures* captures = reinterpret_cast<Captures*>(result->user_data);

captures->removed = result->removed;

captures->latch.Signal();

};

FlutterEngineResult result = embedder_api_.RemoveView(engine_, &info);

if (result != kSuccess) {

FML_LOG(ERROR) << "Starting the remove view operation failed. "

"FlutterEngineRemoveView "

"returned an unexpected result: "

<< result

<< ". This indicates a bug in the Windows embedder.";

FML_DCHECK(false);

return;

}

// Block the platform thread until the engine has removed the view.

// TODO(loicsharma): This blocks the platform thread eagerly and can

// cause unnecessary delay in input processing. Instead, this should block

// lazily only when an operation needs the view.

// https://github.com/flutter/flutter/issues/146248

captures.latch.Wait();

I think the current logic still works with merged threads if UI tasks are executed immediately on the platform thread (as the embedder's signal will be triggered before the embedder waits on the signal). Could you confirm that's correct? Also, could you update the comments to explain the behavior when threads are merged since it won't be obvious.

cc @hbatagelo who's working on multi-window

This will likely deadlock when running with merged platform threads. There might be also other things. That's why I'd first want to merge this with merged threads disabled by default. I have yet to test the canonical PR with merged threads. I'd assume there's going to be some work needed.

…utter#162935)

Manual roll Flutter from 39b4951 to 911aa75 (56 revisions) Manual roll requested by [email protected] flutter/flutter@39b4951...911aa75 2025-02-21 [email protected] Remove legacy scenec stuff from flutter_tool (flutter/flutter#163569) 2025-02-21 [email protected] [fuchsia] reorder tests in test_suites.yaml and update the comment (flutter/flutter#163799) 2025-02-21 [email protected] Roll Dart SDK from 7ceb3be8ed0a to c5e582f15b6c (2 revisions) (flutter/flutter#163844) 2025-02-21 [email protected] Roll Packages from a9b9172 to 5a721da (2 revisions) (flutter/flutter#163848) 2025-02-21 [email protected] Roll Dart SDK from ad642ee6f94b to 7ceb3be8ed0a (2 revisions) (flutter/flutter#163832) 2025-02-21 [email protected] Update how to use VSCode with `clangd` after the monorepo merge (flutter/flutter#163671) 2025-02-21 [email protected] Reland #163711 after #163780 (flutter/flutter#163812) 2025-02-21 [email protected] Fix incorrect function name in FlutterEngineRun documentation (flutter/flutter#163696) 2025-02-21 [email protected] Roll Dart SDK from 557cbf2c7da6 to ad642ee6f94b (5 revisions) (flutter/flutter#163794) 2025-02-21 [email protected] [canvaskit] Use `visualViewport.scale` to determine device pixel ratio. (flutter/flutter#163688) 2025-02-21 [email protected] [DisplayList] Migrate rendering tests and benchmarks to DL geometry (flutter/flutter#163766) 2025-02-21 [email protected] Make `releaseMode` explicit, inform `determineDevDependencies` entirely on the flag (flutter/flutter#163780) 2025-02-21 [email protected] Ensure `exclude_dev_dependencies_test` runs if `flutter_tools` changes. (flutter/flutter#163768) 2025-02-21 [email protected] WebKit should respect TextCapitalization.words (flutter/flutter#161027) 2025-02-21 [email protected] CupertinoSliverNavigationBar.search fidelity updates (flutter/flutter#163089) 2025-02-21 [email protected] feat: Add selectionColor property to selectable text (flutter/flutter#162177) 2025-02-20 [email protected] Restore `linux_android_emulator_tests` to CI. (flutter/flutter#163747) 2025-02-20 [email protected] introduce system color palette (flutter/flutter#163335) 2025-02-20 [email protected] [web] DRY up access to headers required for multi-threaded WebAssembly (flutter/flutter#163555) 2025-02-20 [email protected] fix(CupertinoDatePicker): font is inconsistent (flutter/flutter#162932) 2025-02-20 [email protected] Create Android-API-And-Related-Versions.md (flutter/flutter#163556) 2025-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Avoid implicitly setting `determineDevDependencies: true` (it's not a safe operation) (#163711)" (flutter/flutter#163762) 2025-02-20 [email protected] Revert "Create g3_bug.yml" (flutter/flutter#163757) 2025-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make Flutter version information accessible at runtime (#140783)" (flutter/flutter#163753) 2025-02-20 [email protected] Skip Xcode install on mac_mokey Android tests (flutter/flutter#163685) 2025-02-20 [email protected] Roll Dart SDK from 2cecb16348e4 to 557cbf2c7da6 (3 revisions) (flutter/flutter#163705) 2025-02-20 [email protected] Add `FlutterVersion.engineCommitDate`, helps signal engine artifact SHA issues (flutter/flutter#163652) 2025-02-20 [email protected] Avoid implicitly setting `determineDevDependencies: true` (it's not a safe operation) (flutter/flutter#163711) 2025-02-20 [email protected] It is now safe to call `getBuildMode().release` (flutter/flutter#163712) 2025-02-20 [email protected] [iOS] remove Skia interfaces from iOS platform code. (flutter/flutter#163505) 2025-02-20 [email protected] Make Flutter version information accessible at runtime (flutter/flutter#140783) 2025-02-20 [email protected] [windows] Implement merged UI and platform thread (flutter/flutter#162935) 2025-02-20 [email protected] `CircularProgressIndicator` throws null exception (flutter/flutter#163356) 2025-02-20 [email protected] Fix the issue where DropdownMenu does not gain focus when tapped. (flutter/flutter#162874) 2025-02-20 [email protected] Check for tracked engine.version before overriding (flutter/flutter#163672) 2025-02-20 [email protected] showDialogs adds a `requestFocus` parameter. (flutter/flutter#162928) 2025-02-20 [email protected] Create helper method to dispatch object creation and disposal. (flutter/flutter#163637) 2025-02-20 [email protected] Redistribute iOS TESTOWNERS (flutter/flutter#163653) 2025-02-20 [email protected] enclosingElement -> enclosingElement3 (flutter/flutter#163681) 2025-02-19 [email protected] [ Widget Preview ] Move `preview_detector_test.dart` from `general.shard` to `commands.shard` (flutter/flutter#163619) 2025-02-19 [email protected] [iOS] always post new task during gesture dispatch. (flutter/flutter#163646) 2025-02-19 [email protected] Allow flutter tools to detach a running Chrome session (flutter/flutter#163349) 2025-02-19 [email protected] Reland "[Impeller] add support for rational bezier conics to Path (#63282)" (flutter/flutter#163645) 2025-02-19 [email protected] [web:a11y] wheel events switch to pointer mode (flutter/flutter#163582) ...

#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 <[email protected]>

…utter#162935)

) Manual roll Flutter from 39b4951 to 911aa75 (56 revisions) Manual roll requested by [email protected] flutter/flutter@39b4951...911aa75 2025-02-21 [email protected] Remove legacy scenec stuff from flutter_tool (flutter/flutter#163569) 2025-02-21 [email protected] [fuchsia] reorder tests in test_suites.yaml and update the comment (flutter/flutter#163799) 2025-02-21 [email protected] Roll Dart SDK from 7ceb3be8ed0a to c5e582f15b6c (2 revisions) (flutter/flutter#163844) 2025-02-21 [email protected] Roll Packages from a9b9172 to 5a721da (2 revisions) (flutter/flutter#163848) 2025-02-21 [email protected] Roll Dart SDK from ad642ee6f94b to 7ceb3be8ed0a (2 revisions) (flutter/flutter#163832) 2025-02-21 [email protected] Update how to use VSCode with `clangd` after the monorepo merge (flutter/flutter#163671) 2025-02-21 [email protected] Reland #163711 after #163780 (flutter/flutter#163812) 2025-02-21 [email protected] Fix incorrect function name in FlutterEngineRun documentation (flutter/flutter#163696) 2025-02-21 [email protected] Roll Dart SDK from 557cbf2c7da6 to ad642ee6f94b (5 revisions) (flutter/flutter#163794) 2025-02-21 [email protected] [canvaskit] Use `visualViewport.scale` to determine device pixel ratio. (flutter/flutter#163688) 2025-02-21 [email protected] [DisplayList] Migrate rendering tests and benchmarks to DL geometry (flutter/flutter#163766) 2025-02-21 [email protected] Make `releaseMode` explicit, inform `determineDevDependencies` entirely on the flag (flutter/flutter#163780) 2025-02-21 [email protected] Ensure `exclude_dev_dependencies_test` runs if `flutter_tools` changes. (flutter/flutter#163768) 2025-02-21 [email protected] WebKit should respect TextCapitalization.words (flutter/flutter#161027) 2025-02-21 [email protected] CupertinoSliverNavigationBar.search fidelity updates (flutter/flutter#163089) 2025-02-21 [email protected] feat: Add selectionColor property to selectable text (flutter/flutter#162177) 2025-02-20 [email protected] Restore `linux_android_emulator_tests` to CI. (flutter/flutter#163747) 2025-02-20 [email protected] introduce system color palette (flutter/flutter#163335) 2025-02-20 [email protected] [web] DRY up access to headers required for multi-threaded WebAssembly (flutter/flutter#163555) 2025-02-20 [email protected] fix(CupertinoDatePicker): font is inconsistent (flutter/flutter#162932) 2025-02-20 [email protected] Create Android-API-And-Related-Versions.md (flutter/flutter#163556) 2025-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Avoid implicitly setting `determineDevDependencies: true` (it's not a safe operation) (#163711)" (flutter/flutter#163762) 2025-02-20 [email protected] Revert "Create g3_bug.yml" (flutter/flutter#163757) 2025-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make Flutter version information accessible at runtime (#140783)" (flutter/flutter#163753) 2025-02-20 [email protected] Skip Xcode install on mac_mokey Android tests (flutter/flutter#163685) 2025-02-20 [email protected] Roll Dart SDK from 2cecb16348e4 to 557cbf2c7da6 (3 revisions) (flutter/flutter#163705) 2025-02-20 [email protected] Add `FlutterVersion.engineCommitDate`, helps signal engine artifact SHA issues (flutter/flutter#163652) 2025-02-20 [email protected] Avoid implicitly setting `determineDevDependencies: true` (it's not a safe operation) (flutter/flutter#163711) 2025-02-20 [email protected] It is now safe to call `getBuildMode().release` (flutter/flutter#163712) 2025-02-20 [email protected] [iOS] remove Skia interfaces from iOS platform code. (flutter/flutter#163505) 2025-02-20 [email protected] Make Flutter version information accessible at runtime (flutter/flutter#140783) 2025-02-20 [email protected] [windows] Implement merged UI and platform thread (flutter/flutter#162935) 2025-02-20 [email protected] `CircularProgressIndicator` throws null exception (flutter/flutter#163356) 2025-02-20 [email protected] Fix the issue where DropdownMenu does not gain focus when tapped. (flutter/flutter#162874) 2025-02-20 [email protected] Check for tracked engine.version before overriding (flutter/flutter#163672) 2025-02-20 [email protected] showDialogs adds a `requestFocus` parameter. (flutter/flutter#162928) 2025-02-20 [email protected] Create helper method to dispatch object creation and disposal. (flutter/flutter#163637) 2025-02-20 [email protected] Redistribute iOS TESTOWNERS (flutter/flutter#163653) 2025-02-20 [email protected] enclosingElement -> enclosingElement3 (flutter/flutter#163681) 2025-02-19 [email protected] [ Widget Preview ] Move `preview_detector_test.dart` from `general.shard` to `commands.shard` (flutter/flutter#163619) 2025-02-19 [email protected] [iOS] always post new task during gesture dispatch. (flutter/flutter#163646) 2025-02-19 [email protected] Allow flutter tools to detach a running Chrome session (flutter/flutter#163349) 2025-02-19 [email protected] Reland "[Impeller] add support for rational bezier conics to Path (#63282)" (flutter/flutter#163645) 2025-02-19 [email protected] [web:a11y] wheel events switch to pointer mode (flutter/flutter#163582) ...

github-actions bot added engine flutter/engine related. See also e: labels. platform-windows Building on or for Windows specifically a: desktop Running on desktop labels Feb 8, 2025

knopp changed the title ~~[windows] Implement merged UI and platform thread~~ [wip] [windows] Implement merged UI and platform thread Feb 8, 2025

knopp force-pushed the windows_merged_ui_thread branch from c81c046 to d2233c3 Compare February 8, 2025 15:10

flutter deleted a comment from flutter-dashboard bot Feb 8, 2025

This comment was marked as resolved.

Sign in to view

knopp force-pushed the windows_merged_ui_thread branch 2 times, most recently from 3238d1e to dc79a94 Compare February 8, 2025 15:42

knopp changed the title ~~[wip] [windows] Implement merged UI and platform thread~~ [windows] Implement merged UI and platform thread Feb 8, 2025

knopp requested review from loic-sharma and mattkae February 8, 2025 15:43

knopp force-pushed the windows_merged_ui_thread branch from dc79a94 to 2373a96 Compare February 8, 2025 15:46

knopp mentioned this pull request Feb 8, 2025

[Embedder] Implement merged platform and UI thread #162944

Merged

9 tasks

knopp force-pushed the windows_merged_ui_thread branch from 2373a96 to 0be3f8f Compare February 10, 2025 10:49