Eliminates Dart->Host copy for mutable responses

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
+}

assets/directory_asset_bundle.cc

@@ -50,8 +50,27 @@ 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));
+// 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::FileMapping>(
+      fml::OpenFile(descriptor_, asset_name.c_str(), false,
+                    fml::FilePermission::kRead),
+      std::initializer_list<fml::FileMapping::Protection>(
+          {fml::FileMapping::Protection::kRead}),
+      std::initializer_list<fml::FileMapping::Flags>());
+#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::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}));
+#endif
 
   if (!mapping->IsValid()) {
     return nullptr;

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<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);
+
+#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<fml::Mapping> read_after_write_mapping =
+      bundle->GetAsMapping(filename);
+  EXPECT_EQ(read_after_write_mapping->GetMapping()[0], '\0');
+#endif
+}
+
+}  // namespace testing
+}  // namespace flutter

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

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;
 }

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 = {
-                           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 +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;
 

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) {
   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;

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,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<uint8_t*>(

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<fml::Mapping*>(peer);
+}
+}  // anonymous namespace
+
 PlatformMessageResponseDart::PlatformMessageResponseDart(
     tonic::DartPersistentValue callback,
     fml::RefPtr<fml::TaskRunner> ui_task_runner,
@@ -63,11 +69,28 @@ PlatformMessageResponseDart::~PlatformMessageResponseDart() {
 }
 
 void PlatformMessageResponseDart::Complete(std::unique_ptr<fml::Mapping> data) {
-  PostCompletion(std::move(callback_), ui_task_runner_, &is_complete_, channel_,
-                 [data = std::move(data)] {
-                   return tonic::DartByteData::Create(data->GetMapping(),
-                                                      data->GetSize());
-                 });
+  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;
+      });
 }
 
 void PlatformMessageResponseDart::CompleteEmpty() {

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",

shell/platform/android/apk_asset_provider.cc

@@ -25,6 +25,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:

shell/platform/darwin/common/buffer_conversions.mm

@@ -18,6 +18,15 @@ explicit NSDataMapping(NSData* data) : data_([data retain]) {}
     return static_cast<const uint8_t*>([data_.get() bytes]);
   }
 
+  uint8_t* GetMutableMapping() const override {
+    // 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; }
 
  private:

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;
 

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();

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);