From 34f2a93916568061e34a802536cd2d90c6c193b5 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 25 Jun 2024 14:19:18 -0700 Subject: [PATCH 1/4] node-api: remove RefBase and CallbackWrapper --- src/js_native_api_v8.cc | 265 +++++++++++++++------------------------- src/js_native_api_v8.h | 73 ++++------- 2 files changed, 120 insertions(+), 218 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 44270df350c174..4e34af95287dc0 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -134,16 +134,13 @@ napi_status NewExternalString(napi_env env, return status; } -class TrackedStringResource : public Finalizer, RefTracker { +class TrackedStringResource : public TrackedFinalizer { public: TrackedStringResource(napi_env env, napi_finalize finalize_callback, void* data, void* finalize_hint) - : Finalizer(env, finalize_callback, data, finalize_hint) { - Link(finalize_callback == nullptr ? &env->reflist - : &env->finalizing_reflist); - } + : TrackedFinalizer(env, finalize_callback, data, finalize_hint) {} protected: // The only time Finalize() gets called before Dispose() is if the @@ -156,7 +153,7 @@ class TrackedStringResource : public Finalizer, RefTracker { env_ = nullptr; } - ~TrackedStringResource() { + ~TrackedStringResource() override { if (finalize_callback_ == nullptr) return; if (env_ == nullptr) { // The environment is dead. Call the finalizer directly. @@ -362,7 +359,7 @@ inline napi_status Unwrap(napi_env env, static_cast(val.As()->Value()); if (result) { - *result = reference->Data(); + *result = reference->data(); } if (action == RemoveWrap) { @@ -404,6 +401,12 @@ class CallbackBundle { env, cbdata, 0, Ownership::kRuntime, Delete, bundle, nullptr); return cbdata; } + + static CallbackBundle* FromCallbackData(v8::Local data) { + return reinterpret_cast(data.As()->Value()); + } + + public: napi_env env; // Necessary to invoke C++ NAPI callback void* cb_data; // The user provided callback data napi_callback cb; @@ -415,71 +418,9 @@ class CallbackBundle { } }; -// Base class extended by classes that wrap V8 function and property callback -// info. -class CallbackWrapper { - public: - inline CallbackWrapper(napi_value this_arg, size_t args_length, void* data) - : _this(this_arg), _args_length(args_length), _data(data) {} - - virtual napi_value GetNewTarget() = 0; - virtual void Args(napi_value* buffer, size_t bufferlength) = 0; - virtual void SetReturnValue(napi_value value) = 0; - - napi_value This() { return _this; } - - size_t ArgsLength() { return _args_length; } - - void* Data() { return _data; } - - protected: - const napi_value _this; - const size_t _args_length; - void* _data; -}; - -class CallbackWrapperBase : public CallbackWrapper { - public: - inline CallbackWrapperBase(const v8::FunctionCallbackInfo& cbinfo, - const size_t args_length) - : CallbackWrapper( - JsValueFromV8LocalValue(cbinfo.This()), args_length, nullptr), - _cbinfo(cbinfo) { - _bundle = reinterpret_cast( - cbinfo.Data().As()->Value()); - _data = _bundle->cb_data; - } - - protected: - inline void InvokeCallback() { - napi_callback_info cbinfo_wrapper = reinterpret_cast( - static_cast(this)); - - // All other pointers we need are stored in `_bundle` - napi_env env = _bundle->env; - napi_callback cb = _bundle->cb; - - napi_value result = nullptr; - bool exceptionOccurred = false; - env->CallIntoModule([&](napi_env env) { result = cb(env, cbinfo_wrapper); }, - [&](napi_env env, v8::Local value) { - exceptionOccurred = true; - if (env->terminatedOrTerminating()) { - return; - } - env->isolate->ThrowException(value); - }); - - if (!exceptionOccurred && (result != nullptr)) { - this->SetReturnValue(result); - } - } - - const v8::FunctionCallbackInfo& _cbinfo; - CallbackBundle* _bundle; -}; - -class FunctionCallbackWrapper : public CallbackWrapperBase { +// Wraps up v8::FunctionCallbackInfo. +// The class must be stack allocated. +class FunctionCallbackWrapper { public: static void Invoke(const v8::FunctionCallbackInfo& info) { FunctionCallbackWrapper cbwrapper(info); @@ -514,41 +455,70 @@ class FunctionCallbackWrapper : public CallbackWrapperBase { return napi_clear_last_error(env); } - explicit FunctionCallbackWrapper( - const v8::FunctionCallbackInfo& cbinfo) - : CallbackWrapperBase(cbinfo, cbinfo.Length()) {} - - napi_value GetNewTarget() override { - if (_cbinfo.IsConstructCall()) { - return v8impl::JsValueFromV8LocalValue(_cbinfo.NewTarget()); + napi_value GetNewTarget() { + if (cbinfo_.IsConstructCall()) { + return v8impl::JsValueFromV8LocalValue(cbinfo_.NewTarget()); } else { return nullptr; } } - /*virtual*/ - void Args(napi_value* buffer, size_t buffer_length) override { + void Args(napi_value* buffer, size_t buffer_length) { size_t i = 0; - size_t min = std::min(buffer_length, _args_length); + size_t min_arg_count = std::min(buffer_length, ArgsLength()); - for (; i < min; i += 1) { - buffer[i] = v8impl::JsValueFromV8LocalValue(_cbinfo[i]); + for (; i < min_arg_count; ++i) { + buffer[i] = JsValueFromV8LocalValue(cbinfo_[i]); } if (i < buffer_length) { napi_value undefined = - v8impl::JsValueFromV8LocalValue(v8::Undefined(_cbinfo.GetIsolate())); - for (; i < buffer_length; i += 1) { + JsValueFromV8LocalValue(v8::Undefined(cbinfo_.GetIsolate())); + for (; i < buffer_length; ++i) { buffer[i] = undefined; } } } - /*virtual*/ - void SetReturnValue(napi_value value) override { - v8::Local val = v8impl::V8LocalValueFromJsValue(value); - _cbinfo.GetReturnValue().Set(val); + napi_value This() { return JsValueFromV8LocalValue(cbinfo_.This()); } + + size_t ArgsLength() { return static_cast(cbinfo_.Length()); } + + void* Data() { return bundle_->cb_data; } + + private: + explicit FunctionCallbackWrapper( + const v8::FunctionCallbackInfo& cbinfo) + : cbinfo_(cbinfo), + bundle_(CallbackBundle::FromCallbackData(cbinfo.Data())) {} + + void InvokeCallback() { + napi_callback_info cbinfo_wrapper = + reinterpret_cast(this); + + // All other pointers we need are stored in `_bundle` + napi_env env = bundle_->env; + napi_callback cb = bundle_->cb; + + napi_value result = nullptr; + bool exceptionOccurred = false; + env->CallIntoModule([&](napi_env env) { result = cb(env, cbinfo_wrapper); }, + [&](napi_env env, v8::Local value) { + exceptionOccurred = true; + if (env->terminatedOrTerminating()) { + return; + } + env->isolate->ThrowException(value); + }); + + if (!exceptionOccurred && (result != nullptr)) { + cbinfo_.GetReturnValue().Set(V8LocalValueFromJsValue(result)); + } } + + private: + const v8::FunctionCallbackInfo& cbinfo_; + CallbackBundle* bundle_; }; inline napi_status Wrap(napi_env env, @@ -650,7 +620,9 @@ TrackedFinalizer::~TrackedFinalizer() { // Remove from the env's tracked list. Unlink(); // Try to remove the finalizer from the scheduled second pass callback. - env_->DequeueFinalizer(this); + if (env_ != nullptr) { + env_->DequeueFinalizer(this); + } } void TrackedFinalizer::Finalize() { @@ -665,12 +637,12 @@ void TrackedFinalizer::FinalizeCore(bool deleteMe) { void* finalize_hint = finalize_hint_; ResetFinalizer(); - // Either the RefBase is going to be deleted in the finalize_callback or not, - // it should be removed from the tracked list. + // Either the TrackedFinalizer is going to be deleted in the finalize_callback + // or not, it should be removed from the tracked list. Unlink(); // If the finalize_callback is present, it should either delete the - // derived RefBase, or the RefBase ownership was set to Ownership::kRuntime - // and the deleteMe parameter is true. + // derived Reference, or the Reference ownership was set to + // Ownership::kRuntime and the deleteMe parameter is true. if (finalize_callback != nullptr) { env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); } @@ -680,62 +652,19 @@ void TrackedFinalizer::FinalizeCore(bool deleteMe) { } } -// Wrapper around v8impl::Persistent that implements reference counting. -RefBase::RefBase(napi_env env, - uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) +Reference::Reference(napi_env env, + v8::Local value, + uint32_t initial_refcount, + Ownership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) : TrackedFinalizer(env, finalize_callback, finalize_data, finalize_hint), - refcount_(initial_refcount), - ownership_(ownership) {} - -RefBase* RefBase::New(napi_env env, - uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) { - return new RefBase(env, - initial_refcount, - ownership, - finalize_callback, - finalize_data, - finalize_hint); -} - -void* RefBase::Data() { - return finalize_data_; -} - -uint32_t RefBase::Ref() { - return ++refcount_; -} - -uint32_t RefBase::Unref() { - if (refcount_ == 0) { - return 0; - } - return --refcount_; -} - -uint32_t RefBase::RefCount() { - return refcount_; -} - -void RefBase::Finalize() { - // If the RefBase is not Ownership::kRuntime, userland code should delete it. - // Delete it if it is Ownership::kRuntime. - FinalizeCore(/*deleteMe:*/ ownership_ == Ownership::kRuntime); -} - -template -Reference::Reference(napi_env env, v8::Local value, Args&&... args) - : RefBase(env, std::forward(args)...), persistent_(env->isolate, value), + refcount_(initial_refcount), + ownership_(ownership), can_be_weak_(CanBeHeldWeakly(value)) { - if (RefCount() == 0) { + if (refcount_ == 0) { SetWeak(); } } @@ -767,25 +696,22 @@ uint32_t Reference::Ref() { if (persistent_.IsEmpty()) { return 0; } - uint32_t refcount = RefBase::Ref(); - if (refcount == 1 && can_be_weak_) { + if (++refcount_ == 1 && can_be_weak_) { persistent_.ClearWeak(); } - return refcount; + return refcount_; } uint32_t Reference::Unref() { // When the persistent_ is cleared in the WeakCallback, and a second pass // callback is pending, return 0 unconditionally. - if (persistent_.IsEmpty()) { + if (persistent_.IsEmpty() || refcount_ == 0) { return 0; } - uint32_t old_refcount = RefCount(); - uint32_t refcount = RefBase::Unref(); - if (old_refcount == 1 && refcount == 0) { + if (--refcount_ == 0) { SetWeak(); } - return refcount; + return refcount_; } v8::Local Reference::Get() { @@ -801,8 +727,9 @@ void Reference::Finalize() { // be invoked again. persistent_.Reset(); - // Chain up to perform the rest of the finalization. - RefBase::Finalize(); + // If the RefBase is not Ownership::kRuntime, userland code should delete it. + // Delete it if it is Ownership::kRuntime. + FinalizeCore(/*deleteMe:*/ ownership_ == Ownership::kRuntime); } // Mark the reference as weak and eligible for collection @@ -2048,8 +1975,8 @@ napi_status NAPI_CDECL napi_get_cb_info( CHECK_ENV(env); CHECK_ARG(env, cbinfo); - v8impl::CallbackWrapper* info = - reinterpret_cast(cbinfo); + v8impl::FunctionCallbackWrapper* info = + reinterpret_cast(cbinfo); if (argv != nullptr) { CHECK_ARG(env, argc); @@ -2075,8 +2002,8 @@ napi_status NAPI_CDECL napi_get_new_target(napi_env env, CHECK_ARG(env, cbinfo); CHECK_ARG(env, result); - v8impl::CallbackWrapper* info = - reinterpret_cast(cbinfo); + v8impl::FunctionCallbackWrapper* info = + reinterpret_cast(cbinfo); *result = info->GetNewTarget(); return napi_clear_last_error(env); @@ -2793,7 +2720,7 @@ napi_status NAPI_CDECL napi_reference_unref(napi_env env, v8impl::Reference* reference = reinterpret_cast(ref); - if (reference->RefCount() == 0) { + if (reference->refcount() == 0) { return napi_set_last_error(env, napi_generic_failure); } @@ -3475,15 +3402,16 @@ napi_status NAPI_CDECL napi_set_instance_data(node_api_nogc_env nogc_env, napi_env env = const_cast(nogc_env); CHECK_ENV(env); - v8impl::RefBase* old_data = static_cast(env->instance_data); + v8impl::TrackedFinalizer* old_data = + static_cast(env->instance_data); if (old_data != nullptr) { // Our contract so far has been to not finalize any old data there may be. // So we simply delete it. delete old_data; } - env->instance_data = v8impl::RefBase::New( - env, 0, v8impl::Ownership::kRuntime, finalize_cb, data, finalize_hint); + env->instance_data = + v8impl::TrackedFinalizer::New(env, finalize_cb, data, finalize_hint); return napi_clear_last_error(env); } @@ -3493,9 +3421,10 @@ napi_status NAPI_CDECL napi_get_instance_data(node_api_nogc_env env, CHECK_ENV(env); CHECK_ARG(env, data); - v8impl::RefBase* idata = static_cast(env->instance_data); + v8impl::TrackedFinalizer* idata = + static_cast(env->instance_data); - *data = (idata == nullptr ? nullptr : idata->Data()); + *data = (idata == nullptr ? nullptr : idata->data()); return napi_clear_last_error(env); } diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 1974c3f6873ef7..b536e03a38dfa3 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -370,16 +370,6 @@ class TryCatch : public v8::TryCatch { napi_env _env; }; -// Ownership of a reference. -enum class Ownership { - // The reference is owned by the runtime. No userland call is needed to - // destruct the reference. - kRuntime, - // The reference is owned by the userland. User code is responsible to delete - // the reference with appropriate node-api calls. - kUserland, -}; - // Wrapper around Finalizer that can be tracked. class TrackedFinalizer : public Finalizer, public RefTracker { protected: @@ -400,45 +390,18 @@ class TrackedFinalizer : public Finalizer, public RefTracker { void FinalizeCore(bool deleteMe); }; -// Wrapper around TrackedFinalizer that implements reference counting. -class RefBase : public TrackedFinalizer { - protected: - RefBase(napi_env env, - uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint); - - public: - static RefBase* New(napi_env env, - uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint); - - void* Data(); - uint32_t Ref(); - uint32_t Unref(); - uint32_t RefCount(); - - Ownership ownership() { return ownership_; } - - protected: - void Finalize() override; - - private: - uint32_t refcount_; - Ownership ownership_; +// Ownership of a reference. +enum class Ownership : uint8_t { + // The reference is owned by the runtime. No userland call is needed to + // destruct the reference. + kRuntime, + // The reference is owned by the userland. User code is responsible to delete + // the reference with appropriate node-api calls. + kUserland, }; // Wrapper around v8impl::Persistent. -class Reference : public RefBase { - protected: - template - Reference(napi_env env, v8::Local value, Args&&... args); - +class Reference : public TrackedFinalizer { public: static Reference* New(napi_env env, v8::Local value, @@ -447,21 +410,31 @@ class Reference : public RefBase { napi_finalize finalize_callback = nullptr, void* finalize_data = nullptr, void* finalize_hint = nullptr); + ~Reference() override; - virtual ~Reference(); uint32_t Ref(); uint32_t Unref(); v8::Local Get(); - protected: - void Finalize() override; + uint32_t refcount() { return refcount_; } + Ownership ownership() { return ownership_; } private: + Reference(napi_env env, + v8::Local value, + uint32_t initial_refcount, + Ownership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); static void WeakCallback(const v8::WeakCallbackInfo& data); - void SetWeak(); + void Finalize() override; + private: v8impl::Persistent persistent_; + uint32_t refcount_; + Ownership ownership_; bool can_be_weak_; }; From 6f25e8fe92b3ab3a3473585eb5772db06abbd3bf Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Jun 2024 06:46:27 -0700 Subject: [PATCH 2/4] reduce size of Reference --- src/js_native_api_v8.cc | 289 +++++++++++++++++++++++++--------------- src/js_native_api_v8.h | 133 +++++++++++------- src/node_api.cc | 12 +- 3 files changed, 274 insertions(+), 160 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 4e34af95287dc0..252c1a881ebbdc 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -134,40 +134,38 @@ napi_status NewExternalString(napi_env env, return status; } -class TrackedStringResource : public TrackedFinalizer { +class TrackedStringResource : private RefTracker { public: TrackedStringResource(napi_env env, napi_finalize finalize_callback, void* data, void* finalize_hint) - : TrackedFinalizer(env, finalize_callback, data, finalize_hint) {} + : RefTracker(), finalizer_(env, finalize_callback, data, finalize_hint) { + Link(finalize_callback == nullptr ? &env->reflist + : &env->finalizing_reflist); + } protected: - // The only time Finalize() gets called before Dispose() is if the + // The only time Finalize() gets called before destructor is if the // environment is dying. Finalize() expects that the item will be unlinked, - // so we do it here. V8 will still call Dispose() on us later, so we don't do - // any deleting here. We just null out env_ to avoid passing a stale pointer - // to the user's finalizer when V8 does finally call Dispose(). + // so we do it here. V8 will still call destructor on us later, so we don't do + // any deleting here. We just null out env to avoid passing a stale pointer + // to the user's finalizer when V8 does finally call destructor. void Finalize() override { Unlink(); - env_ = nullptr; + finalizer_.ResetEnv(); } ~TrackedStringResource() override { - if (finalize_callback_ == nullptr) return; - if (env_ == nullptr) { - // The environment is dead. Call the finalizer directly. - finalize_callback_(nullptr, finalize_data_, finalize_hint_); - } else { - // The environment is still alive. Let's remove ourselves from its list - // of references and call the user's finalizer. - Unlink(); - env_->CallFinalizer(finalize_callback_, finalize_data_, finalize_hint_); - } + Unlink(); + finalizer_.CallFinalizer(); } + + private: + Finalizer finalizer_; }; -class ExternalOneByteStringResource +class ExternalOneByteStringResource final : public v8::String::ExternalOneByteStringResource, TrackedStringResource { public: @@ -188,8 +186,8 @@ class ExternalOneByteStringResource const size_t length_; }; -class ExternalStringResource : public v8::String::ExternalStringResource, - TrackedStringResource { +class ExternalStringResource final : public v8::String::ExternalStringResource, + TrackedStringResource { public: ExternalStringResource(napi_env env, char16_t* string, @@ -359,13 +357,13 @@ inline napi_status Unwrap(napi_env env, static_cast(val.As()->Value()); if (result) { - *result = reference->data(); + *result = reference->Data(); } if (action == RemoveWrap) { CHECK(obj->DeletePrivate(context, NAPI_PRIVATE_KEY(context, wrapper)) .FromJust()); - if (reference->ownership() == Ownership::kUserland) { + if (reference->ownership() == ReferenceOwnership::kUserland) { // When the wrap is been removed, the finalizer should be reset. reference->ResetFinalizer(); } else { @@ -397,8 +395,8 @@ class CallbackBundle { bundle->env = env; v8::Local cbdata = v8::External::New(env->isolate, bundle); - Reference::New( - env, cbdata, 0, Ownership::kRuntime, Delete, bundle, nullptr); + ReferenceWithFinalizer::New( + env, cbdata, 0, ReferenceOwnership::kRuntime, Delete, bundle, nullptr); return cbdata; } @@ -549,24 +547,29 @@ inline napi_status Wrap(napi_env env, // before then, then the finalize callback will never be invoked.) // Therefore a finalize callback is required when returning a reference. CHECK_ARG(env, finalize_cb); - reference = v8impl::Reference::New(env, - obj, - 0, - v8impl::Ownership::kUserland, - finalize_cb, - native_object, - finalize_hint); + reference = v8impl::ReferenceWithFinalizer::New( + env, + obj, + 0, + v8impl::ReferenceOwnership::kUserland, + finalize_cb, + native_object, + finalize_hint); *result = reinterpret_cast(reference); - } else { + } else if (finalize_cb != nullptr) { // Create a self-deleting reference. - reference = v8impl::Reference::New( + reference = v8impl::ReferenceWithFinalizer::New( env, obj, 0, - v8impl::Ownership::kRuntime, + v8impl::ReferenceOwnership::kRuntime, finalize_cb, native_object, - finalize_cb == nullptr ? nullptr : finalize_hint); + finalize_hint); + } else { + // Create a self-deleting reference. + reference = v8impl::ReferenceWithData::New( + env, obj, 0, v8impl::ReferenceOwnership::kRuntime, native_object); } CHECK(obj->SetPrivate(context, @@ -591,27 +594,50 @@ inline bool CanBeHeldWeakly(v8::Local value) { } // end of anonymous namespace +void Finalizer::ResetEnv() { + env_ = nullptr; +} + void Finalizer::ResetFinalizer() { finalize_callback_ = nullptr; finalize_data_ = nullptr; finalize_hint_ = nullptr; } +void Finalizer::ResetData(void* data) { + finalize_data_ = data; +} + +void Finalizer::CallFinalizer() { + napi_finalize finalize_callback = finalize_callback_; + void* finalize_data = finalize_data_; + void* finalize_hint = finalize_hint_; + ResetFinalizer(); + + if (finalize_callback == nullptr) return; + if (env_ == nullptr) { + // The environment is dead. Call the finalizer directly. + finalize_callback(nullptr, finalize_data, finalize_hint); + } else { + env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); + } +} + TrackedFinalizer::TrackedFinalizer(napi_env env, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) - : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - RefTracker() { - Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); -} + : RefTracker(), + finalizer_(env, finalize_callback, finalize_data, finalize_hint) {} TrackedFinalizer* TrackedFinalizer::New(napi_env env, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) { - return new TrackedFinalizer( + TrackedFinalizer* finalizer = new TrackedFinalizer( env, finalize_callback, finalize_data, finalize_hint); + finalizer->Link(&env->finalizing_reflist); + return finalizer; } // When a TrackedFinalizer is being deleted, it may have been queued to call its @@ -620,46 +646,20 @@ TrackedFinalizer::~TrackedFinalizer() { // Remove from the env's tracked list. Unlink(); // Try to remove the finalizer from the scheduled second pass callback. - if (env_ != nullptr) { - env_->DequeueFinalizer(this); - } + finalizer_.env()->DequeueFinalizer(this); } void TrackedFinalizer::Finalize() { - FinalizeCore(/*deleteMe:*/ true); -} - -void TrackedFinalizer::FinalizeCore(bool deleteMe) { - // Swap out the field finalize_callback so that it can not be accidentally - // called more than once. - napi_finalize finalize_callback = finalize_callback_; - void* finalize_data = finalize_data_; - void* finalize_hint = finalize_hint_; - ResetFinalizer(); - - // Either the TrackedFinalizer is going to be deleted in the finalize_callback - // or not, it should be removed from the tracked list. Unlink(); - // If the finalize_callback is present, it should either delete the - // derived Reference, or the Reference ownership was set to - // Ownership::kRuntime and the deleteMe parameter is true. - if (finalize_callback != nullptr) { - env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); - } - - if (deleteMe) { - delete this; - } + finalizer_.CallFinalizer(); + delete this; } Reference::Reference(napi_env env, v8::Local value, uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) - : TrackedFinalizer(env, finalize_callback, finalize_data, finalize_hint), + ReferenceOwnership ownership) + : RefTracker(), persistent_(env->isolate, value), refcount_(initial_refcount), ownership_(ownership), @@ -672,22 +672,18 @@ Reference::Reference(napi_env env, Reference::~Reference() { // Reset the handle. And no weak callback will be invoked. persistent_.Reset(); + + // Remove from the env's tracked list. + Unlink(); } Reference* Reference::New(napi_env env, v8::Local value, uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) { - return new Reference(env, - value, - initial_refcount, - ownership, - finalize_callback, - finalize_data, - finalize_hint); + ReferenceOwnership ownership) { + Reference* reference = new Reference(env, value, initial_refcount, ownership); + reference->Link(&env->reflist); + return reference; } uint32_t Reference::Ref() { @@ -714,11 +710,11 @@ uint32_t Reference::Unref() { return refcount_; } -v8::Local Reference::Get() { +v8::Local Reference::Get(napi_env env) { if (persistent_.IsEmpty()) { return v8::Local(); } else { - return v8::Local::New(env_->isolate, persistent_); + return v8::Local::New(env->isolate, persistent_); } } @@ -727,13 +723,30 @@ void Reference::Finalize() { // be invoked again. persistent_.Reset(); - // If the RefBase is not Ownership::kRuntime, userland code should delete it. - // Delete it if it is Ownership::kRuntime. - FinalizeCore(/*deleteMe:*/ ownership_ == Ownership::kRuntime); + // If the Reference is not ReferenceOwnership::kRuntime, userland code should + // delete it. Delete it if it is ReferenceOwnership::kRuntime. + bool deleteMe = ownership_ == ReferenceOwnership::kRuntime; + + // Whether the Reference is going to be deleted in the finalize_callback + // or not, it should be removed from the tracked list. + Unlink(); + + // If the finalize_callback is present, it should either delete the + // derived Reference, or the Reference ownership was set to + // ReferenceOwnership::kRuntime and the deleteMe parameter is true. + CallUserFinalizer(); + + if (deleteMe) { + delete this; + } } -// Mark the reference as weak and eligible for collection -// by the gc. +// Call the Finalize immediately since there is no user finalizer to call. +void Reference::InvokeFinalizerFromGC() { + Finalize(); +} + +// Mark the reference as weak and eligible for collection by the GC. void Reference::SetWeak() { if (can_be_weak_) { persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); @@ -742,14 +755,76 @@ void Reference::SetWeak() { } } -// The N-API finalizer callback may make calls into the engine. V8's heap is -// not in a consistent state during the weak callback, and therefore it does -// not support calls back into it. Enqueue the invocation of the finalizer. +// Static function called by GC. Delegate the call to the reference instance. void Reference::WeakCallback(const v8::WeakCallbackInfo& data) { Reference* reference = data.GetParameter(); - // The reference must be reset during the weak callback as the API protocol. + // The reference must be reset during the weak callback per V8 API protocol. reference->persistent_.Reset(); - reference->env_->InvokeFinalizerFromGC(reference); + reference->InvokeFinalizerFromGC(); +} + +ReferenceWithData* ReferenceWithData::New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + void* data) { + ReferenceWithData* reference = + new ReferenceWithData(env, value, initial_refcount, ownership, data); + reference->Link(&env->reflist); + return reference; +} + +ReferenceWithData::ReferenceWithData(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + void* data) + : Reference(env, value, initial_refcount, ownership), data_(data) {} + +ReferenceWithFinalizer* ReferenceWithFinalizer::New( + napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + ReferenceWithFinalizer* reference = + new ReferenceWithFinalizer(env, + value, + initial_refcount, + ownership, + finalize_callback, + finalize_data, + finalize_hint); + reference->Link(&env->finalizing_reflist); + return reference; +} + +ReferenceWithFinalizer::ReferenceWithFinalizer(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) + : Reference(env, value, initial_refcount, ownership), + finalizer_(env, finalize_callback, finalize_data, finalize_hint) {} + +ReferenceWithFinalizer::~ReferenceWithFinalizer() { + // Try to remove the finalizer from the scheduled second pass callback. + finalizer_.env()->DequeueFinalizer(this); +} + +void ReferenceWithFinalizer::CallUserFinalizer() { + finalizer_.CallFinalizer(); +} + +// The Node-API finalizer callback may make calls into the engine. V8's heap is +// not in a consistent state during the weak callback, and therefore it does +// not support calls back into it. Enqueue the invocation of the finalizer. +void ReferenceWithFinalizer::InvokeFinalizerFromGC() { + finalizer_.env()->InvokeFinalizerFromGC(this); } /** @@ -2523,13 +2598,13 @@ napi_create_external(napi_env env, if (finalize_cb) { // The Reference object will delete itself after invoking the finalizer // callback. - v8impl::Reference::New(env, - external_value, - 0, - v8impl::Ownership::kRuntime, - finalize_cb, - data, - finalize_hint); + v8impl::ReferenceWithFinalizer::New(env, + external_value, + 0, + v8impl::ReferenceOwnership::kRuntime, + finalize_cb, + data, + finalize_hint); } *result = v8impl::JsValueFromV8LocalValue(external_value); @@ -2664,7 +2739,7 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, } v8impl::Reference* reference = v8impl::Reference::New( - env, v8_value, initial_refcount, v8impl::Ownership::kUserland); + env, v8_value, initial_refcount, v8impl::ReferenceOwnership::kUserland); *result = reinterpret_cast(reference); return napi_clear_last_error(env); @@ -2746,7 +2821,7 @@ napi_status NAPI_CDECL napi_get_reference_value(napi_env env, CHECK_ARG(env, result); v8impl::Reference* reference = reinterpret_cast(ref); - *result = v8impl::JsValueFromV8LocalValue(reference->Get()); + *result = v8impl::JsValueFromV8LocalValue(reference->Get(env)); return napi_clear_last_error(env); } @@ -3356,10 +3431,10 @@ napi_add_finalizer(napi_env env, // Create a self-deleting reference if the optional out-param result is not // set. - v8impl::Ownership ownership = result == nullptr - ? v8impl::Ownership::kRuntime - : v8impl::Ownership::kUserland; - v8impl::Reference* reference = v8impl::Reference::New( + v8impl::ReferenceOwnership ownership = + result == nullptr ? v8impl::ReferenceOwnership::kRuntime + : v8impl::ReferenceOwnership::kUserland; + v8impl::Reference* reference = v8impl::ReferenceWithFinalizer::New( env, v8_value, 0, ownership, finalize_cb, finalize_data, finalize_hint); if (result != nullptr) { diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index b536e03a38dfa3..9a7e3201b226e6 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -8,14 +8,15 @@ inline napi_status napi_clear_last_error(node_api_nogc_env env); namespace v8impl { +// Base class to track references and finalizers in a double linked list. class RefTracker { public: + using RefList = RefTracker; + RefTracker() = default; virtual ~RefTracker() = default; virtual void Finalize() {} - typedef RefTracker RefList; - inline void Link(RefList* list) { prev_ = list; next_ = list->next_; @@ -47,7 +48,6 @@ class RefTracker { RefList* prev_ = nullptr; }; -class Finalizer; } // end of namespace v8impl struct napi_env__ { @@ -99,11 +99,7 @@ struct napi_env__ { } } - // Call finalizer immediately. - virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) { - v8::HandleScope handle_scope(isolate); - CallIntoModule([&](napi_env env) { cb(env, data, hint); }); - } + virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) = 0; // Invoke finalizer from V8 garbage collector. void InvokeFinalizerFromGC(v8impl::RefTracker* finalizer); @@ -323,7 +319,7 @@ inline v8::Local V8LocalValueFromJsValue(napi_value v) { // Adapter for napi_finalize callbacks. class Finalizer { - protected: + public: Finalizer(napi_env env, napi_finalize finalize_callback, void* finalize_data, @@ -333,23 +329,15 @@ class Finalizer { finalize_data_(finalize_data), finalize_hint_(finalize_hint) {} - virtual ~Finalizer() = default; - - public: - static Finalizer* New(napi_env env, - napi_finalize finalize_callback = nullptr, - void* finalize_data = nullptr, - void* finalize_hint = nullptr) { - return new Finalizer(env, finalize_callback, finalize_data, finalize_hint); - } - - napi_finalize callback() { return finalize_callback_; } + napi_env env() { return env_; } void* data() { return finalize_data_; } - void* hint() { return finalize_hint_; } + void ResetEnv(); void ResetFinalizer(); + void ResetData(void* data); + void CallFinalizer(); - protected: + private: napi_env env_; napi_finalize finalize_callback_; void* finalize_data_; @@ -371,13 +359,7 @@ class TryCatch : public v8::TryCatch { }; // Wrapper around Finalizer that can be tracked. -class TrackedFinalizer : public Finalizer, public RefTracker { - protected: - TrackedFinalizer(napi_env env, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint); - +class TrackedFinalizer final : public RefTracker { public: static TrackedFinalizer* New(napi_env env, napi_finalize finalize_callback, @@ -385,13 +367,21 @@ class TrackedFinalizer : public Finalizer, public RefTracker { void* finalize_hint); ~TrackedFinalizer() override; - protected: + void* data() { return finalizer_.data(); } + + private: + TrackedFinalizer(napi_env env, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); void Finalize() override; - void FinalizeCore(bool deleteMe); + + private: + Finalizer finalizer_; }; // Ownership of a reference. -enum class Ownership : uint8_t { +enum class ReferenceOwnership : uint8_t { // The reference is owned by the runtime. No userland call is needed to // destruct the reference. kRuntime, @@ -401,32 +391,33 @@ enum class Ownership : uint8_t { }; // Wrapper around v8impl::Persistent. -class Reference : public TrackedFinalizer { +class Reference : public RefTracker { public: static Reference* New(napi_env env, v8::Local value, uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback = nullptr, - void* finalize_data = nullptr, - void* finalize_hint = nullptr); + ReferenceOwnership ownership); ~Reference() override; uint32_t Ref(); uint32_t Unref(); - v8::Local Get(); + v8::Local Get(napi_env env); + + virtual void ResetFinalizer() {} + virtual void* Data() { return nullptr; } uint32_t refcount() { return refcount_; } - Ownership ownership() { return ownership_; } + ReferenceOwnership ownership() { return ownership_; } - private: + protected: Reference(napi_env env, v8::Local value, uint32_t initial_refcount, - Ownership ownership, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint); + ReferenceOwnership ownership); + virtual void CallUserFinalizer() {} + virtual void InvokeFinalizerFromGC(); + + private: static void WeakCallback(const v8::WeakCallbackInfo& data); void SetWeak(); void Finalize() override; @@ -434,10 +425,62 @@ class Reference : public TrackedFinalizer { private: v8impl::Persistent persistent_; uint32_t refcount_; - Ownership ownership_; + ReferenceOwnership ownership_; bool can_be_weak_; }; +// Reference that can store additional data. +class ReferenceWithData final : public Reference { + public: + static ReferenceWithData* New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + void* data); + + void* Data() override { return data_; } + + private: + ReferenceWithData(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + void* data); + + private: + void* data_; +}; + +// Reference that has a user finalizer callback. +class ReferenceWithFinalizer final : public Reference { + public: + static ReferenceWithFinalizer* New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + ~ReferenceWithFinalizer() override; + + void ResetFinalizer() override { finalizer_.ResetFinalizer(); } + void* Data() override { return finalizer_.data(); } + + private: + ReferenceWithFinalizer(napi_env env, + v8::Local value, + uint32_t initial_refcount, + ReferenceOwnership ownership, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + void CallUserFinalizer() override; + void InvokeFinalizerFromGC() override; + + private: + Finalizer finalizer_; +}; + } // end of namespace v8impl #endif // SRC_JS_NATIVE_API_V8_H_ diff --git a/src/node_api.cc b/src/node_api.cc index 27f67d8c7a3875..2a9c1670039ac3 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -131,13 +131,9 @@ class BufferFinalizer : private Finalizer { static void FinalizeBufferCallback(char* data, void* hint) { std::unique_ptr finalizer{ static_cast(hint)}; - finalizer->finalize_data_ = data; - + finalizer->ResetData(data); // It is safe to call into JavaScript at this point. - if (finalizer->finalize_callback_ == nullptr) return; - finalizer->env_->CallFinalizer(finalizer->finalize_callback_, - finalizer->finalize_data_, - finalizer->finalize_hint_); + finalizer->CallFinalizer(); } struct Deleter { @@ -150,10 +146,10 @@ class BufferFinalizer : private Finalizer { void* finalize_data, void* finalize_hint) : Finalizer(env, finalize_callback, finalize_data, finalize_hint) { - env_->Ref(); + env->Ref(); } - ~BufferFinalizer() { env_->Unref(); } + ~BufferFinalizer() { env()->Unref(); } }; void ThrowNodeApiVersionError(node::Environment* node_env, From e73c7a0f1dbf1fa19f7fc515442fa5050a30774e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Jun 2024 14:50:10 -0700 Subject: [PATCH 3/4] address PR feedback --- src/js_native_api_v8.cc | 4 ---- src/js_native_api_v8.h | 3 +-- src/node_api.cc | 9 ++++----- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 252c1a881ebbdc..76162605cc45f9 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -604,10 +604,6 @@ void Finalizer::ResetFinalizer() { finalize_hint_ = nullptr; } -void Finalizer::ResetData(void* data) { - finalize_data_ = data; -} - void Finalizer::CallFinalizer() { napi_finalize finalize_callback = finalize_callback_; void* finalize_data = finalize_data_; diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 9a7e3201b226e6..ee3aa38ebf8bd4 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -8,7 +8,7 @@ inline napi_status napi_clear_last_error(node_api_nogc_env env); namespace v8impl { -// Base class to track references and finalizers in a double linked list. +// Base class to track references and finalizers in a doubly linked list. class RefTracker { public: using RefList = RefTracker; @@ -334,7 +334,6 @@ class Finalizer { void ResetEnv(); void ResetFinalizer(); - void ResetData(void* data); void CallFinalizer(); private: diff --git a/src/node_api.cc b/src/node_api.cc index 2a9c1670039ac3..be9c169cc5c51e 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -121,9 +121,9 @@ namespace { class BufferFinalizer : private Finalizer { public: static BufferFinalizer* New(napi_env env, - napi_finalize finalize_callback = nullptr, - void* finalize_data = nullptr, - void* finalize_hint = nullptr) { + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { return new BufferFinalizer( env, finalize_callback, finalize_data, finalize_hint); } @@ -131,7 +131,6 @@ class BufferFinalizer : private Finalizer { static void FinalizeBufferCallback(char* data, void* hint) { std::unique_ptr finalizer{ static_cast(hint)}; - finalizer->ResetData(data); // It is safe to call into JavaScript at this point. finalizer->CallFinalizer(); } @@ -1053,7 +1052,7 @@ napi_create_external_buffer(napi_env env, // The finalizer object will delete itself after invoking the callback. v8impl::BufferFinalizer* finalizer = - v8impl::BufferFinalizer::New(env, finalize_cb, nullptr, finalize_hint); + v8impl::BufferFinalizer::New(env, finalize_cb, data, finalize_hint); v8::MaybeLocal maybe = node::Buffer::New(isolate, From 2fceb7783743f96add5001c369c46358e41a7525 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Wed, 21 Aug 2024 20:21:03 -0700 Subject: [PATCH 4/4] add const for refcount() Co-authored-by: Yagiz Nizipli --- src/js_native_api_v8.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index ee3aa38ebf8bd4..479c4976f1ddec 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -405,7 +405,7 @@ class Reference : public RefTracker { virtual void ResetFinalizer() {} virtual void* Data() { return nullptr; } - uint32_t refcount() { return refcount_; } + uint32_t refcount() const { return refcount_; } ReferenceOwnership ownership() { return ownership_; } protected: