Skip to content

Conversation

gaaclarke
Copy link
Member

I also formatted the markdown as per our guidelines. I kept the commits for formatting separate if you want to ignore those.

Pre-launch Checklist

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

@gaaclarke gaaclarke requested a review from aam August 8, 2024 15:35
@github-actions github-actions bot added c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. d: docs/ flutter/flutter/docs, for contributors labels Aug 8, 2024
@gaaclarke gaaclarke requested a review from chinmaygarde August 8, 2024 15:35
Even so, it is recommended that custom embedders set up a dedicated thread for
this task runner.

### Flutter Engine Groups
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this can be moved up into "Anatomy of Flutter app" or "Architecture overview".
Basically where "the Engine" is introduced it could be further explained that flutter app could have multiple engines(is that right? not sure what "flutter app" refers to in case of "add to app" for example), and they can be created either directly or via engine groups.
Also, it would be helpful to understand whether flutter engine group concept are limited to android and ios or are available in other embedders too(desktop linux/mac/win, custom) too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've worded all this information in terms of threading so I think it fits under the "Threading" section. I think the section above is talking about a typical single engine case which is what the majority of people will be concerned about. FlutterEngineGroups are inconsequential in that case.

Also, it would be helpful to understand whether flutter engine group concept are limited to android and ios or are available in other embedders too(desktop linux/mac/win, custom) too.

We do have links to the iOS and Android APIs. Desktop platforms are experimental so we don't want to mention those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlutterEngineGroups are inconsequential in that case.

Engine group sounds like an entity that owns and manages its engines, so it sounds like something that should be introduced at the same time the engine is introduced.
If it's not, if it's purpose is to just provide a context for engine instantiation, it probably worth clarifying.

Threading is one aspect of flutter engine group behavior, dart vm isolate group reuse is another important one(in terms of the kind of messages can be send between isolates in the engine group, gc and concurrency constraints for worker isolates in the group, requirement for all engines in the group to run from the same dart source).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not, if it's purpose is to just provide a context for engine instantiation, it probably worth clarifying.

Done, clarified at the top we are talking about one engine.

Threading is one aspect of flutter engine group behavior, dart vm isolate group reuse is another important one(in terms of the kind of messages can be send between isolates in the engine group, gc and concurrency constraints for worker isolates in the group, requirement for all engines in the group to run from the same dart source).

I added the note about sharing source code.

I don't think this is the appropriate doc to go into details about dart isolate groups. It's a dart concept that should be defined in dart documentation and we can link to it. I have a link already in the text, let me know if there is a better one.

If an engine is instantiated directly, it gets its own threads. However, all
engines instantiated with a FlutterEngineGroup will share the same threads
across that group. Similarly, engines in a group will share a [dart isolate
group](https://dart.dev/language/concurrency#performance-and-isolate-groups).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth pointing out that each engine still gets its own root isolate, but now running in the same dart isolate group with all other isolates. And all root isolates from all engines in the group are scheduled to run on the same ui thread(perhaps using the same UI Task Runner?). Spawned worker isolates are running on dart vm thread pool, on the threads that dart vm manages. There is a separate dart vm thread pool for every group.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth pointing out that each engine still gets its own root isolate, but now running in the same dart isolate group with all other isolates. And all root isolates from all engines in the group are scheduled to run on the same ui thread(perhaps using the same UI Task Runner?)

Done. Good point about mentioning the root isolates are unique but share a task runner. I didn't explain that a dart isolate group implies a thread pool since I think that's better left in the Dart documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dart isolate group implies a thread pool

Use of single dart isolate group for the whole engine group means shared thread pool for all worker isolates in the engine group, I think it's important to mention that since if affects concurrency characteristics of the multi-engine app.
It basically goes towards something perhaps worth stating explicitly: that engine group is there to improve resource use(threads, memory, spawn time) and deal with increased dependencies between engines(root isolates run on the same ui thread, dart gc is the same for all engines, worker isolates thread pool is the same for all engines).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I added some of this information, including the shared thread pool. I think finer details should be in the linked dart documentation.

@gaaclarke gaaclarke requested a review from aam August 8, 2024 16:30
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other bits that are out of date I noticed in the reformat but they are unrelated and we can fixup later. Thanks!

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2024
Copy link
Contributor

auto-submit bot commented Aug 8, 2024

auto label is removed for flutter/flutter/153100, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

gaaclarke and others added 3 commits August 12, 2024 13:10
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024
Comment on lines +278 to +280
As a result engines in a FlutterEngineGroup will have a smaller memory
footprint, faster startup latency, and better CPU core usage with shared threads
and a shared thread pool for isolates.
Copy link
Member

@loic-sharma loic-sharma Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As a result engines in a FlutterEngineGroup will have a smaller memory
footprint, faster startup latency, and better CPU core usage with shared threads
and a shared thread pool for isolates.
Thus if multiple engines are in a FlutterEngineGroup,
they will have a smaller memory footprint, faster startup latency, and better CPU core usage with shared threads
and a shared thread pool for isolates.

- A minikin derivative we call libtxt (font selection, bidi, line breaking).
- HarfBuzz (glyph selection, shaping).
- Skia (rendering/GPU back-end), which uses FreeType for font rendering on Android and Fuchsia, and CoreGraphics for font rendering on iOS.
* A minikin derivative we call libtxt (font selection, bidi, line breaking).
Copy link
Member

@loic-sharma loic-sharma Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outdated, we're using Skia paragraph now. (It's a bit out-of-scope for your PR, but you licked the cookie 😅)

@auto-submit auto-submit bot merged commit fa12167 into flutter:master Aug 12, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 13, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 13, 2024
Manual roll requested by tarrinneal@google.com

flutter/flutter@814e49b...2afc452

2024-08-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from adb1fa9fdbcf to 9054eb43aa26 (2 revisions) (flutter/flutter#153338)
2024-08-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from af68e772d298 to adb1fa9fdbcf (2 revisions) (flutter/flutter#153336)
2024-08-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 07ecd90b7755 to af68e772d298 (3 revisions) (flutter/flutter#153332)
2024-08-13 gspencergoog@users.noreply.github.com Add fake dependency on flutter_gpu for the docs (flutter/flutter#153325)
2024-08-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5d8ee52e985b to 07ecd90b7755 (7 revisions) (flutter/flutter#153326)
2024-08-12 victorsanniay@gmail.com Make CupertinoButton interactive by keyboard shortcuts (flutter/flutter#153126)
2024-08-12 30870216+gaaclarke@users.noreply.github.com Added FlutterEngineGroups to engine architecture doc (flutter/flutter#153100)
2024-08-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from bcf2dcc09a13 to 5d8ee52e985b (4 revisions) (flutter/flutter#153313)
2024-08-12 jason-simmons@users.noreply.github.com Disable DevTools when running the hot restart integration test in flutter_tools (flutter/flutter#153247)
2024-08-12 kerber.jg@gmail.com Implemented CupertinoButton new styles/sizes (fixes #92525) (flutter/flutter#152845)
2024-08-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#153297)
2024-08-12 34465683+rkishan516@users.noreply.github.com Refactor: Deprecate inactiveColor from cupertino checkbox (flutter/flutter#152981)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024
I also formatted the markdown as per our guidelines.  I kept the commits for formatting separate if you want to ignore those.
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
I also formatted the markdown as per our guidelines.  I kept the commits for formatting separate if you want to ignore those.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. d: docs/ flutter/flutter/docs, for contributors framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants