Re-enable ThreadChecker and fix associated failures #12257

gw280 · 2019-09-12T22:30:10Z

This patch re-enables the disabled ThreadChecker which is used by the WeakPtrFactory to ensure we access it on the correct threads. This highlighted a bunch of threading issues surrounding Shell.

This patch is not final, I'm creating the PR to:

Run CI and see where we're at
Solicit initial feedback on the fixes I've implemented.

In particular:

Grabbing the display refresh rate adds a blocking task runner call to the constructor for Shell. Ideally I'd like to be able to do this in the existing blocking task runner call in the CreateShell factory, but because that's a static it'd require some special plumbing to get the value into the class. I'm still undecided if I like this or not.
The rasterizer uses callbacks on the Shell which must be called from the GPU thread. The OnFrameRasterized callback uses a delayed task to grab the unreported frames count, by way of calling the method on a WeakPtr to the Shell. I've added a WeakPtrFactory that lives on the GPU thread which is held inside a unique_ptr, but I'm not sure if it's the way we want to go.

gw280 · 2019-09-12T23:28:38Z

I also want to evaluate how feasible it is to run the delayed task in OnFrameRasterized on the platform thread.

gw280 · 2019-09-17T21:41:57Z

I think that's all the tests passing now. Couple more things:

UIDartState::GetSkiaUnrefQueue calls into the IOManager and is called from e.g. PictureRecorder which runs on the UI thread. The IOManager needs to be accessed from the IO thread and so I've brute forced this for now by queuing a task on the IO thread and latching to wait for the return. I'm trying to think if there's a better way to do this.
I'm not convinced our test coverage is exhaustive. For example, UIDartState::GetResourceContext() also calls into the IOManager, but I don't know what thread the callsites for that method are calling from.

dnfield

This will be nice to have.

fml/memory/thread_checker.h

lib/ui/painting/image_decoder_unittests.cc

shell/common/shell.cc

shell/common/shell.h

lib/ui/ui_dart_state.cc

gw280 · 2019-09-18T17:30:48Z

This seems like a good place to start soliciting actual review feedback.

cbracken · 2019-09-23T17:59:04Z

Ping @dnfield @chinmaygarde for review.

cbracken · 2019-09-23T17:59:17Z

@gw280 looks like this may need a rebase to head.

shell/common/shell.cc

dnfield

LGTM but would defer to @chinmaygarde - particularly on whether we should be doing the RunNowOrPostTask in UIDartState::CreateGPUObject

cbracken

@chinmaygarde can you give a pass over?

While this doesn't completely eliminate the threading concerns, it does prevent new regressions and makes the remaining threading violations obvious.

cbracken · 2019-10-16T23:46:19Z

shell/common/shell.cc

+  // We are able to extract the raw pointer from the platform_view unique_ptr
+  // because we know this is guaranteed to execute before the unique_ptr
+  // goes out of scope as we will block on this lambda completing execution
+  // when calling Shell::Setup() when resolving the io_manager_future.


Add a TODO with an issue link, specifically noting that WeakPtr asserts that all derefs to happen on the creating thread and CreateResourceContext does likewise for the IO thread.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-checking-in-comments-that-ask-questions

cbracken · 2019-10-16T23:59:05Z

shell/common/shell.cc

-       io_task_runner                                //
+      [&io_manager_promise,                  //
+       &weak_io_manager_promise,             //
+       platform_view = platform_view.get(),  //


getUnsafe() now that it exists.

lib/ui/ui_dart_state.cc

cbracken · 2019-10-17T00:09:52Z

shell/common/shell.cc

+            display_refresh_rate = engine->GetDisplayRefreshRate();
+            ui_latch.Signal();
+          }));
+  ui_latch.Wait();


@chinmaygarde thoughts on how to avoid the latching here?

I don' think this that call should be on the engine to begin with. The vsync waiter has no thread affinity and there is no reason it has to accessed via the engine. I'd rather we used the getUnsafe call here with a TODO to correct vsync waiter ownership.

fml/memory/weak_ptr.h

cbracken · 2019-10-17T00:13:20Z

lib/ui/ui_dart_state.h

    auto* state = UIDartState::Current();
    FML_DCHECK(state);
    auto queue = state->GetSkiaUnrefQueue();
+


nit: Kill the additional diff here.

chinmaygarde · 2019-10-17T19:36:13Z

I think other than the TODOs and not working around the unsafe access of vsync waiter (the ownership model is just wrong in that case), this looks good to me. Can you please create a meta-bug linking all the TODOs where the unsafe access has been added? We can tackle them all in order and eventually get rid of the call.

…lutter/engine#12257)

[email protected]:flutter/engine.git/compare/508146f0defb...7a621a7 git log 508146f..7a621a7 --no-merges --oneline 2019-10-18 [email protected] Roll src/third_party/skia 20eafffd2d2f..b80d31f8cbe2 (4 commits) (flutter/engine#13226) 2019-10-18 [email protected] Roll src/third_party/skia da29d70f1a59..20eafffd2d2f (1 commits) (flutter/engine#13223) 2019-10-18 [email protected] Roll src/third_party/skia 63a387395751..da29d70f1a59 (11 commits) (flutter/engine#13221) 2019-10-18 [email protected] Roll fuchsia/sdk/core/linux-amd64 from WpvU_... to _G94w... (flutter/engine#13220) 2019-10-18 [email protected] Specify a human readable reason for an error from the embedder API. (flutter/engine#13218) 2019-10-18 [email protected] Roll fuchsia/sdk/core/mac-amd64 from _9Uy_... to KNygX... (flutter/engine#13219) 2019-10-17 [email protected] Reland ICU update to 64.2 (flutter/engine#13216) 2019-10-17 [email protected] Use `window.devicePixelRatio` in the CanvasKit backend (flutter/engine#13192) 2019-10-17 [email protected] Re-enable WeakPtr ThreadChecker and fix associated failures (flutter/engine#12257) 2019-10-17 [email protected] Re-land "Custom compositor layers must take into account the device pixel ratio." 2019-10-17 [email protected] Add trace events around custom compositor callbacks. (flutter/engine#13212) 2019-10-17 [email protected] Roll src/third_party/skia 93e853bf2b83..63a387395751 (9 commits) (flutter/engine#13208) 2019-10-17 [email protected] Roll src/third_party/dart 9b3c7f64d8..a61c775db8 (5 commits) 2019-10-17 [email protected] Document //flutter/runtime/dart_snapshot.h (flutter/engine#13196) 2019-10-17 [email protected] Revert "Custom compositor layers must take into account the device pixel ratio. (#13193)" (flutter/engine#13211) 2019-10-17 [email protected] wrap the text in text editing. This was causing a missalingment issue in textarea. (flutter/engine#13207) 2019-10-17 [email protected] Custom compositor layers must take into account the device pixel ratio. (flutter/engine#13193) 2019-10-17 [email protected] [web] Environment variable to disable felt snapshot (flutter/engine#13187) 2019-10-17 [email protected] Roll src/third_party/dart 9e636b5ab4..9b3c7f64d8 (5 commits) 2019-10-17 [email protected] Roll src/third_party/skia 0df7697235b4..93e853bf2b83 (1 commits) (flutter/engine#13205) 2019-10-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from ek5iQ... to WpvU_... (flutter/engine#13203) 2019-10-17 [email protected] Roll fuchsia/sdk/core/mac-amd64 from 6j3Gw... to _9Uy_... (flutter/engine#13202) 2019-10-17 [email protected] Roll src/third_party/skia 6a19e03047cc..0df7697235b4 (1 commits) (flutter/engine#13200) 2019-10-17 [email protected] Roll src/third_party/dart 1e3e9ee04c..9e636b5ab4 (9 commits) 2019-10-17 [email protected] Roll src/third_party/skia f29cb70281d5..6a19e03047cc (5 commits) (flutter/engine#13198) 2019-10-17 [email protected] Roll src/third_party/dart f020ce5d23..1e3e9ee04c (12 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

[email protected]:flutter/engine.git/compare/508146f0defb...7a621a7 git log 508146f..7a621a7 --no-merges --oneline 2019-10-18 [email protected] Roll src/third_party/skia 20eafffd2d2f..b80d31f8cbe2 (4 commits) (flutter/engine#13226) 2019-10-18 [email protected] Roll src/third_party/skia da29d70f1a59..20eafffd2d2f (1 commits) (flutter/engine#13223) 2019-10-18 [email protected] Roll src/third_party/skia 63a387395751..da29d70f1a59 (11 commits) (flutter/engine#13221) 2019-10-18 [email protected] Roll fuchsia/sdk/core/linux-amd64 from WpvU_... to _G94w... (flutter/engine#13220) 2019-10-18 [email protected] Specify a human readable reason for an error from the embedder API. (flutter/engine#13218) 2019-10-18 [email protected] Roll fuchsia/sdk/core/mac-amd64 from _9Uy_... to KNygX... (flutter/engine#13219) 2019-10-17 [email protected] Reland ICU update to 64.2 (flutter/engine#13216) 2019-10-17 [email protected] Use `window.devicePixelRatio` in the CanvasKit backend (flutter/engine#13192) 2019-10-17 [email protected] Re-enable WeakPtr ThreadChecker and fix associated failures (flutter/engine#12257) 2019-10-17 [email protected] Re-land "Custom compositor layers must take into account the device pixel ratio." 2019-10-17 [email protected] Add trace events around custom compositor callbacks. (flutter/engine#13212) 2019-10-17 [email protected] Roll src/third_party/skia 93e853bf2b83..63a387395751 (9 commits) (flutter/engine#13208) 2019-10-17 [email protected] Roll src/third_party/dart 9b3c7f64d8..a61c775db8 (5 commits) 2019-10-17 [email protected] Document //flutter/runtime/dart_snapshot.h (flutter/engine#13196) 2019-10-17 [email protected] Revert "Custom compositor layers must take into account the device pixel ratio. (flutter#13193)" (flutter/engine#13211) 2019-10-17 [email protected] wrap the text in text editing. This was causing a missalingment issue in textarea. (flutter/engine#13207) 2019-10-17 [email protected] Custom compositor layers must take into account the device pixel ratio. (flutter/engine#13193) 2019-10-17 [email protected] [web] Environment variable to disable felt snapshot (flutter/engine#13187) 2019-10-17 [email protected] Roll src/third_party/dart 9e636b5ab4..9b3c7f64d8 (5 commits) 2019-10-17 [email protected] Roll src/third_party/skia 0df7697235b4..93e853bf2b83 (1 commits) (flutter/engine#13205) 2019-10-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from ek5iQ... to WpvU_... (flutter/engine#13203) 2019-10-17 [email protected] Roll fuchsia/sdk/core/mac-amd64 from 6j3Gw... to _9Uy_... (flutter/engine#13202) 2019-10-17 [email protected] Roll src/third_party/skia 6a19e03047cc..0df7697235b4 (1 commits) (flutter/engine#13200) 2019-10-17 [email protected] Roll src/third_party/dart 1e3e9ee04c..9e636b5ab4 (9 commits) 2019-10-17 [email protected] Roll src/third_party/skia f29cb70281d5..6a19e03047cc (5 commits) (flutter/engine#13198) 2019-10-17 [email protected] Roll src/third_party/dart f020ce5d23..1e3e9ee04c (12 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

gw280 requested a review from chinmaygarde September 12, 2019 22:30

googlebot added the cla: yes label Sep 12, 2019

gw280 force-pushed the gwright-fml-threadchecker branch from 7bda8a2 to 804c77c Compare September 17, 2019 20:51

gw280 requested a review from dnfield September 17, 2019 21:38

gw280 added the CQ+1 label Sep 17, 2019

dnfield reviewed Sep 17, 2019

View reviewed changes

fml/memory/thread_checker.h Show resolved Hide resolved

lib/ui/painting/image_decoder_unittests.cc Show resolved Hide resolved

shell/common/shell.cc Show resolved Hide resolved

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

lib/ui/ui_dart_state.cc Outdated Show resolved Hide resolved

gw280 marked this pull request as ready for review September 18, 2019 17:30

gw280 changed the title ~~[WIP] Re-enable ThreadChecker and fix associated failures~~ Re-enable ThreadChecker and fix associated failures Sep 18, 2019

gw280 force-pushed the gwright-fml-threadchecker branch from 08f849f to 8782077 Compare September 25, 2019 21:47

dnfield reviewed Sep 30, 2019

View reviewed changes

shell/common/shell.cc Outdated Show resolved Hide resolved

gw280 force-pushed the gwright-fml-threadchecker branch 2 times, most recently from 559521c to f69f763 Compare September 30, 2019 20:41

dnfield approved these changes Sep 30, 2019

View reviewed changes

gw280 requested a review from jason-simmons September 30, 2019 21:03

gw280 requested a review from cbracken October 12, 2019 00:02

gw280 force-pushed the gwright-fml-threadchecker branch from b81404e to 5cb8a8a Compare October 16, 2019 20:28

cbracken reviewed Oct 17, 2019

View reviewed changes

George Wright and others added 7 commits October 17, 2019 13:03

Make image_decoder unittests thread safe

7befac7

unit test fixes for shell_unittest

0915013

fix all issues with shell_unittests

c671cfd

code style

99c1057

review updates

113829f

platform_view

509dd5e

review update

78664a5

George Wright added 5 commits October 17, 2019 13:03

fix input_events_unittest

af6e3e0

code formatting

29d6259

rebase fixes

6307110

fixes

317a83c

review updates

a5acfa4

gw280 force-pushed the gwright-fml-threadchecker branch from 77fc93a to a5acfa4 Compare October 17, 2019 20:15

George Wright added 2 commits October 17, 2019 13:17

small comment change

1ddcd24

forgot to save this file

d7fdbad

chinmaygarde approved these changes Oct 17, 2019

View reviewed changes

review nit

21fc25b

gw280 merged commit 8b97619 into flutter:master Oct 17, 2019

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

8b97619 Re-enable WeakPtr ThreadChecker and fix associated failures (f…

8ae6404

…lutter/engine#12257)

engine-flutter-autoroll mentioned this pull request Oct 18, 2019

Roll engine 508146f0defb..7a621a7f7e4f (26 commits) flutter/flutter#43017

Merged

jason-simmons mentioned this pull request Oct 19, 2019

Hold a reference to the Skia unref queue in UIDartState #13239

Merged

Re-enable ThreadChecker and fix associated failures #12257

Re-enable ThreadChecker and fix associated failures #12257

Uh oh!

Conversation

gw280 commented Sep 12, 2019

Uh oh!

gw280 commented Sep 12, 2019

Uh oh!

gw280 commented Sep 17, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gw280 commented Sep 18, 2019

Uh oh!

cbracken commented Sep 23, 2019

Uh oh!

cbracken commented Sep 23, 2019

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants