[engine] move asset mapping copy to background thread #39918
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 (don't just cc him here, he won't see it! He's on Discord!). 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. |
|
|
||
| auto size = data->GetSize(); | ||
| const void* bytes = static_cast<const void*>(data->GetMapping()); | ||
| auto sk_data = MakeSkDataWithCopy(bytes, size); |
There was a problem hiding this comment.
I was scratching my head about what was being decompressed and where. Is this copy doing the decompression via faults?
There was a problem hiding this comment.
oh, I don't know why I called this decompression.
We need to open the file and then copy it into an SkData (due to potential lack of thread-safeness of asset mappings_.
jonahwilliams
changed the title
| // -1 is used as a sentinel that the file could not be opened. | ||
| tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(-1)}); |
There was a problem hiding this comment.
You could also just use Dart_Null() to avoid a sentinel and the tonic conversion.
There was a problem hiding this comment.
Then the receiving callback would have to be nullable and its still a sentinel value.
| tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(buffer_size)}); | ||
| }); | ||
|
|
||
| dart_state->GetConcurrentTaskRunner()->PostTask( |
There was a problem hiding this comment.
nit: this isn't the IO thread either, this is the worker pool.
There should really be some kind of test for this. https://github.com/flutter/engine/pull/29016/files was a similar attempt that had a test.
There was a problem hiding this comment.
will take a look at this
There was a problem hiding this comment.
is there perhaps a better way to do this without mocking out so much functionality? I don't actually care that its done on a background worker either, just that its non blocking.
There was a problem hiding this comment.
I spent some more time looking at this and I'm still scratching my head a bit. I can't mock/override things on the engine to implement this easily since immutable buffer itself is mostly using UIDartState.
@dnfield any ideas?
There was a problem hiding this comment.
I thought you could basically use the test from the PR I linked.
It doesn't assert the copy is happening on the worker thread, but it does assert that assets are opened on the worker thread.
There was a problem hiding this comment.
I could not, that PR predates us moving all of this logic to immutable_buffer
jonahwilliams
changed the title
|
From Triage: Only the AAssetManager backed asset managers have thread safety issues. We should just ask the managers about their thread safety characteristics so that we can elide the copy where we can. I suspect everything except Android can. |
|
tested! |
|
tests worked locally but I was not building unopt, will double check again |
shell/common/shell_unittests.cc
Outdated
| EXPECT_EQ(leaked_resolver->mapping_requests[0], "DoesNotExist"); | ||
|
|
||
| PlatformViewNotifyDestroyed(shell.get()); | ||
| DestroyShell(std::move(shell)); |
There was a problem hiding this comment.
I think you want DestroyShell(std::move(shell), task_runners) - the unopt crash is suggesting the shell is getting destroyed on the wrong thread.
There was a problem hiding this comment.
Thank you for saving me hours of debugging
…ne into move_decompress_to_io
jonahwilliams
added
the
autosubmit
…" (#40147) Revert "[engine] move asset mapping copy to background thread"
Fixes flutter/flutter#121525
Fixes flutter/flutter#121351
Move asset open/copy to a background thread
See timeline trace: