Improve iOS PlatformViews to better handle thread merging. #16935

cyanglaz · 2020-03-04T18:31:43Z

This PR is a partial effort of enabling dynamic thread merging on iOS. flutter/flutter#23975

This PR mainly focuses on potential crashes after enabling thread merge. This PR DOES NOT enable thread merging and the behavior of iOS platform view and rasterizer should be the same as it is today after this. PR lands.

This PR includes:

Postpone thread merging to the very end of the frame. Introduced a EndFrame method in the ExternalViewEmbedder; updated the code in FlutterPlatformViewsController to handle that change.
Adding several main thread checks in the FlutterPlatformViewsController to help reproduce crashes related to running UIKit operations in background threads.
Add some documentations and comments for certain related methods in the FlutterPlatformViewsController.
Makes sure the [CATransaction commit] only runs on the main thread.

Edit:
ExternalViewEmbedder impls in ios_surface_gl and ios_surface_software are combined into ios_surface after this PR was put up. This PR has updated to reflect this change.

iskakaushik · 2020-03-18T20:41:36Z

flow/embedded_views.h

@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

  virtual bool SubmitFrame(GrContext* context);

+  // Caller should make sure to call this after |SubmitFrame|.


This should only be called after |SubmitFrame| reads better.

iskakaushik · 2020-03-18T20:42:15Z

flow/embedded_views.h

@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

  virtual bool SubmitFrame(GrContext* context);

+  // Caller should make sure to call this after |SubmitFrame|.
+  // Embedder that implements this method to do additional tasks after


This method provides the embedder a way to do additional tasks after |SubmitFrame|.

iskakaushik · 2020-03-18T20:42:43Z

flow/embedded_views.h

@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

  virtual bool SubmitFrame(GrContext* context);

+  // Caller should make sure to call this after |SubmitFrame|.
+  // Embedder that implements this method to do additional tasks after
+  // |SubmitFrame|. One of such examples is threading merging on iOS.


For example on the iOS embedder, ...

iskakaushik · 2020-03-18T20:43:21Z

flow/embedded_views.h

+  // Embedder that implements this method to do additional tasks after
+  // |SubmitFrame|. One of such examples is threading merging on iOS.
+  //
+  // After invoking this method, the current task on the TaskRunner should end


It will be useful to document why this is the case as well.

changed the whole paragraph a little bit, could you take a look again?

iskakaushik · 2020-03-18T20:44:05Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -184,8 +184,9 @@
    if (gpu_thread_merger->IsMerged()) {
      gpu_thread_merger->ExtendLeaseTo(kDefaultMergedLeaseDuration);
    } else {
+      // Wait until |EndFrame| to perform thread merging.


to perform thread merging -> to merge the threads.

iskakaushik · 2020-03-18T20:44:56Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -184,8 +184,9 @@
    if (gpu_thread_merger->IsMerged()) {
      gpu_thread_merger->ExtendLeaseTo(kDefaultMergedLeaseDuration);
    } else {
+      // Wait until |EndFrame| to perform thread merging.
+      will_merge_threads_ = true;


nit: merge_threads_

iskakaushik · 2020-03-18T20:45:54Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -368,6 +369,12 @@

 bool FlutterPlatformViewsController::SubmitFrame(GrContext* gr_context,
                                                 std::shared_ptr<IOSContext> ios_context) {
+  if (will_merge_threads_) {


Please add a comment explaining why these are cleared.

iskakaushik · 2020-03-18T20:46:27Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -91,6 +91,8 @@ class FlutterPlatformViewsController {

  void SetFrameSize(SkISize frame_size);

+  // Indicates that we don't compisite any platform views or overlays during this frame.
+  // Also reverts the composite_order_ to its original state at the begining of the frame.


composition_order_

iskakaushik · 2020-03-18T20:47:33Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -170,6 +177,8 @@ class FlutterPlatformViewsController {
  void OnAcceptGesture(FlutterMethodCall* call, FlutterResult& result);
  void OnRejectGesture(FlutterMethodCall* call, FlutterResult& result);

+  // Remove PlatformViews and Overlays that is in `active_composition_order_` but not


that are in, must run on platform thread.

cyanglaz

@iskakaushik Updated based on your comments.

cyanglaz · 2020-03-24T17:35:56Z

flow/embedded_views.h

@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

  virtual bool SubmitFrame(GrContext* context);

+  // Caller should make sure to call this after |SubmitFrame|.


cyanglaz · 2020-03-24T17:39:12Z

flow/embedded_views.h

@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

  virtual bool SubmitFrame(GrContext* context);

+  // Caller should make sure to call this after |SubmitFrame|.
+  // Embedder that implements this method to do additional tasks after
+  // |SubmitFrame|. One of such examples is threading merging on iOS.


cyanglaz · 2020-03-24T17:39:15Z

flow/embedded_views.h

@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

  virtual bool SubmitFrame(GrContext* context);

+  // Caller should make sure to call this after |SubmitFrame|.
+  // Embedder that implements this method to do additional tasks after


cyanglaz · 2020-03-24T17:45:56Z

flow/embedded_views.h

+  // Embedder that implements this method to do additional tasks after
+  // |SubmitFrame|. One of such examples is threading merging on iOS.
+  //
+  // After invoking this method, the current task on the TaskRunner should end


changed the whole paragraph a little bit, could you take a look again?

cyanglaz · 2020-03-24T17:47:22Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -184,8 +184,9 @@
    if (gpu_thread_merger->IsMerged()) {
      gpu_thread_merger->ExtendLeaseTo(kDefaultMergedLeaseDuration);
    } else {
+      // Wait until |EndFrame| to perform thread merging.
+      will_merge_threads_ = true;


cyanglaz · 2020-03-24T17:47:25Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -184,8 +184,9 @@
    if (gpu_thread_merger->IsMerged()) {
      gpu_thread_merger->ExtendLeaseTo(kDefaultMergedLeaseDuration);
    } else {
+      // Wait until |EndFrame| to perform thread merging.


cyanglaz · 2020-03-24T17:48:28Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -368,6 +369,12 @@

 bool FlutterPlatformViewsController::SubmitFrame(GrContext* gr_context,
                                                 std::shared_ptr<IOSContext> ios_context) {
+  if (will_merge_threads_) {


cyanglaz · 2020-03-24T17:49:04Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -91,6 +91,8 @@ class FlutterPlatformViewsController {

  void SetFrameSize(SkISize frame_size);

+  // Indicates that we don't compisite any platform views or overlays during this frame.
+  // Also reverts the composite_order_ to its original state at the begining of the frame.


blasten · 2020-03-27T23:15:17Z

@iskakaushik is there anything left in this PR that you'd like to address?

blasten · 2020-03-27T23:18:22Z

flow/embedded_views.h

+  // A new frame on the platform thread starts immediately. If the GPU thread
+  // still has some task running, there could be two frames being rendered
+  // concurrently, which causes undefined behaviors.
+  virtual void EndFrame(fml::RefPtr<fml::GpuThreadMerger> gpu_thread_merger) {}


q: The description of this method seems similar to FinishFrame. Is it possible to have a single method for both?

we currently only call EndFrame during thread merging process. It doesn't seem to be doing the same thing with we are doing in FinishFrame.

This is used particularly in the thread merging scenario. FinishFrame is actually not the very end of the frame. But the EndFrame marks the very end of the frame.

iskakaushik · 2020-03-27T23:25:25Z

@blasten Sorry, this got off my radar. I will take a look at this on Monday.

iskakaushik · 2020-04-03T22:06:08Z

can we write a test for this change?

…16935)" This reverts commit f6b8eda.

…16935)" (#17600) This reverts commit f6b8eda.

…lutter/engine#16935)

…ging. (flutter#16935)" (flutter#17600)" This reverts commit caebc93.

…lutter/engine#16935)

…6935" (#17609)

…6935)

…lutter#16935)" (flutter#17600) This reverts commit f6b8eda.

…utter#16935" (flutter#17609)

…ging. #16935" (#17609)" This reverts commit 68fd833.

Chris Yang and others added 4 commits March 2, 2020 20:05

first fix

3c1aa03

fix thread merging crash

a5a3a8b

formatting and remove testing code

4f14cf8

remove extra testing code

25a9bac

auto-assign bot requested a review from cbracken March 4, 2020 18:31

cyanglaz requested review from amirh and removed request for cbracken March 4, 2020 18:31

googlebot added the cla: yes label Mar 4, 2020

cyanglaz requested a review from iskakaushik March 4, 2020 18:31

Chris Yang and others added 7 commits March 4, 2020 10:39

remove extra stuff

d903fbe

Merge branch 'master' into thread_merging_crash

09290b4

fix dispose views race condition

89dde6e

merge master

90737aa

remove extra lines generated in merge

d7ec7f6

remove extra line

3d2efcd

catransaction commit only on main thread

64f8e9c

cyanglaz changed the title ~~Fixes crashes in iOS dynamic thread merging.~~ Improve iOS PlatformViews to better handle thread merging. Mar 13, 2020

EndFrame force check platform view controller

8015dc6

iskakaushik reviewed Mar 18, 2020

View reviewed changes

Chris Yang added 2 commits March 24, 2020 10:35

merge master

cbb3bf4

review fixes

9a914a9

cyanglaz commented Mar 24, 2020

View reviewed changes

formatting

08dfdfd

blasten reviewed Mar 27, 2020

View reviewed changes

cyanglaz mentioned this pull request Apr 3, 2020

[image_picker] fix bug, sometimes double click cancel button will crash flutter/plugins#2625

Merged

13 tasks

Chris Yang added 2 commits April 3, 2020 16:06

merge master

61dab20

add test view embedder

3bba821

cyanglaz added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels Apr 8, 2020

Chris Yang added 3 commits April 8, 2020 13:59

Merge branch 'master' into thread_merging_crash

c20864f

add external view embedder in fuschia tests

bf067ab

fix complier error

c23e5f7

cyanglaz merged commit f6b8eda into flutter:master Apr 9, 2020

cyanglaz deleted the thread_merging_crash branch April 9, 2020 00:33

cyanglaz pushed a commit that referenced this pull request Apr 9, 2020

Revert "Improve iOS PlatformViews to better handle thread merging. (#…

5d742da

…16935)" This reverts commit f6b8eda.

cyanglaz mentioned this pull request Apr 9, 2020

Revert "Improve iOS PlatformViews to better handle thread merging." #17600

Merged

cyanglaz pushed a commit that referenced this pull request Apr 9, 2020

Revert "Improve iOS PlatformViews to better handle thread merging. (#…

caebc93

…16935)" (#17600) This reverts commit f6b8eda.

engine-flutter-autoroll mentioned this pull request Apr 9, 2020

Roll engine 5b4b1f33c6d6..caebc93e0c4b (23 commits) flutter/flutter#54338

Closed

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

f6b8eda Improve iOS PlatformViews to better handle thread merging. (f…

a27c1b8

…lutter/engine#16935)

engine-flutter-autoroll mentioned this pull request Apr 9, 2020

Roll engine 5b4b1f33c6d6..916f014d1cfb (24 commits) flutter/flutter#54373

Merged

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

f6b8eda Improve iOS PlatformViews to better handle thread merging. (f…

9d67aad

…lutter/engine#16935)

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Apr 9, 2020

Revert "Revert "Improve iOS PlatformViews to better handle thread mer…

ddbe00a

…ging. (flutter#16935)" (flutter#17600)" This reverts commit caebc93.

cyanglaz mentioned this pull request Apr 9, 2020

Reland "Improve iOS PlatformViews to better handle thread merging. #16935" #17609

Merged

engine-flutter-autoroll mentioned this pull request Apr 9, 2020

Roll engine 5b4b1f33c6d6..e47980d17086 (27 commits) flutter/flutter#54393

Closed

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

f6b8eda Improve iOS PlatformViews to better handle thread merging. (f…

905d3ba

…lutter/engine#16935)

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

f6b8eda Improve iOS PlatformViews to better handle thread merging. (f…

a5929a1

…lutter/engine#16935)

This was referenced Apr 9, 2020

Roll engine 5b4b1f33c6d6..27ec81851eda (28 commits) flutter/flutter#54398

Closed

Roll engine 5b4b1f33c6d6..2a8625791a29 (29 commits) flutter/flutter#54406

Closed

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

f6b8eda Improve iOS PlatformViews to better handle thread merging. (f…

50f7dfa

…lutter/engine#16935)

engine-flutter-autoroll mentioned this pull request Apr 10, 2020

Roll engine 5b4b1f33c6d6..9101b63f9872 (30 commits) flutter/flutter#54409

Merged

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

f6b8eda Improve iOS PlatformViews to better handle thread merging. (f…

329b341

…lutter/engine#16935)

fluttergithubbot pushed a commit that referenced this pull request Apr 10, 2020

Reland "Improve iOS PlatformViews to better handle thread merging. #1…

68fd833

…6935" (#17609)

goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020

Improve iOS PlatformViews to better handle thread merging. (flutter#1…

2f0d4c9

…6935)

goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020

Revert "Improve iOS PlatformViews to better handle thread merging. (f…

37357f1

…lutter#16935)" (flutter#17600) This reverts commit f6b8eda.

goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020

Reland "Improve iOS PlatformViews to better handle thread merging. fl…

d30e23a

…utter#16935" (flutter#17609)

cyanglaz restored the thread_merging_crash branch April 28, 2020 20:42

cyanglaz pushed a commit that referenced this pull request May 1, 2020

Revert "Reland "Improve iOS PlatformViews to better handle thread mer…

93d587c

…ging. #16935" (#17609)" This reverts commit 68fd833.

		@@ -250,6 +250,14 @@ class ExternalViewEmbedder {

		virtual bool SubmitFrame(GrContext* context);

		// Caller should make sure to call this after \|SubmitFrame\|.

Improve iOS PlatformViews to better handle thread merging. #16935

Improve iOS PlatformViews to better handle thread merging. #16935

Uh oh!

Conversation

cyanglaz commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten commented Mar 27, 2020

Uh oh!

blasten Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Mar 27, 2020

Uh oh!

iskakaushik commented Apr 3, 2020

Uh oh!

Uh oh!

cyanglaz commented Mar 4, 2020 •

edited

Loading

blasten Mar 27, 2020 •

edited

Loading