src: make AliasedBuffers in the binding data weak

The binding data holds references to the AliasedBuffers directly from their wrappers which already ensures that the AliasedBuffers won't be accessed when the wrappers are GC'ed. So we can just make the global references to the AliasedBuffers weak. This way we can simply deserialize the typed arrays when deserialize the binding data and avoid the extra Object::Set() calls. It also eliminates the caveat in the JS land where aliased buffers must be dynamically read from the binding. PR-URL: #47354 Refs: #47353 Reviewed-By: Chengzhong Wu <[email protected]>
nodejs · May 2, 2023 · cfafe43 · cfafe43
1 parent 568256d
commit cfafe43
Show file tree

Hide file tree

Showing 13 changed files with 230 additions and 101 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -59,9 +59,6 @@ const {
 const pathModule = require('path');
 const { isArrayBufferView } = require('internal/util/types');
 
-// We need to get the statValues from the binding at the callsite since
-// it's re-initialized after deserialization.
-
 const binding = internalBinding('fs');
 
 const { createBlobFromFilePath } = require('internal/blob');
@@ -78,7 +75,10 @@ const {
   uvException,
 } = require('internal/errors');
 
-const { FSReqCallback } = binding;
+const {
+  FSReqCallback,
+  statValues,
+} = binding;
 const { toPathIfFileURL } = require('internal/url');
 const {
   customPromisifyArgs: kCustomPromisifyArgsSymbol,
@@ -2569,8 +2569,8 @@ function realpathSync(p, options) {
 
     // Continue if not a symlink, break if a pipe/socket
     if (knownHard.has(base) || cache?.get(base) === base) {
-      if (isFileType(binding.statValues, S_IFIFO) ||
-          isFileType(binding.statValues, S_IFSOCK)) {
+      if (isFileType(statValues, S_IFIFO) ||
+          isFileType(statValues, S_IFSOCK)) {
         break;
       }
       continue;
@@ -2727,8 +2727,8 @@ function realpath(p, options, callback) {
 
     // Continue if not a symlink, break if a pipe/socket
     if (knownHard.has(base)) {
-      if (isFileType(binding.statValues, S_IFIFO) ||
-          isFileType(binding.statValues, S_IFSOCK)) {
+      if (isFileType(statValues, S_IFIFO) ||
+          isFileType(statValues, S_IFSOCK)) {
         return callback(null, encodeRealpathResult(p, options));
       }
       return process.nextTick(LOOP);

diff --git a/lib/internal/encoding.js b/lib/internal/encoding.js
@@ -51,6 +51,7 @@ const {
 const binding = internalBinding('encoding_binding');
 const {
   encodeInto,
+  encodeIntoResults,
   encodeUtf8String,
   decodeUTF8,
 } = binding;
@@ -341,7 +342,7 @@ class TextEncoder {
     encodeInto(src, dest);
     // We need to read from the binding here since the buffer gets refreshed
     // from the snapshot.
-    const { 0: read, 1: written } = binding.encodeIntoResults;
+    const { 0: read, 1: written } = encodeIntoResults;
     return { read, written };
   }
 

diff --git a/lib/v8.js b/lib/v8.js
@@ -139,6 +139,10 @@ const {
   kBytecodeAndMetadataSizeIndex,
   kExternalScriptSourceSizeIndex,
   kCPUProfilerMetaDataSizeIndex,
+
+  heapStatisticsBuffer,
+  heapCodeStatisticsBuffer,
+  heapSpaceStatisticsBuffer,
 } = binding;
 
 const kNumberOfHeapSpaces = kHeapSpaces.length;
@@ -170,7 +174,7 @@ function setFlagsFromString(flags) {
  *   }}
  */
 function getHeapStatistics() {
-  const buffer = binding.heapStatisticsBuffer;
+  const buffer = heapStatisticsBuffer;
 
   updateHeapStatisticsBuffer();
 
@@ -204,7 +208,7 @@ function getHeapStatistics() {
  */
 function getHeapSpaceStatistics() {
   const heapSpaceStatistics = new Array(kNumberOfHeapSpaces);
-  const buffer = binding.heapSpaceStatisticsBuffer;
+  const buffer = heapSpaceStatisticsBuffer;
 
   for (let i = 0; i < kNumberOfHeapSpaces; i++) {
     updateHeapSpaceStatisticsBuffer(i);
@@ -230,7 +234,7 @@ function getHeapSpaceStatistics() {
  *   }}
  */
 function getHeapCodeStatistics() {
-  const buffer = binding.heapCodeStatisticsBuffer;
+  const buffer = heapCodeStatisticsBuffer;
 
   updateHeapCodeStatisticsBuffer();
   return {

diff --git a/src/aliased_buffer-inl.h b/src/aliased_buffer-inl.h
@@ -70,14 +70,14 @@ AliasedBufferBase<NativeT, V8T>::AliasedBufferBase(
       count_(that.count_),
       byte_offset_(that.byte_offset_),
       buffer_(that.buffer_) {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   js_array_ = v8::Global<V8T>(that.isolate_, that.GetJSArray());
 }
 
 template <typename NativeT, typename V8T>
 AliasedBufferIndex AliasedBufferBase<NativeT, V8T>::Serialize(
     v8::Local<v8::Context> context, v8::SnapshotCreator* creator) {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   return creator->AddData(context, GetJSArray());
 }
 
@@ -100,7 +100,7 @@ inline void AliasedBufferBase<NativeT, V8T>::Deserialize(
 template <typename NativeT, typename V8T>
 AliasedBufferBase<NativeT, V8T>& AliasedBufferBase<NativeT, V8T>::operator=(
     AliasedBufferBase<NativeT, V8T>&& that) noexcept {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   this->~AliasedBufferBase();
   isolate_ = that.isolate_;
   count_ = that.count_;
@@ -116,7 +116,7 @@ AliasedBufferBase<NativeT, V8T>& AliasedBufferBase<NativeT, V8T>::operator=(
 
 template <typename NativeT, typename V8T>
 v8::Local<V8T> AliasedBufferBase<NativeT, V8T>::GetJSArray() const {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   return js_array_.Get(isolate_);
 }
 
@@ -126,6 +126,21 @@ void AliasedBufferBase<NativeT, V8T>::Release() {
   js_array_.Reset();
 }
 
+template <typename NativeT, typename V8T>
+inline void AliasedBufferBase<NativeT, V8T>::WeakCallback(
+    const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data) {
+  AliasedBufferBase<NativeT, V8T>* buffer = data.GetParameter();
+  DCHECK(buffer->is_valid());
+  buffer->cleared_ = true;
+  buffer->js_array_.Reset();
+}
+
+template <typename NativeT, typename V8T>
+inline void AliasedBufferBase<NativeT, V8T>::MakeWeak() {
+  DCHECK(is_valid());
+  js_array_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
+}
+
 template <typename NativeT, typename V8T>
 v8::Local<v8::ArrayBuffer> AliasedBufferBase<NativeT, V8T>::GetArrayBuffer()
     const {
@@ -134,7 +149,7 @@ v8::Local<v8::ArrayBuffer> AliasedBufferBase<NativeT, V8T>::GetArrayBuffer()
 
 template <typename NativeT, typename V8T>
 inline const NativeT* AliasedBufferBase<NativeT, V8T>::GetNativeBuffer() const {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   return buffer_;
 }
 
@@ -147,22 +162,22 @@ template <typename NativeT, typename V8T>
 inline void AliasedBufferBase<NativeT, V8T>::SetValue(const size_t index,
                                                       NativeT value) {
   DCHECK_LT(index, count_);
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   buffer_[index] = value;
 }
 
 template <typename NativeT, typename V8T>
 inline const NativeT AliasedBufferBase<NativeT, V8T>::GetValue(
     const size_t index) const {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   DCHECK_LT(index, count_);
   return buffer_[index];
 }
 
 template <typename NativeT, typename V8T>
 typename AliasedBufferBase<NativeT, V8T>::Reference
 AliasedBufferBase<NativeT, V8T>::operator[](size_t index) {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   return Reference(this, index);
 }
 
@@ -178,7 +193,7 @@ size_t AliasedBufferBase<NativeT, V8T>::Length() const {
 
 template <typename NativeT, typename V8T>
 void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
-  DCHECK_NULL(index_);
+  DCHECK(is_valid());
   DCHECK_GE(new_capacity, count_);
   DCHECK_EQ(byte_offset_, 0);
   const v8::HandleScope handle_scope(isolate_);
@@ -206,6 +221,11 @@ void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
   count_ = new_capacity;
 }
 
+template <typename NativeT, typename V8T>
+inline bool AliasedBufferBase<NativeT, V8T>::is_valid() const {
+  return index_ == nullptr && !cleared_;
+}
+
 template <typename NativeT, typename V8T>
 inline size_t AliasedBufferBase<NativeT, V8T>::SelfSize() const {
   return sizeof(*this);

diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h
@@ -117,6 +117,14 @@ class AliasedBufferBase : public MemoryRetainer {
 
   void Release();
 
+  /**
+   * Make the global reference to the typed array weak. The caller must make
+   * sure that no operation can be done on the AliasedBuffer when the typed
+   * array becomes unreachable. Usually this means the caller must maintain
+   * a JS reference to the typed array from JS object.
+   */
+  inline void MakeWeak();
+
   /**
   *  Get the underlying v8::ArrayBuffer underlying the TypedArray and
   *  overlaying the native buffer
@@ -164,11 +172,15 @@ class AliasedBufferBase : public MemoryRetainer {
   inline void MemoryInfo(node::MemoryTracker* tracker) const override;
 
  private:
+  inline bool is_valid() const;
+  static inline void WeakCallback(
+      const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data);
   v8::Isolate* isolate_ = nullptr;
   size_t count_ = 0;
   size_t byte_offset_ = 0;
   NativeT* buffer_ = nullptr;
   v8::Global<V8T> js_array_;
+  bool cleared_ = false;
 
   // Deserialize data
   const AliasedBufferIndex* index_ = nullptr;

diff --git a/src/encoding_binding.cc b/src/encoding_binding.cc
@@ -28,30 +28,41 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const {
                       encode_into_results_buffer_);
 }
 
-BindingData::BindingData(Realm* realm, v8::Local<v8::Object> object)
+BindingData::BindingData(Realm* realm,
+                         v8::Local<v8::Object> object,
+                         InternalFieldInfo* info)
     : SnapshotableObject(realm, object, type_int),
-      encode_into_results_buffer_(realm->isolate(), kEncodeIntoResultsLength) {
-  object
-      ->Set(realm->context(),
-            FIXED_ONE_BYTE_STRING(realm->isolate(), "encodeIntoResults"),
-            encode_into_results_buffer_.GetJSArray())
-      .Check();
+      encode_into_results_buffer_(
+          realm->isolate(),
+          kEncodeIntoResultsLength,
+          MAYBE_FIELD_PTR(info, encode_into_results_buffer)) {
+  if (info == nullptr) {
+    object
+        ->Set(realm->context(),
+              FIXED_ONE_BYTE_STRING(realm->isolate(), "encodeIntoResults"),
+              encode_into_results_buffer_.GetJSArray())
+        .Check();
+  } else {
+    encode_into_results_buffer_.Deserialize(realm->context());
+  }
+  encode_into_results_buffer_.MakeWeak();
 }
 
 bool BindingData::PrepareForSerialization(Local<Context> context,
                                           v8::SnapshotCreator* creator) {
-  // We'll just re-initialize the buffers in the constructor since their
-  // contents can be thrown away once consumed in the previous call.
-  encode_into_results_buffer_.Release();
+  DCHECK_NULL(internal_field_info_);
+  internal_field_info_ = InternalFieldInfoBase::New<InternalFieldInfo>(type());
+  internal_field_info_->encode_into_results_buffer =
+      encode_into_results_buffer_.Serialize(context, creator);
   // Return true because we need to maintain the reference to the binding from
   // JS land.
   return true;
 }
 
 InternalFieldInfoBase* BindingData::Serialize(int index) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
-  InternalFieldInfo* info =
-      InternalFieldInfoBase::New<InternalFieldInfo>(type());
+  InternalFieldInfo* info = internal_field_info_;
+  internal_field_info_ = nullptr;
   return info;
 }
 
@@ -63,7 +74,9 @@ void BindingData::Deserialize(Local<Context> context,
   v8::HandleScope scope(context->GetIsolate());
   Realm* realm = Realm::GetCurrent(context);
   // Recreate the buffer in the constructor.
-  BindingData* binding = realm->AddBindingData<BindingData>(context, holder);
+  InternalFieldInfo* casted_info = static_cast<InternalFieldInfo*>(info);
+  BindingData* binding =
+      realm->AddBindingData<BindingData>(context, holder, casted_info);
   CHECK_NOT_NULL(binding);
 }
 

diff --git a/src/encoding_binding.h b/src/encoding_binding.h
@@ -14,10 +14,13 @@ class ExternalReferenceRegistry;
 namespace encoding_binding {
 class BindingData : public SnapshotableObject {
  public:
-  BindingData(Realm* realm, v8::Local<v8::Object> obj);
-
-  using InternalFieldInfo = InternalFieldInfoBase;
+  struct InternalFieldInfo : public node::InternalFieldInfoBase {
+    AliasedBufferIndex encode_into_results_buffer;
+  };
 
+  BindingData(Realm* realm,
+              v8::Local<v8::Object> obj,
+              InternalFieldInfo* info = nullptr);
   SERIALIZABLE_OBJECT_METHODS()
   SET_BINDING_ID(encoding_binding_data)
 
@@ -39,6 +42,7 @@ class BindingData : public SnapshotableObject {
  private:
   static constexpr size_t kEncodeIntoResultsLength = 2;
   AliasedUint32Array encode_into_results_buffer_;
+  InternalFieldInfo* internal_field_info_ = nullptr;
 };
 
 }  // namespace encoding_binding