Create root isolate asynchronously

[scutlight]

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 platform thread in order to run Shell::Setup synchronously.
The cost of Engine's constructor is mainly about the creating of root isolate. Actually, there used to be another time-consuming process, the default font manager setup, which was resolved by #18225.
Similar to #18225, this pr move the creation of root isolate out from creating Engine. After this action, the main thread blocking is quite an acceptable slice.

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

[flutter-dashboard]

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]

The engine failed to restart in one of our unit tests. Below is some useful error messages that I copied from the full log

[ RUN      ] ShellTest.MissAtMostOneFrameForIrregularInputEvents
[ERROR:flutter/shell/common/engine.cc(144)] Engine not prepare and launch isolate.
../../flutter/shell/common/shell_test.cc:73: Failure
Value of: restarted.get_future().get()
  Actual: false
Expected: true

@scutlight : I'm curious whether you've tested this locally with our shell_benchmarks, and what change this PR has brought to the BM_ShellInitialization benchmark?

CC @jason-simmons @gaaclarke @chinmaygarde to see if there's any extra concern on deferring loading the isolate.

[chinmaygarde]

This is racy for the reasons listed and uses an embedder specific API.

[chinmaygarde]

The API as originally designed expects the root isolate to be ready when the runtime controller has been created (this assumption is prevalent throughout the engine). Now, a race has been introduced with no way of telling from the caller about when the root isolate is ready (or the other ivars).

[scutlight]

Yes, it breaks the assumption but also makes sure the root isolate must be ready before running.

[scutlight]

@liyuqian : All failed test cases are resolved now, and benchmarks as follow. BM_ShellInitialization shows ~5x reduction.

before this pr:

../out/host_profile/shell_benchmarks

Run on (8 X 4023.94 MHz CPU s)
2020-08-06 20:39:41
Benchmark                                  Time           CPU Iterations
BM_ShellInitialization               2786435 ns     273327 ns       1000
BM_ShellShutdown                      852115 ns     232808 ns       2974
BM_ShellInitializationAndShutdown   46545353 ns     467147 ns        100

after this pr:

../out/host_profile/shell_benchmarks

Run on (8 X 4002.75 MHz CPU s)
2020-08-06 20:21:21
Benchmark                                  Time           CPU Iterations
BM_ShellInitialization                528979 ns     270995 ns       2568
BM_ShellShutdown                      878690 ns     234845 ns       3030
BM_ShellInitializationAndShutdown   46339905 ns     478631 ns        100

[scutlight]

Mac iOS Engine
ScenariosTests:
-[FlutterViewControllerTest testFirstFrameCallback]
failed, I'll figure it out later.

[scutlight]

Mac iOS Engine
ScenariosTests:
-[FlutterViewControllerTest testFirstFrameCallback]
failed, I'll figure it out later.

The failed test case has nothing to do with this pr, just rebase and get all checks passed.

[liyuqian]

I wonder in what situation would this !engine_ become true? It seems that we would only call GetServiceProtocolDescription after the engine_ is created.

[scutlight]

It's a bit obscure. For the case 'ShellTest.OnServiceProtocolGetSkSLsWorks', Shell is created and destroyed without running the engine. When destructing Shell, engine_ was moved in Shell::~Shell() https://github.com/flutter/engine/blob/master/shell/common/shell.cc#L395, and then comes Shell::OnRootIsolateCreated() and calls GetServiceProtocolDescription() meeting the invalid engine_.
Anyway, it is a protection better than none.

[chinmaygarde]

We reviewed this patch again and I believe this is good to go except this one case where we all preferred if it were an assert. The unit test seems to be accessing the private method because it is a friend of the shell. Now that this code has been reworked, the assumption in the test failed. Can we rework the test to not make this assumption anymore? Let's land then patch then.

[scutlight]

Appreciate and agree your comments. To keep this as an assert, I just simply change engine_ to weak_engine_ since it should run on the ui thread. For the test case, when it was called from Shell::OnRootIsolateCreated(), engine_ had been moved but not destructed and weak_engine_ was safe to refer.

[jason-simmons]

I've proposed a PR that starts an engine in ShellTest.OnServiceProtocolGetSkSLsWorks (#20801).

With that change, a workaround in Shell::GetServiceProtocolDescription will not be needed.

[scutlight]

I've patched #20801 and run the test locally without the workaround in Shell::GetServiceProtocolDescription, and the result is crash for the same reason that Shell::~Shell() is called before Shell::OnRootIsolateCreated(). I think using weak_engine_ instead of engine_ inside Shell::GetServiceProtocolDescription is more reasonable.

[jason-simmons]

Ah, now I see - the Shell::~Shell() call will happen on the platform thread and will transfer ownership of the shell's engine_ into the task queued to the UI thread that deletes the engine.

The task that calls Shell::OnRootIsolateCreated was previously queued to the UI thread when CreateShellOnPlatformThread posted the task that creates the Engine and its RuntimeController. So when Shell::OnRootIsolateCreated calls GetServiceProtocolDescription, the Engine deletion task has not yet run and it's safe to assume that weak_engine_ is still valid.

[scutlight]

Yeah, that it is.

[liyuqian]

I'm glad we figured it out. Maybe put Jason's reasoning in the comment so future contributors would understand why we chose weak_engine_ here instead of engine_.

Alternatively, there are many pieces of if (!weak_engine_) {...} or [engine = weak_engine_]{ if (engine) { ... } } code in shell.cc. So it looks very reasonable if we put a if (!weak_engine_) {...} here.

@jason-simmons : there's one more thing that I don't completely understand. Line 1641 won't return until the UI thread task to run the engine was fully completed. The shell destruction happened on line 1651 after it. The UI thread task of OnRootIsolateCreated was added at RuntimeController and Engine construction which seems to be earlier than RunEngine. So I'd expect OnRootIsolateCreated to be executed before shell destruction in line 1651?

[jason-simmons]

The crash in the unit test happened because Shell::engine_ was null even though the shell had not yet been fully destructed and the engine had not been deleted.

Shell::engine_ is set to null on the platform thread within the Shell destructor by https://github.com/flutter/engine/blob/master/shell/common/shell.cc#L395. The platform thread can do this at any time regardless of what the UI thread is doing. The platform thread will then queue a task to the UI thread that actually deletes the engine.

So it's unsafe to access Shell::engine_ directly from a UI thread task. UI thread tasks should use weak_engine_ and check if it is null.

[liyuqian]

Previously root_isolate_ was private. Now GetRootIsolate is public. I wonder if there's any specific reason to expose it publicly? Its usage seems to be still completely internal. If possible, we'd like to minimize our public API surface.

[scutlight]

GetRootIsolate() is originally public. Engine calls this API to prepare, launch isolate, and do some callbacks. But seems these processes can be withdraw back into RuntimeController and then this API can be marked private.

[liyuqian]

Ah, I missed it as I originally that GetRootIsolate wasn't there. Then it's fine to keep using the public GetRootIsolatehere.

[liyuqian]

Relying on GetRootIsolate().lock() seems to be fragile as one can easily write a root_isolate_.lock() in the future and didn't realize it can cause some issue.

Besides, even if GetRootIsolate().lock() is enforced everywhere, the async speedup may easily be broken if someone calls GetRootIsolate() or some methods that calls it in the wrong place. I believe that's why this PR moves GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); to another place.

(#18225 doesn't seem to have this problem.)

[scutlight]

Access to root_isolate_ directly may escape from creating the isolate, this fragility is a price of this pr. Since it is a private variable, the affection scope is inside RuntimeController and it might be acceptable in my personal opinion. I'll add some comments for root_isolate_ to point it out, is there any better workaround?

If someone calls GetRootIsolate(), it means the root isolate is required right now, then it is necessary to create it in a synchronous way if the isolate is not created yet.
Another reason of moving GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); is that RuntimeController should be accessed only on the UI thread, while originally it was running inside Shell::Setup() on the platform thread.

[liyuqian]

Another reason of moving GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); is that RuntimeController should be accessed only on the UI thread, while originally it was running inside Shell::Setup() on the platform thread.

Good catch, I guess we'll want to fix this regardless if we're going to adopt the bigger solution. We'll have more discussions about whether to accept the price of this pr.