Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView #29145

dnfield · 2021-10-12T23:34:14Z

This patch shows a substantial improvement influtter_gallery__start_up.

I did two runs each with and without this patch:

BEFORE:
    "engineEnterTimestampMicros": 158129422997,
    "timeToFrameworkInitMicros": 344941,
    "timeToFirstFrameRasterizedMicros": 502523,
    "timeToFirstFrameMicros": 382552,
    "timeAfterFrameworkInitMicros": 37610
    
    "engineEnterTimestampMicros": 157811156937,
    "timeToFrameworkInitMicros": 193769,
    "timeToFirstFrameRasterizedMicros": 302143,
    "timeToFirstFrameMicros": 223207,
    "timeAfterFrameworkInitMicros": 29437

AFTER:
    "engineEnterTimestampMicros": 157962907452,
    "timeToFrameworkInitMicros": 139606,
    "timeToFirstFrameRasterizedMicros": 279967,
    "timeToFirstFrameMicros": 171701,
    "timeAfterFrameworkInitMicros": 32094

    "engineEnterTimestampMicros": 158047992070,
    "timeToFrameworkInitMicros": 145351,
    "timeToFirstFrameRasterizedMicros": 273628,
    "timeToFirstFrameMicros": 187442,
    "timeAfterFrameworkInitMicros": 42091

I believe the existing tests are enough to cover the thread safety, particularly around the raster thread merging tests in shell_unittests.cc. I've added a shell_benchmark for this. Before this patch, the values locally for host_profile are:

Run on (16 X 2400 MHz CPU s)
2021-10-12 21:45:46
Benchmark                                                                Time           CPU Iterations
------------------------------------------------------------------------------------------------------
BM_ShellOnPlatformViewCreated                                      6336030 ns     117060 ns       1000

After,

Run on (16 X 2400 MHz CPU s)
2021-10-12 21:45:46
Benchmark                                                                Time           CPU Iterations
------------------------------------------------------------------------------------------------------
BM_ShellOnPlatformViewCreated                                       238989 ns     107066 ns       6398

In one internal application, the time for this call went from >130ms to <100us on the platform thread.

flutter-dashboard · 2021-10-13T04:49:38Z

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.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

dnfield · 2021-10-13T04:56:14Z

The bot will recognize benchmark changes as having a test after flutter/cocoon#1392

chinmaygarde

While it's easy to intuit why removing the latching would make this faster, this code at the intersection or application-lifecycle, shell-lifecycle, thread-merging, and, background GPU access has been the cause of significant instability in the engine in the past. For this reason, I don't think we should count on existing tests to verify correctness or count the benchmark as a test.

From the Discord thread, the following testing approach was discussed. Can we add a test for that instead?

"In shell_unittests, create a shell that calls OnPlatformViewCreated, invoke main
that calls back into native code to wait for a latch. This will block the UI thread.
Then call PlatformViewDestroyed and release the latch. In dart code,
the add a platform view and try to render a frame with it. See what happens?"

chinmaygarde · 2021-10-13T18:12:43Z

shell/common/shell_benchmarks.cc

+  SkCanvas* CompositeEmbeddedView(int view_id) override { return &canvas_; }
+
+ private:
+  SkCanvas canvas_;


Here and elsewhere: Disallow copy and assign.

chinmaygarde · 2021-10-13T18:13:46Z

shell/common/shell.cc

