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

Conversation

@gw280
Copy link
Contributor

@gw280 gw280 commented Jan 3, 2020

Right now we are using our own Loop and Thread implementations on flutter_runner on Fuchsia. This patch migrates us to using the built-in fml types now that they work on Fuchsia (and have unit tests running on CI).

The biggest concern I have with this is the (hopefully temporary) plumbing I've added to extract the async::Loop object from the MessageLoop through the static method FuchsiaLoopForMessageLoop on MessageLoopFuchsia. This is currently necessary because we use the Fuchsia Trace component which requires the async::Loop object to initialise.

@gw280 gw280 requested review from dnfield and iskakaushik January 3, 2020 19:20
@auto-assign auto-assign bot requested a review from flar January 3, 2020 19:20
@gw280 gw280 added the CQ+1 label Jan 3, 2020
@dnfield dnfield removed the CQ+1 label Jan 3, 2020
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Most of this seems very reasonable, hoping @cbracken or @chinmaygarde can fill in some details as well.

private:
Delegate& delegate_;
const std::string thread_label_;
flutter::ThreadHost thread_host_;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a strange class to own the thread host, but that's how we had it before I guess.

Is this migration going to help us migrate Fuchsia to the embedder API eventually, or is it completely unrelated?

@cbracken

Copy link
Contributor

Choose a reason for hiding this comment

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

This change should definitely move us in the direction of migrating Fuchsia to using the embedder API.

@dnfield
Copy link
Contributor

dnfield commented Jan 3, 2020

CI failure on Fuchsia is an infra issue - bot update failed on one of the builders.

@gw280 gw280 requested a review from cbracken January 6, 2020 18:20
@gw280 gw280 force-pushed the gwright-fuchsia-loop branch from 739e190 to 110d157 Compare January 6, 2020 21:15
@gw280 gw280 force-pushed the gwright-fuchsia-loop branch from 6fa8e27 to a4408d1 Compare January 7, 2020 21:19
@gw280
Copy link
Contributor Author

gw280 commented Jan 7, 2020

Some updates:

  • Use async_get_default_dispatcher() for the trace component instead of exposing the async dispatcher object through the MessageLoopImpl
  • Switch to using the fml TaskObserver APIs instead of the old Fuchsia/flutter_runner-specific ones
  • Fix a minor issue where I was creating a new thread for the platform thread in the ThreadHost instead of using the current thread.

Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

@gw280 gw280 changed the title [WIP] Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} Jan 7, 2020
@gw280 gw280 merged commit a50f1ef into flutter:master Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
gw280 pushed a commit to gw280/engine that referenced this pull request Jan 23, 2020
gw280 pushed a commit to gw280/engine that referenced this pull request Jan 23, 2020
gw280 pushed a commit to gw280/engine that referenced this pull request Jan 23, 2020
gw280 pushed a commit to gw280/engine that referenced this pull request Jan 23, 2020
gw280 pushed a commit that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2020
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jan 25, 2020
flutter/engine@f10f03a...51a7964

git log f10f03a..51a7964 --first-parent --oneline
2020-01-24 skia-flutter-autoroll@skia.org Roll src/third_party/dart bc9348829ef8..fc3af737c759 (2 commits) (flutter/engine#15965)
2020-01-24 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Wc7e4... to 8Ns10... (flutter/engine#15964)
2020-01-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia a7e557f3e353..3ea4d5bb857d (4 commits) (flutter/engine#15963)
2020-01-24 skia-flutter-autoroll@skia.org Roll src/third_party/dart c359b5943a52..bc9348829ef8 (1 commits) (flutter/engine#15962)
2020-01-24 gw280@google.com ensure we export the various dart snapshot symbols on Fuchsia (flutter/engine#15953)
2020-01-24 skia-flutter-autoroll@skia.org Roll src/third_party/dart 3eaae5405d37..c359b5943a52 (13 commits) (flutter/engine#15959)
2020-01-24 gw280@google.com Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} (flutter/engine#15118)
2020-01-24 gw280@google.com Re-arm timer as necessary in MessageLoopFuchsia
2020-01-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia c88a3bc3f561..a7e557f3e353 (6 commits) (flutter/engine#15957)
2020-01-24 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from LAOYs... to 6_pZp... (flutter/engine#15954)
2020-01-24 zhongwuzw@qq.com Fixes FlutterCallbackInfomation leaks (flutter/engine#15089)
2020-01-24 zhongwuzw@qq.com Fixes oc leaks in platform plugin (flutter/engine#15041)
2020-01-24 jason-simmons@users.noreply.github.com Do not reset the child isolate preparer if the isolate group data already has one (flutter/engine#15952)


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

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
gw280 pushed a commit to gw280/engine that referenced this pull request Jan 31, 2020
gw280 pushed a commit that referenced this pull request Jan 31, 2020
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Apr 2, 2021
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Apr 2, 2021
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants