Eliminates an extra copy when returning messages from the host platfo…

…rm to dart.
flutter · Jun 15, 2022 · a379730 · a379730
1 parent 10ff302
commit a379730
Show file tree

Hide file tree

Showing 13 changed files with 152 additions and 15 deletions.
diff --git 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
@@ -50,8 +50,14 @@ std::unique_ptr<fml::Mapping> DirectoryAssetBundle::GetAsMapping(
     return nullptr;
   }
 
-  auto mapping = std::make_unique<fml::FileMapping>(fml::OpenFile(
-      descriptor_, asset_name.c_str(), false, fml::FilePermission::kRead));
+  auto mapping = std::make_unique<fml::FileMapping>(
+      fml::OpenFile(descriptor_, asset_name.c_str(), false,
+                    fml::FilePermission::kRead),
+      std::initializer_list<fml::FileMapping::Protection>(
+          {fml::FileMapping::Protection::kRead,
+           fml::FileMapping::Protection::kWrite}),
+      std::initializer_list<fml::FileMapping::Flags>(
+          {fml::FileMapping::Flags::kCopyOnWrite}));
 
   if (!mapping->IsValid()) {
     return nullptr;

diff --git a/assets/directory_asset_bundle_unittests.cc b/assets/directory_asset_bundle_unittests.cc
@@ -0,0 +1,35 @@
+// 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<uint8_t*>(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<AssetResolver> bundle =
+      std::make_unique<DirectoryAssetBundle>(
+          std::move(descriptor), /*is_valid_after_asset_manager_change=*/true);
+  EXPECT_TRUE(bundle->IsValid());
+  std::unique_ptr<fml::Mapping> read_mapping = bundle->GetAsMapping(filename);
+  ASSERT_TRUE(read_mapping);
+  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<fml::Mapping> read_after_write_mapping =
+      bundle->GetAsMapping(filename);
+  EXPECT_EQ(read_after_write_mapping->GetMapping()[0], '\0');
+}
+
+}  // namespace testing
+}  // namespace flutter
diff --git 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
@@ -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
@@ -28,6 +28,8 @@ class Mapping {
 
   virtual const uint8_t* GetMapping() const = 0;
 
+  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 +46,15 @@ class FileMapping final : public Mapping {
     kExecute,
   };
 
-  explicit FileMapping(const fml::UniqueFD& fd,
-                       std::initializer_list<Protection> protection = {
-                           Protection::kRead});
+  enum class Flags {
+    kNone,
+    kCopyOnWrite,
+  };
+
+  explicit FileMapping(
+      const fml::UniqueFD& fd,
+      std::initializer_list<Protection> protection = {Protection::kRead},
+      std::initializer_list<Flags> flags = {Flags::kNone});
 
   ~FileMapping() override;
 
@@ -72,7 +80,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 +112,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 +140,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 +192,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 +222,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
@@ -46,12 +46,23 @@ static bool IsWritable(
   return false;
 }
 
+static bool IsCopyOnWrite(
+    const std::initializer_list<FileMapping::Flags>& 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> protection)
+                         std::initializer_list<Protection> protection,
+                         std::initializer_list<Flags> flags)
     : size_(0), mapping_(nullptr) {
   if (!handle.is_valid()) {
     return;
@@ -72,7 +83,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
@@ -40,8 +40,19 @@ static bool IsExecutable(
   return false;
 }
 
+static bool IsCopyOnWrite(
+    const std::initializer_list<FileMapping::Flags>& flags) {
+  for (auto flag : flags) {
+    if (flag == FileMapping::Flags::kCopyOnWrite) {
+      return true;
+    }
+  }
+  return false;
+}
+
 FileMapping::FileMapping(const fml::UniqueFD& fd,
-                         std::initializer_list<Protection> protections)
+                         std::initializer_list<Protection> protections,
+                         std::initializer_list<Flags> flags)
     : size_(0), mapping_(nullptr) {
   if (!fd.is_valid()) {
     return;
@@ -82,7 +93,9 @@ FileMapping::FileMapping(const fml::UniqueFD& fd,
     return;
   }
 
-  const DWORD desired_access = read_only ? FILE_MAP_READ : FILE_MAP_WRITE;
+  const DWORD desired_access =
+      read_only ? FILE_MAP_READ
+                : (IsCopyOnWrite(flags) ? FILE_MAP_COPY : FILE_MAP_WRITE);
 
   auto mapping = reinterpret_cast<uint8_t*>(
       MapViewOfFile(mapping_handle_.get(), desired_access, 0, 0, mapping_size));

diff --git a/lib/ui/window/platform_message_response_dart.cc b/lib/ui/window/platform_message_response_dart.cc
@@ -14,6 +14,12 @@
 
 namespace flutter {
 
+namespace {
+void MappingFinalizer(void* isolate_callback_data, void* peer) {
+  delete static_cast<fml::Mapping*>(peer);
+}
+}  // anonymous namespace
+
 PlatformMessageResponseDart::PlatformMessageResponseDart(
     tonic::DartPersistentValue callback,
     fml::RefPtr<fml::TaskRunner> ui_task_runner)
@@ -42,8 +48,22 @@ void PlatformMessageResponseDart::Complete(std::unique_ptr<fml::Mapping> data) {
         }
         tonic::DartState::Scope scope(dart_state);
 
-        Dart_Handle byte_buffer =
-            tonic::DartByteData::Create(data->GetMapping(), data->GetSize());
+        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());
+        }
         tonic::DartInvoke(callback.Release(), {byte_buffer});
       }));
 }

diff --git 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
@@ -48,6 +48,8 @@ class APKAssetMapping : public fml::Mapping {
     return reinterpret_cast<const uint8_t*>(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
@@ -18,6 +18,13 @@ explicit NSDataMapping(NSData* data) : data_([data retain]) {}
     return static_cast<const uint8_t*>([data_.get() bytes]);
   }
 
+  uint8_t* GetMutableMapping() const override {
+    id id_data = data_.get();
+    // In practice these are all NSMutableData (see ConvertMappingToNSData).
+    FML_DCHECK([id_data respondsToSelector:@selector(mutableBytes)]);
+    return static_cast<uint8_t*>([id_data mutableBytes]);
+  }
+
   bool IsDontNeedSafe() const override { return false; }
 
  private:
@@ -33,15 +40,15 @@ explicit NSDataMapping(NSData* data) : data_([data retain]) {}
 
 NSData* ConvertMappingToNSData(fml::MallocMapping buffer) {
   size_t size = buffer.GetSize();
-  return [NSData dataWithBytesNoCopy:buffer.Release() length:size];
+  return [NSMutableData dataWithBytesNoCopy:buffer.Release() length:size];
 }
 
 std::unique_ptr<fml::Mapping> ConvertNSDataToMappingPtr(NSData* data) {
   return std::make_unique<NSDataMapping>(data);
 }
 
 NSData* CopyMappingPtrToNSData(std::unique_ptr<fml::Mapping> mapping) {
-  return [NSData dataWithBytes:mapping->GetMapping() length:mapping->GetSize()];
+  return [NSMutableData dataWithBytes:mapping->GetMapping() length:mapping->GetSize()];
 }
 
 }  // namespace flutter
diff --git 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;