Allow embedders to specify their own task runner interfaces. #8273

chinmaygarde · 2019-03-22T22:03:53Z

Currently, all Flutter threads are managed by the engine itself. This works for
all threads except the platform thread. On this thread, the engine cannot see
the underlying event multiplexing mechanism. Using the new task runner
interfaces, the engine can relinquish the task of setting up the event
multiplexing mechanism and instead have the embedder provide one for it during
setup.

This sceme is only wired up for the platform thread. But, the eventual goal
is to expose this message loop interoperability for all threads.

chinmaygarde · 2019-03-22T22:04:32Z

WIP as we figure out if this interface is right and the unit tests have been added.

googlebot · 2019-03-27T21:06:32Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-27T21:06:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

Currently, all Flutter threads are managed by the engine itself. This works for all threads except the platform thread. On this thread, the engine cannot see the underlying event multiplexing mechanism. Using the new task runner interfaces, the engine can relinquish the task of setting up the event multiplexing mechanism and instead have the embedder provide one for it during setup. This sceme is only wired up for the platform thread. But, the eventual goal is to expose this message loop interoperability for all threads.

googlebot · 2019-03-27T21:07:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-27T21:07:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

shell/platform/embedder/embedder.h

jason-simmons · 2019-03-27T22:05:59Z

shell/platform/embedder/embedder_task_runner.cc

+    pending_tasks_[baton] = task;
+  }
+
+  dispatch_table_.post_task_callback(this, baton, target_time);


target_time values are based on the clock used by fml::TimePoint::Now(). The embedder will need to expose an API that returns the value of that clock.

I added FlutterEngineGetCurrentTime

shell/platform/embedder/tests/embedder_unittests.cc

stuartmorgan

Note: I pretty much only looked at embedder.cc/h

stuartmorgan · 2019-03-27T22:57:41Z

shell/platform/embedder/embedder.cc

+          SAFE_ACCESS(args, custom_task_runners, nullptr));
+
+  if (!thread_host || !thread_host->IsValid()) {
+    FML_LOG(ERROR) << "Could not setup or infer thread configuration to run "


Nit: s/setup/set up/

Fixed in followup CL since this has landed.

stuartmorgan · 2019-03-27T23:02:31Z

shell/platform/embedder/embedder.h

+  size_t struct_size;
+  void* user_data;
+  // May be called from any thread. Should return true if tasks posted on the
+  // calling thread will be run on that same thread.


I had a hard time understanding how to reconcile this with the comment saying that running tasks on any thread other than the task runner's thread is undefined behavior. If the intent is that this can be used to check whether the thread it's called on is the task runner's thread, what about calling it something like is_task_runner_thread_callback?

stuartmorgan · 2019-03-27T23:03:06Z

shell/platform/embedder/embedder.h

+    void* /* user data */);
+
+// An interface used by the Flutter engine to execute tasks at the target time
+// on a specified thread. There should be a 1-1 relationship between a thread


There should be a 1-1 relationship

Is this actually true? Couldn't multiple task runners share a thread?

Not all thread configurations are known to be stable. This is an artificial restriction that we want to apply to embedder implementations for now. Once we verify that all thread configurations are stable, we can lift this restriction.

stuartmorgan · 2019-03-27T23:08:17Z

shell/platform/embedder/embedder.h

+  // embedder on the thread associated with that task runner by calling
+  // |FlutterEngineRunTask| at the given target time. The system monotonic clock
+  // should be used for the target time. The target time is the absolute time
+  // from epoch (NOT a delta) at which the task must be returned back to the


the absolute time from epoch

fml::TimePoint says it measures time "since an arbitrary point in the past"; I'm not sure you can necessarily say it's from the epoch. I didn't check all of the implementations, but arbitrary monotonic clocks could easily be using boot time, for instance.

stuartmorgan · 2019-03-27T23:15:02Z

shell/platform/embedder/embedder.h

+  size_t struct_size;
+  // Specify the task runner for the thread on which the |FlutterEngineRun| call
+  // is made.
+  const FlutterTaskRunnerDescription* platform_task_runner;


Should this be given a less generic name, if other thread/runners may be added here in the future?

I think this is fairly specific to the Flutter engine and mentioned in the Wiki and other documentation.

stuartmorgan · 2019-03-27T23:15:44Z

shell/platform/embedder/embedder.h

@@ -715,6 +766,19 @@ FlutterEngineResult FlutterEnginePostRenderThreadTask(FlutterEngine engine,
                                                      VoidCallback callback,
                                                      void* callback_data);

+// Get the current time in nanoseconds from the clock used by the flutter


s/Get/Gets/ (per Google commenting style for C++)

stuartmorgan · 2019-03-27T23:15:54Z

shell/platform/embedder/embedder.h

+FLUTTER_EXPORT
+uint64_t FlutterEngineGetCurrentTime();
+
+// Inform the engine to run the specified task. This task has been given to


…lutter/engine#8273)

