From e7c9865a88835af20ca5610e8d5e57fb448d159b Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 13 Jun 2022 16:24:09 -0700 Subject: [PATCH 1/3] Eliminates an extra copy when returning messages from the host platform to dart. --- assets/BUILD.gn | 12 ++++++ assets/directory_asset_bundle.cc | 23 ++++++++++- assets/directory_asset_bundle_unittests.cc | 40 +++++++++++++++++++ ci/licenses_golden/licenses_flutter | 1 + fml/mapping.cc | 6 ++- fml/mapping.h | 31 ++++++++++++-- fml/platform/posix/mapping_posix.cc | 16 +++++++- fml/platform/win/mapping_win.cc | 16 +++++++- .../window/platform_message_response_dart.cc | 27 +++++++++++-- runtime/BUILD.gn | 1 + shell/platform/android/apk_asset_provider.cc | 2 + .../darwin/common/buffer_conversions.mm | 9 +++++ .../flutter/file_in_namespace_buffer.h | 3 ++ 13 files changed, 174 insertions(+), 13 deletions(-) create mode 100644 assets/directory_asset_bundle_unittests.cc diff --git a/assets/BUILD.gn b/assets/BUILD.gn index eefdaf5eb02c7..d8e6b3f665a1a 100644 --- a/assets/BUILD.gn +++ b/assets/BUILD.gn @@ -18,3 +18,15 @@ source_set("assets") { public_configs = [ "//flutter:config" ] } + +source_set("assets_unittests") { + sources = [ "directory_asset_bundle_unittests.cc" ] + + deps = [ + ":assets", + "//flutter/testing", + ] + + public_configs = [ "//flutter:config" ] + testonly = true +} diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index 7896756614593..15abfec56b8db 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -50,8 +50,27 @@ std::unique_ptr DirectoryAssetBundle::GetAsMapping( return nullptr; } - auto mapping = std::make_unique(fml::OpenFile( - descriptor_, asset_name.c_str(), false, fml::FilePermission::kRead)); +// TODO(tbd): Implement this optimization for Fuchsia. +// TODO(tbd): Implement this optimization for Windows. +#if defined(OS_FUCHSIA) || defined(FML_OS_WIN) + auto mapping = std::make_unique( + fml::OpenFile(descriptor_, asset_name.c_str(), false, + fml::FilePermission::kRead), + std::initializer_list( + {fml::FileMapping::Protection::kRead}), + std::initializer_list()); +#else + // We use a copy-on-write mapping so we can safely pass ownership to Dart + // which currently lacks immutable data buffers. + auto mapping = std::make_unique( + fml::OpenFile(descriptor_, asset_name.c_str(), false, + fml::FilePermission::kRead), + std::initializer_list( + {fml::FileMapping::Protection::kRead, + fml::FileMapping::Protection::kWrite}), + std::initializer_list( + {fml::FileMapping::Flags::kCopyOnWrite})); +#endif if (!mapping->IsValid()) { return nullptr; diff --git a/assets/directory_asset_bundle_unittests.cc b/assets/directory_asset_bundle_unittests.cc new file mode 100644 index 0000000000000..dbb2564d55e4d --- /dev/null +++ b/assets/directory_asset_bundle_unittests.cc @@ -0,0 +1,40 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/assets/directory_asset_bundle.h" +#include "gtest/gtest.h" + +namespace flutter { +namespace testing { + +TEST(DirectoryAssetBundle, MappingIsReadWrite) { + fml::ScopedTemporaryDirectory temp_dir; + const char* filename = "foo.txt"; + fml::MallocMapping write_mapping(static_cast(calloc(1, 4)), 4); + fml::WriteAtomically(temp_dir.fd(), filename, write_mapping); + fml::UniqueFD descriptor = + fml::OpenDirectory(temp_dir.path().c_str(), /*create_if_necessary=*/false, + fml::FilePermission::kRead); + std::unique_ptr bundle = + std::make_unique( + std::move(descriptor), /*is_valid_after_asset_manager_change=*/true); + EXPECT_TRUE(bundle->IsValid()); + std::unique_ptr read_mapping = bundle->GetAsMapping(filename); + ASSERT_TRUE(read_mapping); + +#if defined(OS_FUCHSIA) || defined(FML_OS_WIN) + ASSERT_FALSE(read_mapping->GetMutableMapping()); +#else + ASSERT_TRUE(read_mapping->GetMutableMapping()); + EXPECT_EQ(read_mapping->GetSize(), 4u); + read_mapping->GetMutableMapping()[0] = 'A'; + EXPECT_EQ(read_mapping->GetMapping()[0], 'A'); + std::unique_ptr read_after_write_mapping = + bundle->GetAsMapping(filename); + EXPECT_EQ(read_after_write_mapping->GetMapping()[0], '\0'); +#endif +} + +} // namespace testing +} // namespace flutter diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 2c1b9c9718ef6..d1a982587bb3e 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -20,6 +20,7 @@ FILE: ../../../flutter/assets/asset_manager.h FILE: ../../../flutter/assets/asset_resolver.h FILE: ../../../flutter/assets/directory_asset_bundle.cc FILE: ../../../flutter/assets/directory_asset_bundle.h +FILE: ../../../flutter/assets/directory_asset_bundle_unittests.cc FILE: ../../../flutter/benchmarking/benchmarking.cc FILE: ../../../flutter/benchmarking/benchmarking.h FILE: ../../../flutter/benchmarking/library.cc diff --git a/fml/mapping.cc b/fml/mapping.cc index 0c2815f9705dc..91788745dfe24 100644 --- a/fml/mapping.cc +++ b/fml/mapping.cc @@ -11,7 +11,7 @@ namespace fml { // FileMapping -uint8_t* FileMapping::GetMutableMapping() { +uint8_t* FileMapping::GetMutableMapping() const { return mutable_mapping_; } @@ -146,6 +146,10 @@ const uint8_t* MallocMapping::GetMapping() const { return data_; } +uint8_t* MallocMapping::GetMutableMapping() const { + return data_; +} + bool MallocMapping::IsDontNeedSafe() const { return false; } diff --git a/fml/mapping.h b/fml/mapping.h index 3bf50841cd130..4c845c9fd1b1d 100644 --- a/fml/mapping.h +++ b/fml/mapping.h @@ -28,6 +28,10 @@ class Mapping { virtual const uint8_t* GetMapping() const = 0; + /// Returns a non-const pointer to the data if one is available, otherwise it + /// returns nullptr. + virtual uint8_t* GetMutableMapping() const = 0; + // Whether calling madvise(DONTNEED) on the mapping is non-destructive. // Generally true for file-mapped memory and false for anonymous memory. virtual bool IsDontNeedSafe() const = 0; @@ -44,9 +48,15 @@ class FileMapping final : public Mapping { kExecute, }; - explicit FileMapping(const fml::UniqueFD& fd, - std::initializer_list protection = { - Protection::kRead}); + enum class Flags { + kNone, + kCopyOnWrite, + }; + + explicit FileMapping( + const fml::UniqueFD& fd, + std::initializer_list protection = {Protection::kRead}, + std::initializer_list flags = {Flags::kNone}); ~FileMapping() override; @@ -72,7 +82,8 @@ class FileMapping final : public Mapping { // |Mapping| bool IsDontNeedSafe() const override; - uint8_t* GetMutableMapping(); + // |Mapping| + uint8_t* GetMutableMapping() const override; bool IsValid() const; @@ -103,6 +114,9 @@ class DataMapping final : public Mapping { // |Mapping| const uint8_t* GetMapping() const override; + // |Mapping| + uint8_t* GetMutableMapping() const override { return nullptr; } + // |Mapping| bool IsDontNeedSafe() const override; @@ -128,6 +142,9 @@ class NonOwnedMapping final : public Mapping { // |Mapping| const uint8_t* GetMapping() const override; + // |Mapping| + uint8_t* GetMutableMapping() const override { return nullptr; } + // |Mapping| bool IsDontNeedSafe() const override; @@ -177,6 +194,9 @@ class MallocMapping final : public Mapping { // |Mapping| const uint8_t* GetMapping() const override; + // |Mapping| + uint8_t* GetMutableMapping() const override; + // |Mapping| bool IsDontNeedSafe() const override; @@ -204,6 +224,9 @@ class SymbolMapping final : public Mapping { // |Mapping| const uint8_t* GetMapping() const override; + // |Mapping| + uint8_t* GetMutableMapping() const override { return nullptr; } + // |Mapping| bool IsDontNeedSafe() const override; diff --git a/fml/platform/posix/mapping_posix.cc b/fml/platform/posix/mapping_posix.cc index 02351a00d629a..c926bd4be48d2 100644 --- a/fml/platform/posix/mapping_posix.cc +++ b/fml/platform/posix/mapping_posix.cc @@ -46,12 +46,23 @@ static bool IsWritable( return false; } +static bool IsCopyOnWrite( + const std::initializer_list& flags) { + for (auto flag : flags) { + if (flag == FileMapping::Flags::kCopyOnWrite) { + return true; + } + } + return false; +} + Mapping::Mapping() = default; Mapping::~Mapping() = default; FileMapping::FileMapping(const fml::UniqueFD& handle, - std::initializer_list protection) { + std::initializer_list protection, + std::initializer_list flags) { if (!handle.is_valid()) { return; } @@ -71,7 +82,8 @@ FileMapping::FileMapping(const fml::UniqueFD& handle, auto* mapping = ::mmap(nullptr, stat_buffer.st_size, ToPosixProtectionFlags(protection), - is_writable ? MAP_SHARED : MAP_PRIVATE, handle.get(), 0); + is_writable && !IsCopyOnWrite(flags) ? MAP_SHARED : MAP_PRIVATE, + handle.get(), 0); if (mapping == MAP_FAILED) { return; diff --git a/fml/platform/win/mapping_win.cc b/fml/platform/win/mapping_win.cc index 6225a9c6d0311..6c6d30d584769 100644 --- a/fml/platform/win/mapping_win.cc +++ b/fml/platform/win/mapping_win.cc @@ -40,8 +40,19 @@ static bool IsExecutable( return false; } +static bool IsCopyOnWrite( + const std::initializer_list& flags) { + for (auto flag : flags) { + if (flag == FileMapping::Flags::kCopyOnWrite) { + return true; + } + } + return false; +} + FileMapping::FileMapping(const fml::UniqueFD& fd, - std::initializer_list protections) + std::initializer_list protections, + std::initializer_list flags) : size_(0), mapping_(nullptr) { if (!fd.is_valid()) { return; @@ -82,6 +93,9 @@ FileMapping::FileMapping(const fml::UniqueFD& fd, return; } + // TODO(tbd): Implement copy-on-write semantics for Windows. It should + // involve using the `FILE_MAP_COPY` flag with [MapViewOfFile]. + FML_DCHECK(!IsCopyOnWrite(flags)); const DWORD desired_access = read_only ? FILE_MAP_READ : FILE_MAP_WRITE; auto mapping = reinterpret_cast( diff --git a/lib/ui/window/platform_message_response_dart.cc b/lib/ui/window/platform_message_response_dart.cc index 39d55f41d736b..a6b19d04e9efd 100644 --- a/lib/ui/window/platform_message_response_dart.cc +++ b/lib/ui/window/platform_message_response_dart.cc @@ -47,6 +47,12 @@ void PostCompletion(Callback&& callback, } } // namespace +namespace { +void MappingFinalizer(void* isolate_callback_data, void* peer) { + delete static_cast(peer); +} +} // anonymous namespace + PlatformMessageResponseDart::PlatformMessageResponseDart( tonic::DartPersistentValue callback, fml::RefPtr ui_task_runner, @@ -64,9 +70,24 @@ PlatformMessageResponseDart::~PlatformMessageResponseDart() { void PlatformMessageResponseDart::Complete(std::unique_ptr data) { PostCompletion(std::move(callback_), ui_task_runner_, &is_complete_, channel_, - [data = std::move(data)] { - return tonic::DartByteData::Create(data->GetMapping(), - data->GetSize()); + [data = std::move(data)]() mutable { + void* mapping = data->GetMutableMapping(); + Dart_Handle byte_buffer; + if (mapping) { + size_t data_size = data->GetSize(); + byte_buffer = Dart_NewExternalTypedDataWithFinalizer( + /*type=*/Dart_TypedData_kByteData, + /*data=*/mapping, + /*length=*/data_size, + /*peer=*/data.release(), + /*external_allocation_size=*/data_size, + /*callback=*/MappingFinalizer); + + } else { + byte_buffer = tonic::DartByteData::Create( + data->GetMapping(), data->GetSize()); + } + return byte_buffer; }); } diff --git a/runtime/BUILD.gn b/runtime/BUILD.gn index 48ef5882eca4b..9666e1f9f5b78 100644 --- a/runtime/BUILD.gn +++ b/runtime/BUILD.gn @@ -142,6 +142,7 @@ if (enable_unittests) { ":libdart", ":runtime", ":runtime_fixtures", + "//flutter/assets:assets_unittests", "//flutter/common", "//flutter/fml", "//flutter/lib/snapshot", diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc index f0670fa84801d..6a98cd379c949 100644 --- a/shell/platform/android/apk_asset_provider.cc +++ b/shell/platform/android/apk_asset_provider.cc @@ -25,6 +25,8 @@ class APKAssetMapping : public fml::Mapping { return reinterpret_cast(AAsset_getBuffer(asset_)); } + uint8_t* GetMutableMapping() const override { return nullptr; } + bool IsDontNeedSafe() const override { return !AAsset_isAllocated(asset_); } private: diff --git a/shell/platform/darwin/common/buffer_conversions.mm b/shell/platform/darwin/common/buffer_conversions.mm index 6ba9b1e4a53ab..134be5a4bba59 100644 --- a/shell/platform/darwin/common/buffer_conversions.mm +++ b/shell/platform/darwin/common/buffer_conversions.mm @@ -18,6 +18,15 @@ explicit NSDataMapping(NSData* data) : data_([data retain]) {} return static_cast([data_.get() bytes]); } + uint8_t* GetMutableMapping() const override { + // This is safe because the NSData instances we are wrapping all live in RAM + // and are short lived objects that live for the purpose of responding to + // messages. We could convert the API to use NSMutableData but that is a + // breaking change and allocating a NSMutableData is much slower than NSData + // unfortunately. + return const_cast(static_cast([data_.get() bytes])); + } + bool IsDontNeedSafe() const override { return false; } private: diff --git a/shell/platform/fuchsia/flutter/file_in_namespace_buffer.h b/shell/platform/fuchsia/flutter/file_in_namespace_buffer.h index 2a075edbd0f94..c57a9bf9e7dce 100644 --- a/shell/platform/fuchsia/flutter/file_in_namespace_buffer.h +++ b/shell/platform/fuchsia/flutter/file_in_namespace_buffer.h @@ -22,6 +22,9 @@ class FileInNamespaceBuffer final : public fml::Mapping { // |fml::Mapping| const uint8_t* GetMapping() const override; + // |fml::Mapping| + uint8_t* GetMutableMapping() const override { return nullptr; } + // |fml::Mapping| size_t GetSize() const override; From dece935a6784ed7961a64449f989e6fd9a0faf15 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 17 Jun 2022 11:09:11 -0700 Subject: [PATCH 2/3] removed iOS --- shell/platform/darwin/common/buffer_conversions.mm | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/shell/platform/darwin/common/buffer_conversions.mm b/shell/platform/darwin/common/buffer_conversions.mm index 134be5a4bba59..4cc77515cdf92 100644 --- a/shell/platform/darwin/common/buffer_conversions.mm +++ b/shell/platform/darwin/common/buffer_conversions.mm @@ -19,12 +19,12 @@ explicit NSDataMapping(NSData* data) : data_([data retain]) {} } uint8_t* GetMutableMapping() const override { - // This is safe because the NSData instances we are wrapping all live in RAM - // and are short lived objects that live for the purpose of responding to - // messages. We could convert the API to use NSMutableData but that is a - // breaking change and allocating a NSMutableData is much slower than NSData - // unfortunately. - return const_cast(static_cast([data_.get() bytes])); + // TODO(tbd): Implement this so ownership can be passed to Dart. + // Previously this was attempted with a const_cast of the mapping. That is + // safe in most cases since the data is ephemeral and we are the creaters + // and the sole owners of the data. That is not the case when the + // BinaryCodec is used however. + return nullptr; } bool IsDontNeedSafe() const override { return false; } From ac5c3e77c39d9726ec6c91d60ba1fae73283e11e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 17 Jun 2022 11:26:59 -0700 Subject: [PATCH 3/3] jonahs feedback - faster smaller messages --- .../window/platform_message_response_dart.cc | 40 ++++++++++--------- .../tonic/typed_data/dart_byte_data.cc | 10 ++--- third_party/tonic/typed_data/dart_byte_data.h | 1 + 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/lib/ui/window/platform_message_response_dart.cc b/lib/ui/window/platform_message_response_dart.cc index a6b19d04e9efd..c1551cd1ef0b9 100644 --- a/lib/ui/window/platform_message_response_dart.cc +++ b/lib/ui/window/platform_message_response_dart.cc @@ -69,26 +69,28 @@ PlatformMessageResponseDart::~PlatformMessageResponseDart() { } void PlatformMessageResponseDart::Complete(std::unique_ptr data) { - PostCompletion(std::move(callback_), ui_task_runner_, &is_complete_, channel_, - [data = std::move(data)]() mutable { - void* mapping = data->GetMutableMapping(); - Dart_Handle byte_buffer; - if (mapping) { - size_t data_size = data->GetSize(); - byte_buffer = Dart_NewExternalTypedDataWithFinalizer( - /*type=*/Dart_TypedData_kByteData, - /*data=*/mapping, - /*length=*/data_size, - /*peer=*/data.release(), - /*external_allocation_size=*/data_size, - /*callback=*/MappingFinalizer); + PostCompletion( + std::move(callback_), ui_task_runner_, &is_complete_, channel_, + [data = std::move(data)]() mutable { + void* mapping = data->GetMutableMapping(); + Dart_Handle byte_buffer; + size_t data_size = data->GetSize(); + if (mapping && + data_size >= tonic::DartByteData::kExternalSizeThreshold) { + byte_buffer = Dart_NewExternalTypedDataWithFinalizer( + /*type=*/Dart_TypedData_kByteData, + /*data=*/mapping, + /*length=*/data_size, + /*peer=*/data.release(), + /*external_allocation_size=*/data_size, + /*callback=*/MappingFinalizer); - } else { - byte_buffer = tonic::DartByteData::Create( - data->GetMapping(), data->GetSize()); - } - return byte_buffer; - }); + } else { + byte_buffer = + tonic::DartByteData::Create(data->GetMapping(), data->GetSize()); + } + return byte_buffer; + }); } void PlatformMessageResponseDart::CompleteEmpty() { diff --git a/third_party/tonic/typed_data/dart_byte_data.cc b/third_party/tonic/typed_data/dart_byte_data.cc index cab038d14f85d..f4a557f17e946 100644 --- a/third_party/tonic/typed_data/dart_byte_data.cc +++ b/third_party/tonic/typed_data/dart_byte_data.cc @@ -11,17 +11,15 @@ namespace tonic { namespace { - -// For large objects it is more efficient to use an external typed data object -// with a buffer allocated outside the Dart heap. -const int kExternalSizeThreshold = 1000; - void FreeFinalizer(void* isolate_callback_data, void* peer) { free(peer); } - } // anonymous namespace +// For large objects it is more efficient to use an external typed data object +// with a buffer allocated outside the Dart heap. +const size_t DartByteData::kExternalSizeThreshold = 1000; + Dart_Handle DartByteData::Create(const void* data, size_t length) { if (length < kExternalSizeThreshold) { auto handle = DartByteData{data, length}.dart_handle(); diff --git a/third_party/tonic/typed_data/dart_byte_data.h b/third_party/tonic/typed_data/dart_byte_data.h index 9e14bc969fb8e..911d0d5da0d7e 100644 --- a/third_party/tonic/typed_data/dart_byte_data.h +++ b/third_party/tonic/typed_data/dart_byte_data.h @@ -14,6 +14,7 @@ namespace tonic { class DartByteData { public: + static const size_t kExternalSizeThreshold; static Dart_Handle Create(const void* data, size_t length); explicit DartByteData(Dart_Handle list);