-                  raster_task, should_post_raster_task,
-                  &latch  //
-  ] {
+  // TODO(dnfield): This probably isn't necessary. The engine should be able to


Nit: TODO(91717) instead

chinmaygarde · 2021-10-13T18:16:33Z

shell/common/shell_benchmarks.cc

+    // snapshot.
+    fml::TaskRunner::RunNowOrPostTask(
+        thread_host->ui_thread->GetTaskRunner(),
+        []() { std::this_thread::sleep_for(std::chrono::milliseconds(5)); });


Please avoid magic timeouts.

To elaborate on how to avoid magic timeouts, you could either setup a latch that gets signaled on the NotifyDestroyed. That can be after the timing for the run has been paused if needed.

I was thinking a latch wouldn't be great here because the benchmark wouldn't even work without my change, but I guess it's fine and I have to re-order the threading anyway because of a misuse of GetPlatformView.

dnfield · 2021-10-13T21:00:00Z

Per offline conversation, updating this PR to also have the OnDestroyed call not latch the UI task runner, which shoul dbe equally safe. Will push tests and updated benchmark shortly.

dnfield · 2021-10-13T21:19:20Z

Benchmark is just giving me lots of headaches, and I'm not sure how much value it has. I've added tests.

I also removed the check about whether we need to post the raster task or not from OnDestroy. It seems completely unnecessary at this point, since we're running the task from the platform thread immediately after firing off the UI task. Nothing could interrupt us at that point to change the merge status.

dnfield · 2021-10-15T19:34:04Z

@chinmaygarde - mind giving this another review?

…formView (flutter/engine#29145)

…flutter#29145) * Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView

…flutter#29145)

…formView (flutter#29145)" This reverts commit 7d03471.

…eatePlatformView (flutter#29145)"" This reverts commit 5c16f37.

…ll::OnCreatePlatformView (flutter#29145)""" This reverts commit 810fe7a.

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView

6ea45fe

google-cla bot added the cla: yes label Oct 12, 2021

dnfield added 3 commits October 12, 2021 16:46

Merge remote-tracking branch 'upstream/master' into on_create_fast

60bf805

Cleanup

3ca204c

benchmark

9e8c02b

dnfield requested review from gaaclarke, chinmaygarde, blasten and jason-simmons October 13, 2021 04:49

dnfield marked this pull request as ready for review October 13, 2021 04:49

flutter-dashboard bot added the needs tests label Oct 13, 2021

dnfield requested a review from cbracken October 13, 2021 04:50

chinmaygarde reviewed Oct 13, 2021

View reviewed changes

tests, review, remove benchmark

ed6f483

dnfield removed the needs tests label Oct 13, 2021

dnfield requested a review from chinmaygarde October 14, 2021 21:03

chinmaygarde approved these changes Oct 18, 2021

View reviewed changes

dnfield merged commit b95484e into flutter:master Oct 18, 2021

dnfield deleted the on_create_fast branch October 18, 2021 21:46

engine-flutter-autoroll mentioned this pull request Oct 19, 2021

Roll Engine from faa45f497f92 to b95484ed0d13 (7 revisions) flutter/flutter#92057

Closed

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

b95484e Do not serialize on UI/Raster threads for Shell::OnCreatePlat…

07d355c

…formView (flutter/engine#29145)

engine-flutter-autoroll mentioned this pull request Oct 19, 2021

Roll Engine from faa45f497f92 to 910395ef686f (9 revisions) flutter/flutter#92082

Merged

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

b95484e Do not serialize on UI/Raster threads for Shell::OnCreatePlat…

8ba6c7a

…formView (flutter/engine#29145)

zanderso mentioned this pull request Oct 19, 2021

Regression in time to first frame on Mac_ios_flutter_gallery_ios__start_up flutter/flutter#92133

Open

kylinchen pushed a commit to XianyuTech/engine that referenced this pull request Oct 22, 2021

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView (…

7fce35b

…flutter#29145) * Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView

dnfield mentioned this pull request Nov 15, 2021

Explore whether providing a pbuffer surface before the SurfaceView is available helps first frame latency flutter/flutter#90835

Closed

yx-mike added a commit to yx-mike/engine that referenced this pull request Jan 18, 2022

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView (…

7d03471

…flutter#29145)

yx-mike added a commit to yx-mike/engine that referenced this pull request May 18, 2022

Revert "Do not serialize on UI/Raster threads for Shell::OnCreatePlat…

5c16f37

…formView (flutter#29145)" This reverts commit 7d03471.

yx-mike added a commit to yx-mike/engine that referenced this pull request Aug 10, 2022

Revert "Revert "Do not serialize on UI/Raster threads for Shell::OnCr…

810fe7a

…eatePlatformView (flutter#29145)"" This reverts commit 5c16f37.

yx-mike added a commit to yx-mike/engine that referenced this pull request Aug 10, 2022

Revert "Revert "Revert "Do not serialize on UI/Raster threads for She…

3fdbcdc

…ll::OnCreatePlatformView (flutter#29145)""" This reverts commit 810fe7a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView #29145

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView #29145

dnfield commented Oct 12, 2021 •

edited

Loading

flutter-dashboard bot commented Oct 13, 2021

dnfield commented Oct 13, 2021 •

edited

Loading

chinmaygarde left a comment

chinmaygarde Oct 13, 2021

dnfield Oct 13, 2021

chinmaygarde Oct 13, 2021

dnfield Oct 13, 2021

chinmaygarde Oct 13, 2021

chinmaygarde Oct 13, 2021

dnfield Oct 13, 2021

dnfield commented Oct 13, 2021

dnfield commented Oct 13, 2021

dnfield commented Oct 15, 2021

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView #29145

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView #29145

Conversation

dnfield commented Oct 12, 2021 • edited Loading

flutter-dashboard bot commented Oct 13, 2021

dnfield commented Oct 13, 2021 • edited Loading

chinmaygarde left a comment

Choose a reason for hiding this comment

chinmaygarde Oct 13, 2021

Choose a reason for hiding this comment

dnfield Oct 13, 2021

Choose a reason for hiding this comment

chinmaygarde Oct 13, 2021

Choose a reason for hiding this comment

dnfield Oct 13, 2021

Choose a reason for hiding this comment

chinmaygarde Oct 13, 2021

Choose a reason for hiding this comment

chinmaygarde Oct 13, 2021

Choose a reason for hiding this comment

dnfield Oct 13, 2021

Choose a reason for hiding this comment

dnfield commented Oct 13, 2021

dnfield commented Oct 13, 2021

dnfield commented Oct 15, 2021

dnfield commented Oct 12, 2021 •

edited

Loading

dnfield commented Oct 13, 2021 •

edited

Loading