Ensure threads are merged when tearing down the Rasterizer #19919

cyanglaz · 2020-07-21T19:16:17Z

Description

This is to solve the deadlock in Shell::OnPlatformViewDestroy when dynamic thread merging is enabled.
In this solution, before we tearDown the rasterizer, we make sure the threads are merged.
We also make sure the threads are un-merged after tearing down the rasterizer, so we can re-create the surface on the raster thread.

go/flutter-thread-merging-rasterizer-teardown

TODO:
1. remove logging code.
2. add tests.

Related Issues

Fixes flutter/flutter#57067, flutter/flutter#23975

Tests

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-07-21T19:16:20Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

cyanglaz · 2020-07-21T19:18:09Z

@iskakaushik @chinmaygarde I haven't documented the new APIs in this PR nor have I added tests yet. I wanted to run this solution by you guys first. Let me know if you see obvious drawbacks in this solution.

fml/raster_thread_merger.cc

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

blasten · 2020-07-22T01:03:22Z

direction LGTM. How are you planning to test it?

cyanglaz · 2020-07-22T17:37:48Z

direction LGTM. How are you planning to test it?

I think we can test this using ShellTests. (https://github.com/flutter/engine/blob/master/shell/common/shell_test.h)
I just haven't wired it up yet as I want to get an approval of the general direction of this solution first.

fml/raster_thread_merger.cc

fml/raster_thread_merger.h

blasten · 2020-08-11T19:19:51Z

shell/common/rasterizer.cc

+      task_runners_.GetPlatformTaskRunner()) {
+    return true;
+  }
+  if (surface_ == nullptr || raster_thread_merger_.get() == nullptr) {


in what case surface_ = nullptr ?

If the rasterizer has already been torn down, the surface_ would be null. And in that case we shouldn't try to merge threads or wait the threads to be merged.

shell/common/rasterizer.cc

shell/common/shell_test_external_view_embedder.h

cyanglaz

@blasten I updated the PR with your comments.

fml/raster_thread_merger.cc

cyanglaz · 2020-08-12T03:05:40Z

shell/common/rasterizer.cc

+      task_runners_.GetPlatformTaskRunner()) {
+    return true;
+  }
+  if (surface_ == nullptr || raster_thread_merger_.get() == nullptr) {


If the rasterizer has already been torn down, the surface_ would be null. And in that case we shouldn't try to merge threads or wait the threads to be merged.

shell/common/rasterizer.cc

shell/common/rasterizer.h

shell/common/shell_test_external_view_embedder.h

blasten · 2020-08-12T16:46:40Z

EmbedderTest.PushingMutlipleFramesSetsUpNewRecordingCanvasWithoutCustomCompositor test failed in the last run

fml/raster_thread_merger.h

blasten · 2020-08-12T16:51:19Z

shell/common/rasterizer.h

+  /// @attention  If raster and platform task runners are not the same or not
+  ///             merged. This method will try to merge the task runners and
+  ///             might block the current thread and wait until the 2 task
+  ///             runners are merged.


nit: run-on sentence

shell/common/rasterizer.h

blasten

LGTM + just minor nits. Excited to give this a try!

fml/raster_thread_merger.cc

iskakaushik · 2020-08-12T17:33:22Z

fml/raster_thread_merger.cc

@@ -26,6 +32,15 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) {
    is_merged_ = task_queues_->Merge(platform_queue_id_, gpu_queue_id_);
    lease_term_ = lease_term;
  }
+  merged_condition_.notify_one();


iskakaushik · 2020-08-12T17:36:16Z

fml/raster_thread_merger.h

@@ -55,7 +67,9 @@ class RasterThreadMerger
  fml::TaskQueueId gpu_queue_id_;
  fml::RefPtr<fml::MessageLoopTaskQueues> task_queues_;
  std::atomic_int lease_term_;
-  bool is_merged_;
+  std::atomic_bool is_merged_;


can this be replaced by lease_term_ > 0

cyanglaz

@iskakaushik @blasten Updated with suggests and also added some unit tests for raster_thread_merger

fml/raster_thread_merger.cc

fml/raster_thread_merger.h

cyanglaz · 2020-08-12T17:51:16Z

fml/raster_thread_merger.h

@@ -55,7 +67,9 @@ class RasterThreadMerger
  fml::TaskQueueId gpu_queue_id_;
  fml::RefPtr<fml::MessageLoopTaskQueues> task_queues_;
  std::atomic_int lease_term_;
-  bool is_merged_;
+  std::atomic_bool is_merged_;


shell/common/rasterizer.h

cyanglaz · 2020-08-12T18:01:28Z

shell/common/rasterizer.h

+  /// @attention  If raster and platform task runners are not the same or not
+  ///             merged. This method will try to merge the task runners and
+  ///             might block the current thread and wait until the 2 task
+  ///             runners are merged.


cyanglaz · 2020-08-13T18:54:45Z

Some tests are failing because we are running raster_thread_merger on platforms that don't support it. See flutter/flutter#38844
I've created a PR to fix it #20487. We need to land 20487 in order to fix the tests here.

blasten · 2020-08-18T20:03:57Z

fml/message_loop_task_queues.cc

@@ -246,7 +246,7 @@ bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner) {
 bool MessageLoopTaskQueues::Owns(TaskQueueId owner,
                                 TaskQueueId subsumed) const {
  std::lock_guard guard(queue_mutex_);
-  return subsumed == queue_entries_.at(owner)->owner_of || owner == subsumed;
+  return subsumed == queue_entries_.at(owner)->owner_of;


nit: Would you mind adding a TODO, and filing an issue about reverting this logic once Android is fixed?

We actually might not need to revert this as I think this make sense. waiting for @iskakaushik to confirm.

Yup, this is good.

blasten · 2020-08-18T20:07:17Z

fml/raster_thread_merger_unittests.cc

-  fml::AutoResetWaitableEvent term_platform;
-  fml::AutoResetWaitableEvent latch_wait_until_merged;
-  std::thread thread_platform([&]() {
+TEST(RasterThreadMerger, HandleTaskQueuesAreTheSame) {


nit: missing checks for ASSERT_TRUE(raster_thread_merger_->TaskQueuesAreSame()), and ASSERT_FALSE(raster_thread_merger_->TaskQueuesAreSame()) in a separate TEST.

I think we should make raster_thread_merger_->TaskQueuesAreSame() private. The user of the raster_thread_merger shouldn't need to know if the merging is static or dynamic. So the test only needed to test if the threads are merged.

blasten

LGTM + some minor nits

iskakaushik

LGTM

…lutter/engine#19919)

blasten · 2020-08-26T02:37:50Z

shell/common/shell.cc

    if (engine) {
      engine->OnOutputSurfaceDestroyed();
    }
-    // Step 1: Next, tell the raster thread that its rasterizer should suspend
-    // access to the underlying surface.
-    if (should_post_raster_task) {


@cyanglaz I was thinking about this. Sorry for all these questions:

Is it possible that should_post_raster_task is true at this point even though it should be false?

Why isn't this checking a global shared state (such as RasterThreadMerger::IsMerged()) instead of a local state?

Could this be the reason why we need to merge the threads to fix the race condition?

should_post_raster_task was only exists for static thread merging. This value is always true for dynamic thread merging.

I'm not sure why it was a local var, but global state would also work.

This is the reason why we needed to merge threads here. Because thread merging happens on raster thread and this is running on platform thread, we cannot know if threads are merged.

If the platform thread is blocked at this point waiting on the latch, and the raster queue is running on the platform thread, can we release the latch and indicate that the raster should be teared down right after the latch is awaited?

Chris Yang added 2 commits July 21, 2020 11:53

merge threads when onplatformviewdestroy

f31e62f

cleanup

54c8447

cyanglaz requested a review from iskakaushik July 21, 2020 19:16

flutter-dashboard bot added the platform-ios label Jul 21, 2020

googlebot added the cla: yes label Jul 21, 2020

cyanglaz requested a review from chinmaygarde July 21, 2020 19:16

auto-assign bot requested a review from liyuqian July 21, 2020 19:19

cyanglaz requested a review from blasten July 21, 2020 19:21

cyanglaz commented Jul 21, 2020

View reviewed changes

fml/raster_thread_merger.cc Outdated Show resolved Hide resolved

blasten reviewed Jul 21, 2020

View reviewed changes

fml/raster_thread_merger.cc Outdated Show resolved Hide resolved

blasten reviewed Jul 21, 2020

View reviewed changes

fml/raster_thread_merger.cc Outdated Show resolved Hide resolved

Merge branch 'master' into sync_thread_merging

a9214c2

blasten reviewed Jul 22, 2020

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Show resolved Hide resolved

chinmaygarde reviewed Jul 27, 2020

View reviewed changes

fml/raster_thread_merger.cc Outdated Show resolved Hide resolved

chinmaygarde approved these changes Jul 27, 2020

View reviewed changes

fml/raster_thread_merger.cc Outdated Show resolved Hide resolved

fml/raster_thread_merger.h Outdated Show resolved Hide resolved

chinmaygarde self-requested a review July 27, 2020 21:17

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Jul 30, 2020

Chris Yang added 9 commits August 3, 2020 10:11

merge master

55f758d

atomic<bool> -> atomic_bool

c197572

wait until merged only on platform thread

edd4dfd

unit tests

11e014c

Merge branch 'master' into sync_thread_merging

2b8f977

cleanup

cdbee3d

fix some tests

292a647

fix typo

9863985

Rasterizer::EnsureThreadsAreMerged works with no surface

0c6e1c3

blasten reviewed Aug 11, 2020

View reviewed changes

shell/common/rasterizer.cc Show resolved Hide resolved

blasten reviewed Aug 11, 2020

View reviewed changes

shell/common/shell_test_external_view_embedder.h Outdated Show resolved Hide resolved

reviews

72049f3

cyanglaz commented Aug 12, 2020

View reviewed changes

blasten reviewed Aug 12, 2020

View reviewed changes

fml/raster_thread_merger.h Outdated Show resolved Hide resolved

blasten reviewed Aug 12, 2020

View reviewed changes

fml/raster_thread_merger.h Outdated Show resolved Hide resolved

blasten reviewed Aug 12, 2020

View reviewed changes

shell/common/rasterizer.h Outdated Show resolved Hide resolved

blasten approved these changes Aug 12, 2020

View reviewed changes

iskakaushik reviewed Aug 12, 2020

View reviewed changes

cyanglaz commented Aug 12, 2020

View reviewed changes

Chris Yang added 2 commits August 12, 2020 13:03

review & more unit tests for raster_thread_merger

bbb0210

refactor

5d60a2e

Chris Yang added 2 commits August 16, 2020 10:59

merge master

5471a38

make raster_thraed_merger handle static thread merging

f0aa0a6

blasten reviewed Aug 18, 2020

View reviewed changes

blasten approved these changes Aug 18, 2020

View reviewed changes

iskakaushik approved these changes Aug 19, 2020

View reviewed changes

cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 19, 2020

fluttergithubbot merged commit e7136c3 into flutter:master Aug 19, 2020

engine-flutter-autoroll mentioned this pull request Aug 19, 2020

Roll Engine from dcbe3d3d3dff to 81779371338a (4 revisions) flutter/flutter#64171

Merged

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

e7136c3 Ensure threads are merged when tearing down the Rasterizer (f…

f2c271e

…lutter/engine#19919)

This was referenced Aug 20, 2020

Enable external view embedder on iOS to test benchmark flutter/gallery#267

Merged

Dynamically change thread configuration when a UIView is embedded flutter/flutter#23975

Closed

blasten reviewed Aug 26, 2020

View reviewed changes

kinarobin mentioned this pull request Apr 14, 2021

Remove meaningless judgments for gpu task when raster and platform are not same thread #25583

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure threads are merged when tearing down the Rasterizer #19919

Ensure threads are merged when tearing down the Rasterizer #19919

cyanglaz commented Jul 21, 2020 •

edited

Loading

flutter-dashboard bot commented Jul 21, 2020

cyanglaz commented Jul 21, 2020

blasten commented Jul 22, 2020

cyanglaz commented Jul 22, 2020

blasten Aug 11, 2020

cyanglaz Aug 12, 2020

cyanglaz left a comment

cyanglaz Aug 12, 2020

blasten commented Aug 12, 2020

blasten Aug 12, 2020

cyanglaz Aug 12, 2020

blasten left a comment

iskakaushik Aug 12, 2020

iskakaushik Aug 12, 2020

cyanglaz Aug 12, 2020

cyanglaz left a comment

cyanglaz Aug 12, 2020

cyanglaz Aug 12, 2020

cyanglaz commented Aug 13, 2020

blasten Aug 18, 2020

cyanglaz Aug 18, 2020

iskakaushik Aug 19, 2020

blasten Aug 18, 2020

cyanglaz Aug 18, 2020

blasten left a comment

iskakaushik left a comment

blasten Aug 26, 2020

cyanglaz Aug 26, 2020

blasten Aug 26, 2020

Ensure threads are merged when tearing down the Rasterizer #19919

Ensure threads are merged when tearing down the Rasterizer #19919

Conversation

cyanglaz commented Jul 21, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

flutter-dashboard bot commented Jul 21, 2020

cyanglaz commented Jul 21, 2020

blasten commented Jul 22, 2020

cyanglaz commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten 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

cyanglaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Aug 13, 2020

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

blasten left a comment

Choose a reason for hiding this comment

iskakaushik 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

cyanglaz commented Jul 21, 2020 •

edited

Loading