Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[engine] move asset mapping copy to background thread #39918

Merged
merged 9 commits into from
Mar 8, 2023
7 changes: 6 additions & 1 deletion lib/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6293,7 +6293,12 @@ class ImmutableBuffer extends NativeFieldWrapperClass1 {
final ImmutableBuffer instance = ImmutableBuffer._(0);
return _futurize((_Callback<int> callback) {
return instance._initFromAsset(encodedKey, callback);
}).then((int length) => instance.._length = length);
}).then((int length) {
if (length == -1) {
throw Exception('Asset not found');
}
return instance.._length = length;
});
}

/// Create a buffer from the file with [path].
Expand Down
64 changes: 49 additions & 15 deletions lib/ui/painting/immutable_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Dart_Handle ImmutableBuffer::init(Dart_Handle buffer_handle,
return Dart_Null();
}

Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle buffer_handle,
Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle raw_buffer_handle,
Dart_Handle asset_name_handle,
Dart_Handle callback_handle) {
UIDartState::ThrowIfUIOperationsProhibited();
Expand All @@ -62,21 +62,55 @@ Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle buffer_handle,
std::string asset_name = std::string{reinterpret_cast<const char*>(chars),
static_cast<size_t>(asset_length)};

std::shared_ptr<AssetManager> asset_manager = UIDartState::Current()
->platform_configuration()
->client()
->GetAssetManager();
std::unique_ptr<fml::Mapping> data = asset_manager->GetAsMapping(asset_name);
if (data == nullptr) {
return tonic::ToDart("Asset not found");
}
auto* dart_state = UIDartState::Current();
auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner();
auto buffer_callback =
std::make_unique<tonic::DartPersistentValue>(dart_state, callback_handle);
auto buffer_handle = std::make_unique<tonic::DartPersistentValue>(
dart_state, raw_buffer_handle);
auto asset_manager = UIDartState::Current()
->platform_configuration()
->client()
->GetAssetManager();

auto size = data->GetSize();
const void* bytes = static_cast<const void*>(data->GetMapping());
auto sk_data = MakeSkDataWithCopy(bytes, size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was scratching my head about what was being decompressed and where. Is this copy doing the decompression via faults?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_.

auto buffer = fml::MakeRefCounted<ImmutableBuffer>(sk_data);
buffer->AssociateWithDartWrapper(buffer_handle);
tonic::DartInvoke(callback_handle, {tonic::ToDart(size)});
auto ui_task = fml::MakeCopyable(
[buffer_callback = std::move(buffer_callback),
buffer_handle = std::move(buffer_handle)](const sk_sp<SkData>& sk_data,
size_t buffer_size) mutable {
auto dart_state = buffer_callback->dart_state().lock();
if (!dart_state) {
return;
}
tonic::DartState::Scope scope(dart_state);
if (!sk_data) {
// -1 is used as a sentinel that the file could not be opened.
tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(-1)});
Comment on lines +86 to +87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also just use Dart_Null() to avoid a sentinel and the tonic conversion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the receiving callback would have to be nullable and its still a sentinel value.

return;
}
auto buffer = fml::MakeRefCounted<ImmutableBuffer>(sk_data);
buffer->AssociateWithDartWrapper(buffer_handle->Get());
tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(buffer_size)});
});

dart_state->GetConcurrentTaskRunner()->PostTask(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will take a look at this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not, that PR predates us moving all of this logic to immutable_buffer

[asset_name = std::move(asset_name),
asset_manager = std::move(asset_manager),
ui_task_runner = std::move(ui_task_runner), ui_task] {
std::unique_ptr<fml::Mapping> mapping =
asset_manager->GetAsMapping(asset_name);

sk_sp<SkData> sk_data;
size_t buffer_size = 0;
if (mapping != nullptr) {
buffer_size = mapping->GetSize();
const void* bytes = static_cast<const void*>(mapping->GetMapping());
sk_data = MakeSkDataWithCopy(bytes, buffer_size);
}
ui_task_runner->PostTask(
[sk_data = std::move(sk_data), ui_task = ui_task, buffer_size]() {
ui_task(sk_data, buffer_size);
});
});
return Dart_Null();
}

Expand Down