Move Assets loading from UI thread to IO thread #22077
Conversation
|
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. |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Besides the nit about RunNowOrPostTask, you can add a test for this in the shell_unittests harness where you create a shell with a custom task runner, get its engine, call UpdateAssetManager on the engine with an asset manager you give it with a custom asset provider for the test, in the asset provider, you can assert that the task runner is the same as the one you gave to the caller to create the shell.
|
@chinmaygarde I added the test. |
format
|
@chinmaygarde looks like this PR is ready for another round |
|
@chinmaygarde @xster @gaaclarke Hi, anyone can help to review? :) |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I reviewed the code, I'll leave it up to @chinmaygarde to decide if this is a good idea. The important thing is to add an assertion to you test.
| AssetResolver::AssetResolverType type_; | ||
| }; | ||
|
|
||
| class PlatformMessageResponseTest : public flutter::PlatformMessageResponse { |
There was a problem hiding this comment.
I think both of these classes could be mocked with gmock which would be less code to maintain.
| SendEnginePlatformMessage(shell.get(), std::move(platform_message)); | ||
|
|
||
| DestroyShell(std::move(shell), std::move(task_runners)); |
| fml::RefPtr<PlatformMessageResponse> response = message->response(); | ||
| if (!response) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Seems like this early out should happen on the UI thread since it should be low cost. What line of this code is the heaviest? GetAsMapping? I'd only defer to the IO thread if we are going to hit that since posting to another thread incurs extra cost.
|
Closing this in favour of #29016. |
5 participants
Description
All Flutter assets loading(file io operations) called on UI thread, we can move them to IO thread instead.
responsestill called on UI thread.Related Issues
N/A
Tests
I added the following tests:
Not yet.
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.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.