src: remove custom tracking for SharedArrayBuffers

Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs#30044 PR-URL: nodejs#30020 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax · Nov 8, 2019 · 2bdeb88 · 2bdeb88
1 parent 2707efd
commit 2bdeb88
Show file tree

Hide file tree

Showing 13 changed files with 58 additions and 335 deletions.
diff --git a/node.gyp b/node.gyp
@@ -564,7 +564,6 @@
         'src/node_zlib.cc',
         'src/pipe_wrap.cc',
         'src/process_wrap.cc',
-        'src/sharedarraybuffer_metadata.cc',
         'src/signal_wrap.cc',
         'src/spawn_sync.cc',
         'src/stream_base.cc',
@@ -642,7 +641,6 @@
         'src/pipe_wrap.h',
         'src/req_wrap.h',
         'src/req_wrap-inl.h',
-        'src/sharedarraybuffer_metadata.h',
         'src/spawn_sync.h',
         'src/stream_base.h',
         'src/stream_base-inl.h',

diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -279,6 +279,14 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator,
   return NewIsolate(&params, event_loop, platform);
 }
 
+Isolate* NewIsolate(std::shared_ptr<ArrayBufferAllocator> allocator,
+                    uv_loop_t* event_loop,
+                    MultiIsolatePlatform* platform) {
+  Isolate::CreateParams params;
+  if (allocator) params.array_buffer_allocator_shared = allocator;
+  return NewIsolate(&params, event_loop, platform);
+}
+
 IsolateData* CreateIsolateData(Isolate* isolate,
                                uv_loop_t* loop,
                                MultiIsolatePlatform* platform,

diff --git a/src/env.cc b/src/env.cc
@@ -1038,21 +1038,6 @@ char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
   return new_data;
 }
 
-void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
-    std::shared_ptr<v8::ArrayBuffer::Allocator> allocator) {
-  if (keep_alive_allocators_ == nullptr) {
-    MultiIsolatePlatform* platform = isolate_data()->platform();
-    CHECK_NOT_NULL(platform);
-
-    keep_alive_allocators_ = new ArrayBufferAllocatorList();
-    platform->AddIsolateFinishedCallback(isolate(), [](void* data) {
-      delete static_cast<ArrayBufferAllocatorList*>(data);
-    }, static_cast<void*>(keep_alive_allocators_));
-  }
-
-  keep_alive_allocators_->insert(allocator);
-}
-
 bool Environment::RunWeakRefCleanup() {
   isolate()->ClearKeptObjects();
 

diff --git a/src/env.h b/src/env.h
@@ -156,7 +156,6 @@ constexpr size_t kFsStatsBufferLength =
   V(contextify_global_private_symbol, "node:contextify:global")               \
   V(decorated_private_symbol, "node:decorated")                               \
   V(napi_wrapper, "node:napi:wrapper")                                        \
-  V(sab_lifetimepartner_symbol, "node:sharedArrayBufferLifetimePartner")      \
 
 // Symbols are per-isolate primitives but Environment proxies them
 // for the sake of convenience.
@@ -1250,10 +1249,6 @@ class Environment : public MemoryRetainer {
 
 #endif  // HAVE_INSPECTOR
 
-  // Only available if a MultiIsolatePlatform is in use.
-  void AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
-      std::shared_ptr<v8::ArrayBuffer::Allocator>);
-
  private:
   template <typename Fn>
   inline void CreateImmediate(Fn&& cb,
@@ -1433,10 +1428,6 @@ class Environment : public MemoryRetainer {
   // Used by embedders to shutdown running Node instance.
   AsyncRequest thread_stopper_;
 
-  typedef std::unordered_set<std::shared_ptr<v8::ArrayBuffer::Allocator>>
-      ArrayBufferAllocatorList;
-  ArrayBufferAllocatorList* keep_alive_allocators_ = nullptr;
-
   template <typename T>
   void ForEachBaseObject(T&& iterator);
 

diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h
@@ -109,6 +109,16 @@ void MemoryTracker::TrackField(const char* edge_name,
   TrackField(edge_name, value.get(), node_name);
 }
 
+template <typename T>
+void MemoryTracker::TrackField(const char* edge_name,
+                               const std::shared_ptr<T>& value,
+                               const char* node_name) {
+  if (value.get() == nullptr) {
+    return;
+  }
+  TrackField(edge_name, value.get(), node_name);
+}
+
 template <typename T, typename Iterator>
 void MemoryTracker::TrackField(const char* edge_name,
                                const T& value,
@@ -206,6 +216,12 @@ void MemoryTracker::TrackField(const char* edge_name,
   TrackFieldWithSize(edge_name, value.size, "MallocedBuffer");
 }
 
+void MemoryTracker::TrackField(const char* edge_name,
+                               const v8::BackingStore* value,
+                               const char* node_name) {
+  TrackFieldWithSize(edge_name, value->ByteLength(), "BackingStore");
+}
+
 void MemoryTracker::TrackField(const char* name,
                                const uv_buf_t& value,
                                const char* node_name) {

diff --git a/src/memory_tracker.h b/src/memory_tracker.h
@@ -138,6 +138,10 @@ class MemoryTracker {
   inline void TrackField(const char* edge_name,
                          const std::unique_ptr<T>& value,
                          const char* node_name = nullptr);
+  template <typename T>
+  inline void TrackField(const char* edge_name,
+                         const std::shared_ptr<T>& value,
+                         const char* node_name = nullptr);
 
   // For containers, the elements will be graphed as grandchildren nodes
   // if the container is not empty.
@@ -197,6 +201,9 @@ class MemoryTracker {
   inline void TrackField(const char* edge_name,
                          const MallocedBuffer<T>& value,
                          const char* node_name = nullptr);
+  inline void TrackField(const char* edge_name,
+                         const v8::BackingStore* value,
+                         const char* node_name = nullptr);
   // We do not implement CleanupHookCallback as MemoryRetainer
   // but instead specialize the method here to avoid the cost of
   // virtual pointers.

diff --git a/src/node.h b/src/node.h
@@ -328,6 +328,10 @@ NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator,
 NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator,
                                     struct uv_loop_s* event_loop,
                                     MultiIsolatePlatform* platform);
+NODE_EXTERN v8::Isolate* NewIsolate(
+    std::shared_ptr<ArrayBufferAllocator> allocator,
+    struct uv_loop_s* event_loop,
+    MultiIsolatePlatform* platform);
 
 // Creates a new context with Node.js-specific tweaks.
 NODE_EXTERN v8::Local<v8::Context> NewContext(

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
@@ -12,7 +12,7 @@
 using node::contextify::ContextifyContext;
 using v8::Array;
 using v8::ArrayBuffer;
-using v8::ArrayBufferCreationMode;
+using v8::BackingStore;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::Exception;
@@ -123,10 +123,9 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
   std::vector<Local<SharedArrayBuffer>> shared_array_buffers;
   // Attach all transferred SharedArrayBuffers to their new Isolate.
   for (uint32_t i = 0; i < shared_array_buffers_.size(); ++i) {
-    Local<SharedArrayBuffer> sab;
-    if (!shared_array_buffers_[i]->GetSharedArrayBuffer(env, context)
-            .ToLocal(&sab))
-      return MaybeLocal<Value>();
+    Local<SharedArrayBuffer> sab =
+        SharedArrayBuffer::New(env->isolate(),
+                               std::move(shared_array_buffers_[i]));
     shared_array_buffers.push_back(sab);
   }
   shared_array_buffers_.clear();
@@ -141,30 +140,12 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
   delegate.deserializer = &deserializer;
 
   // Attach all transferred ArrayBuffers to their new Isolate.
-  for (uint32_t i = 0; i < array_buffer_contents_.size(); ++i) {
-    if (!env->isolate_data()->uses_node_allocator()) {
-      // We don't use Node's allocator on the receiving side, so we have
-      // to create the ArrayBuffer from a copy of the memory.
-      AllocatedBuffer buf =
-          env->AllocateManaged(array_buffer_contents_[i].size);
-      memcpy(buf.data(),
-             array_buffer_contents_[i].data,
-             array_buffer_contents_[i].size);
-      deserializer.TransferArrayBuffer(i, buf.ToArrayBuffer());
-      continue;
-    }
-
-    env->isolate_data()->node_allocator()->RegisterPointer(
-        array_buffer_contents_[i].data, array_buffer_contents_[i].size);
-
+  for (uint32_t i = 0; i < array_buffers_.size(); ++i) {
     Local<ArrayBuffer> ab =
-        ArrayBuffer::New(env->isolate(),
-                         array_buffer_contents_[i].release(),
-                         array_buffer_contents_[i].size,
-                         ArrayBufferCreationMode::kInternalized);
+        ArrayBuffer::New(env->isolate(), std::move(array_buffers_[i]));
     deserializer.TransferArrayBuffer(i, ab);
   }
-  array_buffer_contents_.clear();
+  array_buffers_.clear();
 
   if (deserializer.ReadHeader(context).IsNothing())
     return MaybeLocal<Value>();
@@ -173,8 +154,8 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
 }
 
 void Message::AddSharedArrayBuffer(
-    const SharedArrayBufferMetadataReference& reference) {
-  shared_array_buffers_.push_back(reference);
+    std::shared_ptr<BackingStore> backing_store) {
+  shared_array_buffers_.emplace_back(std::move(backing_store));
 }
 
 void Message::AddMessagePort(std::unique_ptr<MessagePortData>&& data) {
@@ -249,16 +230,9 @@ class SerializerDelegate : public ValueSerializer::Delegate {
       }
     }
 
-    auto reference = SharedArrayBufferMetadata::ForSharedArrayBuffer(
-        env_,
-        context_,
-        shared_array_buffer);
-    if (!reference) {
-      return Nothing<uint32_t>();
-    }
     seen_shared_array_buffers_.emplace_back(
       Global<SharedArrayBuffer> { isolate, shared_array_buffer });
-    msg_->AddSharedArrayBuffer(reference);
+    msg_->AddSharedArrayBuffer(shared_array_buffer->GetBackingStore());
     return Just(i);
   }
 
@@ -386,18 +360,12 @@ Maybe<bool> Message::Serialize(Environment* env,
   }
 
   for (Local<ArrayBuffer> ab : array_buffers) {
-    // If serialization succeeded, we want to take ownership of
-    // (a.k.a. externalize) the underlying memory region and render
-    // it inaccessible in this Isolate.
-    ArrayBuffer::Contents contents = ab->Externalize();
+    // If serialization succeeded, we render it inaccessible in this Isolate.
+    std::shared_ptr<BackingStore> backing_store = ab->GetBackingStore();
+    ab->Externalize(backing_store);
     ab->Detach();
 
-    CHECK(env->isolate_data()->uses_node_allocator());
-    env->isolate_data()->node_allocator()->UnregisterPointer(
-        contents.Data(), contents.ByteLength());
-
-    array_buffer_contents_.emplace_back(MallocedBuffer<char>{
-        static_cast<char*>(contents.Data()), contents.ByteLength()});
+    array_buffers_.emplace_back(std::move(backing_store));
   }
 
   delegate.Finish();
@@ -411,9 +379,8 @@ Maybe<bool> Message::Serialize(Environment* env,
 }
 
 void Message::MemoryInfo(MemoryTracker* tracker) const {
-  tracker->TrackField("array_buffer_contents", array_buffer_contents_);
-  tracker->TrackFieldWithSize("shared_array_buffers",
-      shared_array_buffers_.size() * sizeof(shared_array_buffers_[0]));
+  tracker->TrackField("array_buffers_", array_buffers_);
+  tracker->TrackField("shared_array_buffers", shared_array_buffers_);
   tracker->TrackField("message_ports", message_ports_);
 }
 

diff --git a/src/node_messaging.h b/src/node_messaging.h
@@ -5,7 +5,6 @@
 
 #include "env.h"
 #include "node_mutex.h"
-#include "sharedarraybuffer_metadata.h"
 #include <list>
 
 namespace node {
@@ -52,7 +51,7 @@ class Message : public MemoryRetainer {
 
   // Internal method of Message that is called when a new SharedArrayBuffer
   // object is encountered in the incoming value's structure.
-  void AddSharedArrayBuffer(const SharedArrayBufferMetadataReference& ref);
+  void AddSharedArrayBuffer(std::shared_ptr<v8::BackingStore> backing_store);
   // Internal method of Message that is called once serialization finishes
   // and that transfers ownership of `data` to this message.
   void AddMessagePort(std::unique_ptr<MessagePortData>&& data);
@@ -74,8 +73,8 @@ class Message : public MemoryRetainer {
 
  private:
   MallocedBuffer<char> main_message_buf_;
-  std::vector<MallocedBuffer<char>> array_buffer_contents_;
-  std::vector<SharedArrayBufferMetadataReference> shared_array_buffers_;
+  std::vector<std::shared_ptr<v8::BackingStore>> array_buffers_;
+  std::vector<std::shared_ptr<v8::BackingStore>> shared_array_buffers_;
   std::vector<std::unique_ptr<MessagePortData>> message_ports_;
   std::vector<v8::WasmModuleObject::TransferrableModule> wasm_modules_;
 

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -51,7 +51,6 @@ Worker::Worker(Environment* env,
       per_isolate_opts_(per_isolate_opts),
       exec_argv_(exec_argv),
       platform_(env->isolate_data()->platform()),
-      array_buffer_allocator_(ArrayBufferAllocator::Create()),
       start_profiler_idle_notifier_(env->profiler_idle_notifier_started()),
       thread_id_(Environment::AllocateThreadId()),
       env_vars_(env->env_vars()) {
@@ -95,10 +94,6 @@ bool Worker::is_stopped() const {
   return stopped_;
 }
 
-std::shared_ptr<ArrayBufferAllocator> Worker::array_buffer_allocator() {
-  return array_buffer_allocator_;
-}
-
 void Worker::UpdateResourceConstraints(ResourceConstraints* constraints) {
   constraints->set_stack_limit(reinterpret_cast<uint32_t*>(stack_base_));
 
@@ -138,9 +133,11 @@ class WorkerThreadData {
     : w_(w) {
     CHECK_EQ(uv_loop_init(&loop_), 0);
 
+    std::shared_ptr<ArrayBufferAllocator> allocator =
+        ArrayBufferAllocator::Create();
     Isolate::CreateParams params;
     SetIsolateCreateParamsForNode(&params);
-    params.array_buffer_allocator = w->array_buffer_allocator_.get();
+    params.array_buffer_allocator_shared = allocator;
 
     w->UpdateResourceConstraints(&params.constraints);
 
@@ -164,7 +161,7 @@ class WorkerThreadData {
       isolate_data_.reset(CreateIsolateData(isolate,
                                             &loop_,
                                             w_->platform_,
-                                            w->array_buffer_allocator_.get()));
+                                            allocator.get()));
       CHECK(isolate_data_);
       if (w_->per_isolate_opts_)
         isolate_data_->set_options(std::move(w_->per_isolate_opts_));

diff --git a/src/node_worker.h b/src/node_worker.h
@@ -48,7 +48,6 @@ class Worker : public AsyncWrap {
   SET_SELF_SIZE(Worker)
 
   bool is_stopped() const;
-  std::shared_ptr<ArrayBufferAllocator> array_buffer_allocator();
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void CloneParentEnvVars(
@@ -72,7 +71,6 @@ class Worker : public AsyncWrap {
   std::vector<std::string> argv_;
 
   MultiIsolatePlatform* platform_;
-  std::shared_ptr<ArrayBufferAllocator> array_buffer_allocator_;
   v8::Isolate* isolate_ = nullptr;
   bool start_profiler_idle_notifier_;
   uv_thread_t tid_;