Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[macOS] Implement merged UI and platform thread #162883

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[macOS] Implement merged UI and platform thread #162883

wants to merge 2 commits into from
+673 −1,071

Conversation

knopp
Copy link
Member

@knopp knopp commented Feb 7, 2025 •
edited
Loading

Original issue: #150525

This PR lets the macOS embedder run both with and without UI and platform thread merged.

Thread merging is controlled through FLTEnableMergedPlatformUIThread info.plist option similar to iOS embedder, though the default value is currently false.

Changes in the resize / vsync synchronization:

  • Added FlutterRunLoop class to schedule Flutter tasks on main thread in a way where it is possible to only process these (Flutter posted) tasks while waiting for correct frame size during resizing. This significantly simplifies the resize synchronization and makes the same code work both with separate UI thread and with UI and platform thread merged.
  • FlutterThreadSynchronizer has been renamed to FlutterResizeSynchronizer vastly simplified, mutex and conditions are removed and the blocking is now done by only processing Flutter messages while waiting for resizing. It is now per view (instead of storing a viewId->Size map internally) and owned by the view itself, instead of engine.
  • This approach to resize synchronization will work for Windows and Linux as well. This will allow us to conceptually consolidate the way we do threading and resize synchronization on all three desktop embedders.

Pre-launch Checklist

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

@github-actions github-actions bot added engine flutter/engine repository. See also e: labels. a: desktop Running on desktop platform-macos labels Feb 7, 2025
@knopp knopp force-pushed the macos_merged_ui_thread branch from 6dc1b3e to 4f80954 Compare February 7, 2025 17:58
@munrocket
Copy link

Awesome! Making own graphic API with FFI on macOS is impossible without this. We need to land it ASAP.

@knopp knopp force-pushed the macos_merged_ui_thread branch 2 times, most recently from dc3f160 to f585e97 Compare February 8, 2025 15:41
@knopp knopp mentioned this pull request Feb 8, 2025
Merged
9 tasks
@knopp knopp force-pushed the macos_merged_ui_thread branch 3 times, most recently from eb3c3b5 to fbcd706 Compare February 11, 2025 14:23
@knopp knopp requested a review from cbracken February 11, 2025 14:29
@knopp knopp force-pushed the macos_merged_ui_thread branch from fbcd706 to 582cba0 Compare February 13, 2025 10:31
@knopp knopp changed the title [macOS] Implement merged UI and platform thread WIP: [macOS] Implement merged UI and platform thread Feb 25, 2025
@knopp
Copy link
Member Author

knopp commented Feb 25, 2025 •
edited
Loading

Putting this as WIP. With this PR with unmerged thread, CVDisplayLink is started on UI thread and stopped on platform thread. I initially assumed that would work, but it doesn't. During resizing the CVDisplayLink sometimes creates two threads and trying to stop it deadlocks.

CVDisplayLink API documentation doesn't make any threading guarantees, I assumed it was thread safe but it doesn't seem to be the case.

@knopp knopp changed the title WIP: [macOS] Implement merged UI and platform thread [macOS] Implement merged UI and platform thread Feb 26, 2025
@knopp
Copy link
Member Author

knopp commented Feb 26, 2025

I've simplified display link / vsync waiter threading model. Now everything is expected to happen on main thread. This requires one extra hop from UI to platform thread with unmerged threads, and zero extra hops when running with merged threads (which will become default).

The mental overhead of trying to keep everything thread safe was probably not really worth it with unmerged threads and is definitely not worth it with merged threads. Also while not documented anywhere, CVDisplayLink seems to break when start and stop are called from different threads (despite having an internal mutex). This is no longer an issue since all start / stop calls are serialized and performed on main thread.

@knopp knopp force-pushed the macos_merged_ui_thread branch from 582cba0 to 6a3e7b0 Compare February 26, 2025 12:44
@johnmccutchan
Copy link
Contributor

@knopp This is so great!

@knopp knopp force-pushed the macos_merged_ui_thread branch from 6a3e7b0 to 067d3ec Compare February 28, 2025 23:24
@knopp knopp force-pushed the macos_merged_ui_thread branch from 067d3ec to ceb43c1 Compare March 11, 2025 13:00
@loic-sharma loic-sharma requested a review from dkwingsmt March 18, 2025 17:33
@loic-sharma
Copy link
Member

cc @dkwingsmt as you've also worked on the macOS embedder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbracken cbracken Awaiting requested review from cbracken

@dkwingsmt dkwingsmt Awaiting requested review from dkwingsmt

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
a: desktop Running on desktop engine flutter/engine repository. See also e: labels. platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knopp @munrocket @johnmccutchan @loic-sharma