Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[WIP] Allow running macOS embedder with UI and platform thread merged#53747

Closed
knopp wants to merge 1 commit intoflutter:mainfrom
knopp:merged_ui_platform_thread
Closed

[WIP] Allow running macOS embedder with UI and platform thread merged#53747
knopp wants to merge 1 commit intoflutter:mainfrom
knopp:merged_ui_platform_thread

Conversation

@knopp
Copy link
Member

@knopp knopp commented Jul 7, 2024 •
edited
Loading

Original issue: flutter/flutter#150525

This is a work in progress PR for letting the macOS embedder run both with and without UI and platform thread merged.

Changes in the resize / vsync synchronization:

  • Added FlutterRunLoop class to schedule tasks Flutter tasks on main thread in a way where it is possible to only process these 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.

Regarding resize synchronization this approach should work for Windows and Linux embedders as well. On both platforms we manage the task queue internally and are able to only process only Flutter messages so that we can block the platform thread while we keep the UI task runner going.

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@knopp
Copy link
Member Author

knopp commented Jul 7, 2024 •
edited
Loading

Remaining issues to solve:

  • Tests need to be updated
  • Configure through Info.plist
  • Figure out how to flush the microtask queue

The microtask queue is currently being flushed though a MessageLoopTaskQueues observers, which is being setup in a slightly convoluted way through callbacks in DartIsolate constructor. This works for iOS and Android since the message loop for platform thread exists on those (and is compatible with platform thread because it uses CFRunLoop and ALooper) internally, but this is not the case on desktop in general (it does not exist on platform thread and even if it did, it would not be compatible on Windows and Linux).

Instead, we should post the message directly the task runner provided by the embedder, and possibly flush the microtask queue after each task internally.

cc @jason-simmons, @jonahwilliams

@knopp knopp added the Work in progress (WIP) Not ready (yet) for review! label Jul 7, 2024
@knopp knopp force-pushed the merged_ui_platform_thread branch from ba7d8dc to df02653 Compare July 7, 2024 15:07
@knopp knopp mentioned this pull request Jul 7, 2024
Merged
@knopp knopp force-pushed the merged_ui_platform_thread branch from df02653 to d7cf0e8 Compare July 7, 2024 15:09
@knopp knopp force-pushed the merged_ui_platform_thread branch from d7cf0e8 to 57a040c Compare July 9, 2024 20:08
@jonahwilliams jonahwilliams mentioned this pull request Jul 25, 2024
Closed
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jul 29, 2024

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@chinmaygarde
Copy link
Contributor

chinmaygarde commented Aug 8, 2024

Since this is blocked on the linked issue, can we close this till support for that API is added?

@knopp knopp closed this Aug 8, 2024
@knopp knopp mentioned this pull request Oct 7, 2024
Open
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cbracken cbracken Awaiting requested review from cbracken cbracken is a code owner

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees

No one assigned

Labels

affects: desktop platform-android platform-ios platform-macos Work in progress (WIP) Not ready (yet) for review!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@knopp @chinmaygarde