flutter/engine@37947f9...c4d14a0 git log 37947f9..c4d14a0 --no-merges --oneline c4d14a0 Roll src/third_party/skia e4c67058ddb1..01a065884b7d (1 commits) (flutter/engine#8344) 5983b7a Roll src/third_party/dart ffee99d79b..cf32584870 (6 commits) 84c62b4 Build Windows shell (flutter/engine#8331) 6415277 Roll src/third_party/skia 8d2c19554e4a..e4c67058ddb1 (1 commits) (flutter/engine#8341) a011010 Roll src/third_party/dart 991c9da720..ffee99d79b (7 commits) 2098398 Cleanups to run_tests.sh script (flutter/engine#8337) 5c99138 Build GLFW from source for Linux shell (flutter/engine#8327) 6800245 Fix Windows build. (flutter/engine#8336) 6d8b836 Remove use of epoxy from Linux shell (flutter/engine#8334) 4efc321 Add super call in FLEView reshape (flutter/engine#8335) cb8eb80 Allow embedders to specify their own task runner interfaces. (flutter/engine#8273) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (bmparr@google.com), and stop the roller if necessary.

…#8273) Currently, all Flutter threads are managed by the engine itself. This works for all threads except the platform thread. On this thread, the engine cannot see the underlying event multiplexing mechanism. Using the new task runner interfaces, the engine can relinquish the task of setting up the event multiplexing mechanism and instead have the embedder provide one for it during setup. This scheme is only wired up for the platform thread. But, the eventual goal is to expose this message loop interoperability for all threads.

…lutter#8273)" This reverts commit 9518151.

chinmaygarde requested review from cbracken, GaryQian and jason-simmons March 22, 2019 22:03

googlebot added the cla: yes label Mar 22, 2019

chinmaygarde changed the title ~~Allow embedder to specify their own task runner interfaces.~~ Allow embedders to specify their own task runner interfaces. Mar 22, 2019

chinmaygarde force-pushed the embedder_task_runner branch from 4fdbb34 to f484779 Compare March 27, 2019 21:06

googlebot added cla: no and removed cla: yes labels Mar 27, 2019

chinmaygarde force-pushed the embedder_task_runner branch from f484779 to ff7dbb7 Compare March 27, 2019 21:07

googlebot added cla: yes and removed cla: no labels Mar 27, 2019

Done

bf18704

chinmaygarde requested a review from stuartmorgan March 27, 2019 21:34

jason-simmons reviewed Mar 27, 2019

View reviewed changes

chinmaygarde added 3 commits March 27, 2019 15:23

Add FlutterEngineGetCurrentTime

ec6e248

PR

9cc6822

PR

1606e83

jason-simmons approved these changes Mar 27, 2019

View reviewed changes

chinmaygarde merged commit cb8eb80 into flutter:master Mar 27, 2019

chinmaygarde deleted the embedder_task_runner branch March 27, 2019 23:17

stuartmorgan reviewed Mar 27, 2019

View reviewed changes

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2019

cb8eb80 Allow embedders to specify their own task runner interfaces. (f…

9753d59

…lutter/engine#8273)

engine-flutter-autoroll mentioned this pull request Mar 28, 2019

Roll engine 37947f961a62..c4d14a00716f (11 commits) flutter/flutter#30094

Merged

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Allow embedders to specify their own task runner interfaces. (f…

e594176

…lutter#8273)" This reverts commit 9518151.

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Allow embedders to specify their own task runner interfaces. (f…

54e979d

…lutter#8273)" This reverts commit 9518151.

chinmaygarde mentioned this pull request Apr 8, 2019

Deprecate __FlutterEngineFlushPendingTasksNow and allow embedder to create their own threads and message loops. flutter/flutter#16912

Closed

This was referenced Aug 1, 2019

Feature/event loop interop go-flutter-desktop/go-flutter#215

Closed

Feature/event loop interop go-flutter-desktop/go-flutter#218

Merged

pchampio mentioned this pull request Aug 18, 2019

Beta go-flutter-desktop/go-flutter#219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow embedders to specify their own task runner interfaces. #8273

Allow embedders to specify their own task runner interfaces. #8273

chinmaygarde commented Mar 22, 2019

chinmaygarde commented Mar 22, 2019

googlebot commented Mar 27, 2019

googlebot commented Mar 27, 2019

googlebot commented Mar 27, 2019

googlebot commented Mar 27, 2019

jason-simmons Mar 27, 2019

chinmaygarde Mar 27, 2019

stuartmorgan left a comment

stuartmorgan Mar 27, 2019

chinmaygarde Mar 27, 2019

stuartmorgan Mar 27, 2019

stuartmorgan Mar 27, 2019

chinmaygarde Mar 27, 2019

stuartmorgan Mar 27, 2019

stuartmorgan Mar 27, 2019

chinmaygarde Mar 27, 2019

stuartmorgan Mar 27, 2019

chinmaygarde Mar 27, 2019

stuartmorgan Mar 27, 2019

chinmaygarde Mar 27, 2019

Allow embedders to specify their own task runner interfaces. #8273

Allow embedders to specify their own task runner interfaces. #8273

Conversation

chinmaygarde commented Mar 22, 2019

chinmaygarde commented Mar 22, 2019

googlebot commented Mar 27, 2019

googlebot commented Mar 27, 2019

googlebot commented Mar 27, 2019

googlebot commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment