Allow embedders to schedule a callback on all engine managed threads. #15980

chinmaygarde · 2020-01-24T23:48:24Z

FlutterEnginePostCallbackOnAllNativeThreads schedule a callback to be run on
all engine managed threads. The engine will attempt to service this callback the
next time the message loops for each managed thread is idle. Since the engine
manages the entire lifecycle of multiple threads, there is no opportunity for
the embedders to finely tune the priorities of threads directly, or, perform
other thread specific configuration (for example, setting thread names for
tracing). This callback gives embedders a chance to affect such tuning.

Fixes flutter/flutter#49551
Fixes b/143774406
Fixes b/148278215
Fixes b/148278931

chinmaygarde · 2020-01-24T23:51:06Z

cc @dnicoara

shell/platform/embedder/embedder.h

jason-simmons · 2020-01-25T00:03:27Z

shell/platform/embedder/embedder.h

+/// @param[in]  user_data  A baton passed by the engine to the callback. This
+///                        baton is not interpreted by the engine in any way.
+///
+/// @return     The flutter export.


Change this to "The result of the call"

cbracken · 2020-01-25T00:03:49Z

shell/platform/embedder/embedder.cc

@@ -1747,3 +1746,27 @@ FlutterEngineResult FlutterEngineNotifyLowMemoryWarning(
                   kInternalInconsistency,
                   "Could not dispatch the low memory notification message.");
 }
+
+FlutterEngineResult FlutterEnginePostCallbackOnAllNativeThreads(


Question -- I didn't look through the two bugs immediately before reviewing but what's the rationale for posting to all threads and having the user potentially do something conditionally on the type param in the closure rather than having this take a thread-type and posting only to the specified task runner? Is the use-case such that you want to post on all or none?

Ah I see -- so we can provide some guarantee of synchronicity, looks like.

The use case requires posting to all threads. But, I can see your approach as being viable as well. Posting to specific threads repeatedly is not a use case that we want to handle. This is supposed to be called once very early on in engine lifecycle.

Should we be warning people not to attempt to do any cross-thread latching in the closures they pass in?

cbracken

LGTM.

chinmaygarde · 2020-01-25T00:55:14Z

hmm, I just realized you cannot post a callback from a callback. I’ll make that safe and test it before committing this.

dnicoara · 2020-01-25T14:28:48Z

Wondering, would this be sufficient to cover corner cases? Would it be possible for the embedder to call FlutterEnginePostCallbackOnAllNativeThreads before all the threads are created? With respect to worker threads, would it be possible for new worker threads to be spawned at arbitrary points in time?

chinmaygarde · 2020-01-25T18:28:27Z

Would it be possible for the embedder to call FlutterEnginePostCallbackOnAllNativeThreads before all the threads are created?

No. If you have an engine ref, this is guaranteed to work.

With respect to worker threads, would it be possible for new worker threads to be spawned at arbitrary points in time?

The number of worker threads are constant at this time. If/when this changes, the engine can hold onto the callback till the engine instance is collected. Maybe I should document that the callback may be executed a long time into the future and the embedder should not associate any handles with the baton with the expectation that all callbacks will be made in a timely manner?

`FlutterEnginePostCallbackOnAllNativeThreads` schedule a callback to be run on all engine managed threads. The engine will attempt to service this callback the next time the message loops for each managed thread is idle. Since the engine manages the entire lifecycle of multiple threads, there is no opportunity for the embedders to finely tune the priorities of threads directly, or, perform other thread specific configuration (for example, setting thread names for tracing). This callback gives embedders a chance to affect such tuning. Fixes b/143774406 Fixes b/148278215 Fixes b/148278931

chinmaygarde · 2020-01-25T21:07:20Z

I have patched and tested the re-entrancy issue.

…troying the engine.

chinmaygarde · 2020-01-25T22:19:58Z

The Windows presubmit failure is MSVC "fatal error C1001: An internal error has occurred in the compiler.". Will try to simplify the statement.

dnicoara · 2020-01-27T14:09:29Z

Awesome, thank you for quelling my concerns!

…threads. (flutter/engine#15980)

flutter/engine@c4229bf...ec32966 git log c4229bf..ec32966 --first-parent --oneline 2020-01-28 [email protected] Revert "Always make gpu thread different from platform thread regardless of platform view (#16068)" (flutter/engine#16161) 2020-01-28 [email protected] Roll fuchsia/sdk/core/mac-amd64 from gNitp... to 5fMtM... (flutter/engine#16153) 2020-01-28 [email protected] Revert "Disable setting a library tag handler." (flutter/engine#16157) 2020-01-28 [email protected] Always make gpu thread different from platform thread regardless of platform view (flutter/engine#16068) 2020-01-28 [email protected] Add test to ensure that concurrent message loops have at least one workers. (flutter/engine#16074) 2020-01-28 [email protected] Revert breaking PRs (flutter/engine#16148) 2020-01-28 [email protected] Roll fuchsia/sdk/core/linux-amd64 from 8Ns10... to 2rLoq... (flutter/engine#15971) 2020-01-28 [email protected] Gets the DPI for all awareness mode and older Windows versions (flutter/engine#15951) 2020-01-28 [email protected] Hold a mutex when updating all CanPostTaskToAllNativeThreads::Captures members. (flutter/engine#16085) 2020-01-28 [email protected] Disable setting a library tag handler. (flutter/engine#16086) 2020-01-28 [email protected] Web PargraphStyle TextHeightBehavior integration (flutter/engine#16075) 2020-01-28 [email protected] Fix flake by making thread ID tracking in CanPostTaskToAllNativeThreads thread safe. (flutter/engine#16081) 2020-01-28 [email protected] Remove buggy assertion in EmbedderTest::CanPostTaskToAllNativeThreads. (flutter/engine#16071) 2020-01-27 [email protected] Remove tonic/platform. (flutter/engine#16062) 2020-01-27 [email protected] Allow embedders to schedule a callback on all engine managed threads. (flutter/engine#15980) 2020-01-27 [email protected] Engine/LibTxt/dart:ui impl of TextHeightBehavior (flutter/engine#15087) 2020-01-27 [email protected] Roll src/third_party/skia f1b2b42613cb..9c1d30dd163e (8 commits) (flutter/engine#16060) 2020-01-27 [email protected] Remove stale recipe changelog. (flutter/engine#15985) 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 [email protected] 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

…flutter#15980) `FlutterEnginePostCallbackOnAllNativeThreads` schedule a callback to be run on all engine managed threads. The engine will attempt to service this callback the next time the message loops for each managed thread is idle. Since the engine manages the entire lifecycle of multiple threads, there is no opportunity for the embedders to finely tune the priorities of threads directly, or, perform other thread specific configuration (for example, setting thread names for tracing). This callback gives embedders a chance to affect such tuning. Fixes flutter/flutter#49551 Fixes b/143774406 Fixes b/148278215 Fixes b/148278931

…threads. (flutter#15980)" This reverts commit 3e7a4ca.

chinmaygarde requested review from cbracken and jason-simmons January 24, 2020 23:48

googlebot added the cla: yes label Jan 24, 2020

auto-assign bot requested a review from franciscojma86 January 24, 2020 23:48

cbracken reviewed Jan 24, 2020

View reviewed changes

shell/platform/embedder/embedder.h Outdated Show resolved Hide resolved

cbracken reviewed Jan 24, 2020

View reviewed changes

shell/platform/embedder/embedder.h Outdated Show resolved Hide resolved

cbracken reviewed Jan 24, 2020

View reviewed changes

shell/platform/embedder/embedder.h Outdated Show resolved Hide resolved

jason-simmons reviewed Jan 25, 2020

View reviewed changes

cbracken reviewed Jan 25, 2020

View reviewed changes

cbracken approved these changes Jan 25, 2020

View reviewed changes

chinmaygarde force-pushed the thread_prio branch from 2260fef to 68b829d Compare January 25, 2020 21:06

Fix CanPostTaskToAllNativeThreadsRecursively to make it safe when des…

609787d

…troying the engine.

chinmaygarde mentioned this pull request Jan 27, 2020

Performance of <elided application> scrolling is < 30FPS P10 flutter/flutter#49551

Closed

Fix Windows

50df695

chinmaygarde merged commit c5329ef into flutter:master Jan 27, 2020

chinmaygarde deleted the thread_prio branch January 27, 2020 21:49

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

c5329ef Allow embedders to schedule a callback on all engine managed …

3c891c7

…threads. (flutter/engine#15980)

This was referenced Jan 28, 2020

Roll engine c4229bfbbae4..b4fc4b5ab343 (12 commits) flutter/flutter#49630

Closed

Roll engine c4229bfbbae4..b4fc4b5ab343 (12 commits) flutter/flutter#49632

Closed

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

c5329ef Allow embedders to schedule a callback on all engine managed …

2b824d6

…threads. (flutter/engine#15980)

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

c5329ef Allow embedders to schedule a callback on all engine managed …

2674c61

…threads. (flutter/engine#15980)

engine-flutter-autoroll mentioned this pull request Jan 28, 2020

Roll engine c4229bfbbae4..b4fc4b5ab343 (12 commits) flutter/flutter#49636

Closed

engine-flutter-autoroll mentioned this pull request Jan 29, 2020

Roll engine c4229bfbbae4..53baa7aee535 (13 commits) flutter/flutter#49653

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 29, 2020

c5329ef Allow embedders to schedule a callback on all engine managed …

25327ec

…threads. (flutter/engine#15980)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 29, 2020

c5329ef Allow embedders to schedule a callback on all engine managed …

b1a7e0e

…threads. (flutter/engine#15980)

engine-flutter-autoroll mentioned this pull request Jan 29, 2020

Roll engine c4229bfbbae4..53baa7aee535 (13 commits) flutter/flutter#49655

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 29, 2020

c5329ef Allow embedders to schedule a callback on all engine managed …

aaa6897

…threads. (flutter/engine#15980)

engine-flutter-autoroll mentioned this pull request Jan 29, 2020

Roll engine c4229bfbbae4..ec32966e030a (18 commits) flutter/flutter#49656

Merged

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "Allow embedders to schedule a callback on all engine managed …

fd58a2f

…threads. (flutter#15980)" This reverts commit 3e7a4ca.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow embedders to schedule a callback on all engine managed threads. #15980

Allow embedders to schedule a callback on all engine managed threads. #15980

chinmaygarde commented Jan 24, 2020 •

edited

Loading

chinmaygarde commented Jan 24, 2020

jason-simmons Jan 25, 2020

chinmaygarde Jan 25, 2020

cbracken Jan 25, 2020

cbracken Jan 25, 2020

chinmaygarde Jan 25, 2020

cbracken Jan 25, 2020

cbracken left a comment

chinmaygarde commented Jan 25, 2020

dnicoara commented Jan 25, 2020

chinmaygarde commented Jan 25, 2020

chinmaygarde commented Jan 25, 2020

chinmaygarde commented Jan 25, 2020

dnicoara commented Jan 27, 2020

Allow embedders to schedule a callback on all engine managed threads. #15980

Allow embedders to schedule a callback on all engine managed threads. #15980

Conversation

chinmaygarde commented Jan 24, 2020 • edited Loading

chinmaygarde commented Jan 24, 2020

jason-simmons Jan 25, 2020

Choose a reason for hiding this comment

chinmaygarde Jan 25, 2020

Choose a reason for hiding this comment

cbracken Jan 25, 2020

Choose a reason for hiding this comment

cbracken Jan 25, 2020

Choose a reason for hiding this comment

chinmaygarde Jan 25, 2020

Choose a reason for hiding this comment

cbracken Jan 25, 2020

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

chinmaygarde commented Jan 25, 2020

dnicoara commented Jan 25, 2020

chinmaygarde commented Jan 25, 2020

chinmaygarde commented Jan 25, 2020

chinmaygarde commented Jan 25, 2020

dnicoara commented Jan 27, 2020

chinmaygarde commented Jan 24, 2020 •

edited

Loading