Move task running to UI thread for loadDartDefferredLibrary. #35460
Conversation
Signed-off-by: xieguo <[email protected]>
|
Can you please file an issue for this and link to it here? I think this change is sane but would like to better understand the failures you're seeing without it. |
shell/common/shell.cc
Outdated
| bool transient) { | ||
| engine_->LoadDartDeferredLibraryError(loading_unit_id, error_message, | ||
| transient); | ||
| task_runners_.GetUITaskRunner()->PostTask( |
There was a problem hiding this comment.
Consider using fml::TaskRunner::RunNowOrPostTask( here and below.
shell/common/shell_unittests.cc
Outdated
| TaskRunners task_runners("test", task_runner, task_runner, task_runner, | ||
| task_runner); | ||
| auto shell = CreateShell(std::move(settings), task_runners); | ||
| auto shell = CreateShell(std::move(settings)); |
There was a problem hiding this comment.
I think we still want this test to pass as is. Instead of modifying the threading configuration here, can you add a new test?
If you use RunNowOrPostTask this test should be fine (although it may need to update some expectations below).
There was a problem hiding this comment.
I have modified based on your comments, but I still have a question for this test case, since it is not real that the four flutter threads(UI/raster/IO/Platform) run in just one thread, why not create four threads for each?
There was a problem hiding this comment.
An embedder can choose to run single threaded, and even in "normal" Flutter applications threads might get merged (although not normally the UI thread).
This reverts commit 451ad62.
Signed-off-by: xieguo <[email protected]>
Signed-off-by: xieguo <[email protected]>
|
@GaryQian can you provide a secondary review? |
dnfield
added
the
autosubmit
GaryQian
left a comment
GaryQian
left a comment
There was a problem hiding this comment.
The changes themseves lgtm, but I'm still not clear what problem this fixes. The dart loadLibrary call needs to be on UI thread, but these error messages and asset loading are already performed async and it's not clear to me why they need to be on specifically the UI thread. If there is something not working, I'd love to know where.
In any case, this shouldn't hurt, if only for consistency sake where all deferred components tasks are on the UI thread.
Yes, as Gary mentioned, no issue is encountered by us, this PR is for consistency sake. |
4 participants
func LoadDartDeferredLibraryError() and UpdateAssetResolverByType in shell.cc should be post to run in UI thread.
Fixed the issue flutter/flutter#109762
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.