Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Image.asset will block UI thread when load large image #58572 #18808

Closed
wants to merge 2 commits into from
Closed

Image.asset will block UI thread when load large image #58572 #18808

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19a7899
Image.asset will block UI thread when load large image #58572
zljj0818 Jun 4, 2020
b077e7b
Image.asset will block UI thread when load large image #58572 clear f…
zljj0818 Jun 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions lib/ui/window/platform_message_response_dart.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,40 @@ namespace {
// Avoid copying the contents of messages beyond a certain size.
const int kMessageCopyThreshold = 1000;

class DataWrapper {
Copy link
Contributor

@liyuqian liyuqian Jun 4, 2020

Choose a reason for hiding this comment

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

I think this DataWrapper would be unnecessary if we just use ToByteData's implementation inside WrapByteData:

Dart_Handle WrapByteData(std::unique_ptr<fml::Mapping> mapping) {
  Dart_Handle data_handle =
      Dart_NewTypedData(Dart_TypedData_kByteData, mapping->GetSize());
  if (Dart_IsError(data_handle))
    return data_handle;

  Dart_TypedData_Type type;
  void* data = nullptr;
  intptr_t num_bytes = 0;
  FML_CHECK(!Dart_IsError(
      Dart_TypedDataAcquireData(data_handle, &type, &data, &num_bytes)));

  memcpy(data, buffer.data(), num_bytes);
  Dart_TypedDataReleaseData(data_handle);
  return data_handle;
}

Copy link
Contributor Author

@zljj0818 zljj0818 Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I intend to optimze according to orignal code. The origial code if if (data.size() < kMessageCopyThreshold) , it will use dart vm heap to allocate data, else it use application heap to alloate data, dart vm only use DataWrapper pointer.
If use ToByteData will consume more dart vm heap.

public:
DataWrapper(void* data) { data_ = data; }

~DataWrapper() { free(data_); }

private:
void* data_;
};

void MessageDataFinalizer(void* isolate_callback_data,
Dart_WeakPersistentHandle handle,
void* peer) {
std::vector<uint8_t>* data = reinterpret_cast<std::vector<uint8_t>*>(peer);
DataWrapper* data = reinterpret_cast<DataWrapper*>(peer);
delete data;
}

Dart_Handle WrapByteData(std::vector<uint8_t> data) {
if (data.size() < kMessageCopyThreshold) {
Dart_Handle WrapByteData(std::unique_ptr<fml::Mapping> mapping) {
size_t size = mapping->GetSize();
if (size < kMessageCopyThreshold) {
Copy link
Contributor

@liyuqian liyuqian Jun 4, 2020

Choose a reason for hiding this comment

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

This if branch is unnecessary as we're always calling memcpy: whether inside ToByteData, or inside the else branch, or inside the old WrapByteData(std::unique_ptr<fml::Mapping> mapping) function.

std::vector<uint8_t> data(size);
memcpy(data.data(), mapping->GetMapping(), size);
return ToByteData(data);
} else {
std::vector<uint8_t>* heap_data = new std::vector<uint8_t>(std::move(data));
return Dart_NewExternalTypedDataWithFinalizer(
Dart_TypedData_kByteData, heap_data->data(), heap_data->size(),
heap_data, heap_data->size(), MessageDataFinalizer);
void* data = malloc(size);
memset(data, 0, size);
Copy link
Contributor

@liyuqian liyuqian Jun 4, 2020

Choose a reason for hiding this comment

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

The memset seems to be unnecessary?

Copy link
Contributor Author

@zljj0818 zljj0818 Jun 4, 2020

Choose a reason for hiding this comment

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

It would be nice to add a microbenchmark to test PlatformMessageResponseDart with a 3MB asset. See, e.g., the benchmark in https://github.com/flutter/engine/blob/master/shell/common/shell_benchmarks.cc

PlatformMessageResponseDart only post a ui task, and this ui task will consume time when execute

Copy link
Contributor Author

@zljj0818 zljj0818 Jun 4, 2020

Choose a reason for hiding this comment

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

The memset seems to be unnecessary?

Yes, just according to the logical of std::vector

memcpy(data, mapping->GetMapping(), size);
DataWrapper* wrapper = new DataWrapper(data);
return Dart_NewExternalTypedDataWithFinalizer(Dart_TypedData_kByteData,
data, size, wrapper, size,
MessageDataFinalizer);
}
}

Dart_Handle WrapByteData(std::unique_ptr<fml::Mapping> mapping) {
std::vector<uint8_t> data(mapping->GetSize());
memcpy(data.data(), mapping->GetMapping(), mapping->GetSize());
return WrapByteData(std::move(data));
}

} // anonymous namespace

PlatformMessageResponseDart::PlatformMessageResponseDart(
Expand Down