Make `Shell::Create` not blocking platform thread by asynchronously setup shell subsystems #19641

scutlight · 2020-07-10T08:56:16Z

Description

As the related issue refer, the application may be doing too much work on its main thread even in a simple hello_world demo.
That is because the creation of engine on the UI thread takes a noticeable time, and it is blocking the main thread in order to run Shell::Setup synchronously.
This PR breaks the main thread blocking, by introducing std::async to wait for the shell subsystems creation in a background thread. Shell::Setup will be executed when the subsystems are ready. Before shell is setup, calls to shell subsystem will be pushed into a pending tasks queue which would be run in sequence once the setup is done.
Code changes are mainly inside Shell::CreateShellOnPlatformThread and Shell::Setup.

Related Issues

flutter/flutter#40563 could be resolved by this PR.

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A
change in behaviour with no test covering it will likely get reverted
accidentally sooner or later. PRs must include tests for all
changed/updated/fixed behaviors. See testing the engine for instructions on
writing and running engine 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

…etup shell subsystems.

fluttergithubbot · 2020-07-10T08:56:19Z

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.

liyuqian

This idea looks very interesting to me. From users' perspective, this PR is great as it brings async setup to them in a completely transparent way and there's no extra work for them to receive the benefit. On the other hand, I'm a little concerned by how many pieces of code like https://github.com/scutlight/engine/blob/b791479e926f75c883c0d93251fe780acf69e30f/shell/common/shell.cc#L660-L671 need to be injected not just now, but also for every new API added to Shell in the future.

@gaaclarke @chinmaygarde @jason-simmons : what's your thought on this? Shall we give Shell users some extra work if they want async setup so we can leave most Shell functions intact (#18047), or shall we give no extra work to Shell users but modify most Shell functions for async support (this PR)?

liyuqian · 2020-07-23T00:12:16Z

shell/common/shell.cc

@@ -61,16 +61,18 @@ std::unique_ptr<Shell> Shell::CreateShellOnPlatformThread(
  std::promise<fml::WeakPtr<SnapshotDelegate>> snapshot_delegate_promise;


If we're going to land this, Shell::CreateShellOnPlatformThread should be properly documented by stating that the Shell returned may not be ready yet due to asynchronous setup but all functions can be called as normal because we'll defer them in a queue until the shell is set up.

Sure thing, only if we've reach agreement on this asynchronous setup idea. And there are many failed test cases to be updated due to asynchronous setup.

gaaclarke · 2020-07-27T22:49:10Z

shell/common/shell.cc

+  // creation and then setup the shell.
+  auto result = std::async(
+      std::launch::async,
+      [shell = shell.get(), platform_view = std::move(platform_view),


Sending a raw pointer shell to an async operation is not going to cut it. You'll need a shared_ptr.

I'm not quite sure whether I've caught you. What's the meaning of 'cut it'? Do you mean that shell might have been destructed while executing the async operation? In that case, shall I use shell = shell->weak_factory_.GetWeakPtr() instead?

Sorry, "not going to cut it" is an english idiom that means "isn't going to be adequate".

Yes, the shell could be deleted before this operation or during this operation. A weak pointer isn't going to be good enough. In the case they are happening at the same time you'd need to block the deleting thread until this has completed. The easiest solution is using a shared_ptr.

gaaclarke · 2020-07-27T22:51:57Z

shell/common/shell.h

@@ -391,6 +391,8 @@ class Shell final : public PlatformView::Delegate,
                     >
      service_protocol_handlers_;
  bool is_setup_ = false;
+  std::future<bool> async_waiting_subsystems_;
+  std::queue<std::future<void>> pending_tasks_before_setup_;


You are editing and reading this queue from multiple threads, but it's not a threadsafe datastructure.

pending_tasks_before_setup_ is only access on platform thread, it does not need to be thread safe.

I read through the code and I think you are right. It would be nice to enforce this.

gaaclarke · 2020-07-27T23:04:52Z

I think there is some promise in this approach since it is taking advantage of the async design of RunEngine. I'm not too happy with that design but this doesn't make things worse.

This particular implementation, as it is today, is perilous. I've added some comments on how to make it safe.

scutlight · 2020-07-28T10:24:51Z

@liyuqian @gaaclarke : Thanks for your reviews. This pr really brings some extra maintain works to shell APIs. I confess that I'm hesitating. But it is the only way I found to solve the synchronous blocking issue. As my individual opinion, I would keep it simple for users.

scutlight · 2020-07-30T06:39:17Z

An inspiration rushed into my mind when I woke up at midnight 🤪 .
Decomposing the synchronous Shell::Create is such a complicate work, why not just let it be. I make another solution to reduce the main thread blocking to be transient. Please also review that pr #20142 @gaaclarke @liyuqian

liyuqian · 2020-08-31T21:39:50Z

@scutlight : I guess this is going to be replaced by #20142?

scutlight · 2020-09-01T02:26:26Z

@liyuqian : Yes. With the changes of #18225 and #20142, creating Shell shall not blocking the main thread for an unacceptable waiting. Close this one.

Make Shell::Create not blocking platform thread by asynchronously s…

b791479

…etup shell subsystems.

googlebot added the cla: yes label Jul 10, 2020

auto-assign bot requested a review from liyuqian July 10, 2020 08:58

scutlight mentioned this pull request Jul 10, 2020

future: support engine create shell with aysnc mode #18047

Closed

liyuqian added the severe: performance Relates to speed or footprint issues. label Jul 13, 2020

liyuqian reviewed Jul 23, 2020

View reviewed changes

liyuqian added the perf: speed Performance issues related to (mostly rendering) speed label Jul 23, 2020

gaaclarke reviewed Jul 27, 2020

View reviewed changes

scutlight requested a review from gaaclarke July 28, 2020 10:00

lybeen mentioned this pull request Aug 12, 2020

support init engine asynchronous #20433

Closed

8 tasks

scutlight closed this Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Shell::Create` not blocking platform thread by asynchronously setup shell subsystems #19641

Make `Shell::Create` not blocking platform thread by asynchronously setup shell subsystems #19641

scutlight commented Jul 10, 2020

fluttergithubbot commented Jul 10, 2020

liyuqian left a comment

liyuqian Jul 23, 2020

scutlight Jul 28, 2020

gaaclarke Jul 27, 2020

scutlight Jul 28, 2020

gaaclarke Jul 29, 2020

gaaclarke Jul 27, 2020

scutlight Jul 28, 2020

gaaclarke Jul 29, 2020

gaaclarke commented Jul 27, 2020

scutlight commented Jul 28, 2020

scutlight commented Jul 30, 2020

liyuqian commented Aug 31, 2020

scutlight commented Sep 1, 2020

		@@ -61,16 +61,18 @@ std::unique_ptr<Shell> Shell::CreateShellOnPlatformThread(
		std::promise<fml::WeakPtr<SnapshotDelegate>> snapshot_delegate_promise;

Make Shell::Create not blocking platform thread by asynchronously setup shell subsystems #19641

Make Shell::Create not blocking platform thread by asynchronously setup shell subsystems #19641

Conversation

scutlight commented Jul 10, 2020

Description

Related Issues

Tests

Checklist

Breaking Change

fluttergithubbot commented Jul 10, 2020

liyuqian 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

gaaclarke commented Jul 27, 2020

scutlight commented Jul 28, 2020

scutlight commented Jul 30, 2020

liyuqian commented Aug 31, 2020

scutlight commented Sep 1, 2020

Make `Shell::Create` not blocking platform thread by asynchronously setup shell subsystems #19641

Make `Shell::Create` not blocking platform thread by asynchronously setup shell subsystems #19641