Inform the Dart VM when snapshots are safe to use with madvise(DONTNE…

…ED). (#29320) Bug: dart-lang/sdk#44019 Bug: flutter/flutter#92120
flutter · Oct 28, 2021 · 111b80e · 111b80e
1 parent eb7401c
commit 111b80e
Show file tree

Hide file tree

Showing 16 changed files with 163 additions and 29 deletions.
diff --git a/fml/file_unittest.cc b/fml/file_unittest.cc
@@ -269,6 +269,36 @@ TEST(FileTest, EmptyMappingTest) {
   ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "my_contents"));
 }
 
+TEST(FileTest, MappingDontNeedSafeTest) {
+  fml::ScopedTemporaryDirectory dir;
+
+  {
+    auto file = fml::OpenFile(dir.fd(), "my_contents", true,
+                              fml::FilePermission::kReadWrite);
+    WriteStringToFile(file, "some content");
+  }
+
+  {
+    auto file = fml::OpenFile(dir.fd(), "my_contents", false,
+                              fml::FilePermission::kRead);
+    fml::FileMapping mapping(file);
+    ASSERT_TRUE(mapping.IsValid());
+    ASSERT_EQ(mapping.GetMutableMapping(), nullptr);
+    ASSERT_TRUE(mapping.IsDontNeedSafe());
+  }
+
+  {
+    auto file = fml::OpenFile(dir.fd(), "my_contents", false,
+                              fml::FilePermission::kReadWrite);
+    fml::FileMapping mapping(file, {fml::FileMapping::Protection::kRead,
+                                    fml::FileMapping::Protection::kWrite});
+    ASSERT_TRUE(mapping.IsValid());
+    ASSERT_NE(mapping.GetMutableMapping(), nullptr);
+    ASSERT_FALSE(mapping.IsDontNeedSafe());
+  }
+  ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "my_contents"));
+}
+
 TEST(FileTest, FileTestsWork) {
   fml::ScopedTemporaryDirectory dir;
   ASSERT_TRUE(dir.fd().is_valid());

diff --git a/fml/mapping.cc b/fml/mapping.cc
@@ -81,11 +81,19 @@ const uint8_t* DataMapping::GetMapping() const {
   return data_.data();
 }
 
+bool DataMapping::IsDontNeedSafe() const {
+  return false;
+}
+
 // NonOwnedMapping
 NonOwnedMapping::NonOwnedMapping(const uint8_t* data,
                                  size_t size,
-                                 const ReleaseProc& release_proc)
-    : data_(data), size_(size), release_proc_(release_proc) {}
+                                 const ReleaseProc& release_proc,
+                                 bool dontneed_safe)
+    : data_(data),
+      size_(size),
+      release_proc_(release_proc),
+      dontneed_safe_(dontneed_safe) {}
 
 NonOwnedMapping::~NonOwnedMapping() {
   if (release_proc_) {
@@ -101,6 +109,10 @@ const uint8_t* NonOwnedMapping::GetMapping() const {
   return data_;
 }
 
+bool NonOwnedMapping::IsDontNeedSafe() const {
+  return dontneed_safe_;
+}
+
 // MallocMapping
 MallocMapping::MallocMapping() : data_(nullptr), size_(0) {}
 
@@ -134,6 +146,10 @@ const uint8_t* MallocMapping::GetMapping() const {
   return data_;
 }
 
+bool MallocMapping::IsDontNeedSafe() const {
+  return false;
+}
+
 uint8_t* MallocMapping::Release() {
   uint8_t* result = data_;
   data_ = nullptr;
@@ -174,4 +190,8 @@ const uint8_t* SymbolMapping::GetMapping() const {
   return mapping_;
 }
 
+bool SymbolMapping::IsDontNeedSafe() const {
+  return true;
+}
+
 }  // namespace fml
diff --git a/fml/mapping.h b/fml/mapping.h
@@ -28,6 +28,10 @@ class Mapping {
 
   virtual const uint8_t* GetMapping() 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;
+
  private:
   FML_DISALLOW_COPY_AND_ASSIGN(Mapping);
 };
@@ -65,6 +69,9 @@ class FileMapping final : public Mapping {
   // |Mapping|
   const uint8_t* GetMapping() const override;
 
+  // |Mapping|
+  bool IsDontNeedSafe() const override;
+
   uint8_t* GetMutableMapping();
 
   bool IsValid() const;
@@ -96,6 +103,9 @@ class DataMapping final : public Mapping {
   // |Mapping|
   const uint8_t* GetMapping() const override;
 
+  // |Mapping|
+  bool IsDontNeedSafe() const override;
+
  private:
   std::vector<uint8_t> data_;
 
@@ -107,7 +117,8 @@ class NonOwnedMapping final : public Mapping {
   using ReleaseProc = std::function<void(const uint8_t* data, size_t size)>;
   NonOwnedMapping(const uint8_t* data,
                   size_t size,
-                  const ReleaseProc& release_proc = nullptr);
+                  const ReleaseProc& release_proc = nullptr,
+                  bool dontneed_safe = false);
 
   ~NonOwnedMapping() override;
 
@@ -117,10 +128,14 @@ class NonOwnedMapping final : public Mapping {
   // |Mapping|
   const uint8_t* GetMapping() const override;
 
+  // |Mapping|
+  bool IsDontNeedSafe() const override;
+
  private:
   const uint8_t* const data_;
   const size_t size_;
   const ReleaseProc release_proc_;
+  const bool dontneed_safe_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(NonOwnedMapping);
 };
@@ -162,6 +177,9 @@ class MallocMapping final : public Mapping {
   // |Mapping|
   const uint8_t* GetMapping() const override;
 
+  // |Mapping|
+  bool IsDontNeedSafe() const override;
+
   /// Removes ownership of the data buffer.
   /// After this is called; the mapping will point to nullptr.
   [[nodiscard]] uint8_t* Release();
@@ -186,6 +204,9 @@ class SymbolMapping final : public Mapping {
   // |Mapping|
   const uint8_t* GetMapping() const override;
 
+  // |Mapping|
+  bool IsDontNeedSafe() const override;
+
  private:
   fml::RefPtr<fml::NativeLibrary> native_library_;
   const uint8_t* mapping_ = nullptr;

diff --git a/fml/mapping_unittests.cc b/fml/mapping_unittests.cc
@@ -52,4 +52,11 @@ TEST(MallocMapping, Release) {
   ASSERT_EQ(0u, mapping.GetSize());
 }
 
+TEST(MallocMapping, IsDontNeedSafe) {
+  size_t length = 10;
+  MallocMapping mapping(reinterpret_cast<uint8_t*>(malloc(length)), length);
+  ASSERT_NE(nullptr, mapping.GetMapping());
+  ASSERT_FALSE(mapping.IsDontNeedSafe());
+}
+
 }  // namespace fml
diff --git a/fml/platform/posix/mapping_posix.cc b/fml/platform/posix/mapping_posix.cc
@@ -100,6 +100,10 @@ const uint8_t* FileMapping::GetMapping() const {
   return mapping_;
 }
 
+bool FileMapping::IsDontNeedSafe() const {
+  return mutable_mapping_ == nullptr;
+}
+
 bool FileMapping::IsValid() const {
   return valid_;
 }

diff --git a/fml/platform/win/mapping_win.cc b/fml/platform/win/mapping_win.cc
@@ -115,6 +115,10 @@ const uint8_t* FileMapping::GetMapping() const {
   return mapping_;
 }
 
+bool FileMapping::IsDontNeedSafe() const {
+  return mutable_mapping_ == nullptr;
+}
+
 bool FileMapping::IsValid() const {
   return valid_;
 }

diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc
@@ -74,6 +74,10 @@ void DartIsolate::Flags::SetNullSafetyEnabled(bool enabled) {
   flags_.null_safety = enabled;
 }
 
+void DartIsolate::Flags::SetIsDontNeedSafe(bool value) {
+  flags_.snapshot_is_dontneed_safe = value;
+}
+
 Dart_IsolateFlags DartIsolate::Flags::Get() const {
   return flags_;
 }
@@ -139,6 +143,7 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
 
   isolate_flags.SetNullSafetyEnabled(
       isolate_configration->IsNullSafetyEnabled(*isolate_snapshot));
+  isolate_flags.SetIsDontNeedSafe(isolate_snapshot->IsDontNeedSafe());
 
   auto isolate = CreateRootIsolate(settings,                           //
                                    isolate_snapshot,                   //

diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h
@@ -70,6 +70,7 @@ class DartIsolate : public UIDartState {
     ~Flags();
 
     void SetNullSafetyEnabled(bool enabled);
+    void SetIsDontNeedSafe(bool value);
 
     Dart_IsolateFlags Get() const;
 

diff --git a/runtime/dart_snapshot.cc b/runtime/dart_snapshot.cc
@@ -97,7 +97,11 @@ static std::shared_ptr<const fml::Mapping> SearchMapping(
 static std::shared_ptr<const fml::Mapping> ResolveVMData(
     const Settings& settings) {
 #if DART_SNAPSHOT_STATIC_LINK
-  return std::make_unique<fml::NonOwnedMapping>(kDartVmSnapshotData, 0);
+  return std::make_unique<fml::NonOwnedMapping>(kDartVmSnapshotData,
+                                                0,        // size
+                                                nullptr,  // release_func
+                                                true      // dontneed_safe
+  );
 #else   // DART_SNAPSHOT_STATIC_LINK
   return SearchMapping(
       settings.vm_snapshot_data,          // embedder_mapping_callback
@@ -112,7 +116,11 @@ static std::shared_ptr<const fml::Mapping> ResolveVMData(
 static std::shared_ptr<const fml::Mapping> ResolveVMInstructions(
     const Settings& settings) {
 #if DART_SNAPSHOT_STATIC_LINK
-  return std::make_unique<fml::NonOwnedMapping>(kDartVmSnapshotInstructions, 0);
+  return std::make_unique<fml::NonOwnedMapping>(kDartVmSnapshotInstructions,
+                                                0,        // size
+                                                nullptr,  // release_func
+                                                true      // dontneed_safe
+  );
 #else   // DART_SNAPSHOT_STATIC_LINK
   return SearchMapping(
       settings.vm_snapshot_instr,           // embedder_mapping_callback
@@ -127,7 +135,11 @@ static std::shared_ptr<const fml::Mapping> ResolveVMInstructions(
 static std::shared_ptr<const fml::Mapping> ResolveIsolateData(
     const Settings& settings) {
 #if DART_SNAPSHOT_STATIC_LINK
-  return std::make_unique<fml::NonOwnedMapping>(kDartIsolateSnapshotData, 0);
+  return std::make_unique<fml::NonOwnedMapping>(kDartIsolateSnapshotData,
+                                                0,        // size
+                                                nullptr,  // release_func
+                                                true      // dontneed_safe
+  );
 #else   // DART_SNAPSHOT_STATIC_LINK
   return SearchMapping(
       settings.isolate_snapshot_data,       // embedder_mapping_callback
@@ -143,7 +155,11 @@ static std::shared_ptr<const fml::Mapping> ResolveIsolateInstructions(
     const Settings& settings) {
 #if DART_SNAPSHOT_STATIC_LINK
   return std::make_unique<fml::NonOwnedMapping>(
-      kDartIsolateSnapshotInstructions, 0);
+      kDartIsolateSnapshotInstructions,
+      0,        // size
+      nullptr,  // release_func
+      true      // dontneed_safe
+  );
 #else   // DART_SNAPSHOT_STATIC_LINK
   return SearchMapping(
       settings.isolate_snapshot_instr,           // embedder_mapping_callback
@@ -233,6 +249,14 @@ const uint8_t* DartSnapshot::GetInstructionsMapping() const {
   return instructions_ ? instructions_->GetMapping() : nullptr;
 }
 
+bool DartSnapshot::IsDontNeedSafe() const {
+  if (data_ && !data_->IsDontNeedSafe())
+    return false;
+  if (instructions_ && !instructions_->IsDontNeedSafe())
+    return false;
+  return true;
+}
+
 bool DartSnapshot::IsNullSafetyEnabled(const fml::Mapping* kernel) const {
   return ::Dart_DetectNullSafety(
       nullptr,           // script_uri (unsupported by Flutter)

diff --git a/runtime/dart_snapshot.h b/runtime/dart_snapshot.h
@@ -160,6 +160,11 @@ class DartSnapshot : public fml::RefCountedThreadSafe<DartSnapshot> {
   ///
   const uint8_t* GetInstructionsMapping() const;
 
+  //----------------------------------------------------------------------------
+  /// @brief      Returns whether both the data and instructions mappings are
+  ///             safe to use with madvise(DONTNEED).
+  bool IsDontNeedSafe() const;
+
   bool IsNullSafetyEnabled(
       const fml::Mapping* application_kernel_mapping) const;
 

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_));
   }
 
+  bool IsDontNeedSafe() const override { return !AAsset_isAllocated(asset_); }
+
  private:
   AAsset* const asset_;
 

diff --git a/shell/platform/darwin/common/buffer_conversions.mm b/shell/platform/darwin/common/buffer_conversions.mm
@@ -18,6 +18,8 @@
     return static_cast<const uint8_t*>([data_.get() bytes]);
   }
 
+  bool IsDontNeedSafe() const override { return false; }
+
  private:
   fml::scoped_nsobject<NSData> data_;
   FML_DISALLOW_COPY_AND_ASSIGN(NSDataMapping);

diff --git a/shell/platform/fuchsia/flutter/component_v1.cc b/shell/platform/fuchsia/flutter/component_v1.cc
@@ -292,27 +292,28 @@ ComponentV1::ComponentV1(
       }
       auto hold_snapshot = [snapshot](const uint8_t* _, size_t __) {};
       settings_.vm_snapshot_data = [hold_snapshot, vm_data]() {
-        return std::make_unique<fml::NonOwnedMapping>(vm_data, 0,
-                                                      hold_snapshot);
+        return std::make_unique<fml::NonOwnedMapping>(vm_data, 0, hold_snapshot,
+                                                      true /* dontneed_safe */);
       };
       settings_.vm_snapshot_instr = [hold_snapshot, vm_instructions]() {
-        return std::make_unique<fml::NonOwnedMapping>(vm_instructions, 0,
-                                                      hold_snapshot);
+        return std::make_unique<fml::NonOwnedMapping>(
+            vm_instructions, 0, hold_snapshot, true /* dontneed_safe */);
       };
       settings_.isolate_snapshot_data = [hold_snapshot, isolate_data]() {
-        return std::make_unique<fml::NonOwnedMapping>(isolate_data, 0,
-                                                      hold_snapshot);
+        return std::make_unique<fml::NonOwnedMapping>(
+            isolate_data, 0, hold_snapshot, true /* dontneed_safe */);
       };
       settings_.isolate_snapshot_instr = [hold_snapshot,
                                           isolate_instructions]() {
-        return std::make_unique<fml::NonOwnedMapping>(isolate_instructions, 0,
-                                                      hold_snapshot);
+        return std::make_unique<fml::NonOwnedMapping>(
+            isolate_instructions, 0, hold_snapshot, true /* dontneed_safe */);
       };
       isolate_snapshot_ = fml::MakeRefCounted<flutter::DartSnapshot>(
-          std::make_shared<fml::NonOwnedMapping>(isolate_data, 0,
-                                                 hold_snapshot),
+          std::make_shared<fml::NonOwnedMapping>(isolate_data, 0, hold_snapshot,
+                                                 true /* dontneed_safe */),
           std::make_shared<fml::NonOwnedMapping>(isolate_instructions, 0,
-                                                 hold_snapshot));
+                                                 hold_snapshot,
+                                                 true /* dontneed_safe */));
     } else {
       const int namespace_fd = component_data_directory_.get();
       settings_.vm_snapshot_data = [namespace_fd]() {