From 79b2ba6744af6f63335b7f63b9a52789fd7aa32e Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 18 Nov 2020 14:32:23 -0800 Subject: [PATCH] n-api: clean up binding creation * Remove dead code for `GetterCallbackWrapper` and `SetterCallbackWrapper`. * Factor out creation of new `v8::Function`s. * Factor out creation of new `v8::FunctionTemplate`s. * Turn `CallbackBundle` into a class, internalizing creation of new instances and garbage collection. Signed-off-by: Gabriel Schulhof PR-URL: https://github.com/nodejs/node/pull/36170 Reviewed-By: James M Snell Reviewed-By: Rich Trott Reviewed-By: Anna Henningsen Reviewed-By: David Carlier Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 263 ++++++++++++---------------------------- 1 file changed, 80 insertions(+), 183 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 64e4298e122f43..e037c4297de0c5 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -418,18 +418,36 @@ inline static napi_status Unwrap(napi_env env, // calling through N-API. // Ref: benchmark/misc/function_call // Discussion (incl. perf. data): https://github.com/nodejs/node/pull/21072 -struct CallbackBundle { +class CallbackBundle { + public: + // Creates an object to be made available to the static function callback + // wrapper, used to retrieve the native callback function and data pointer. + static inline v8::Local + New(napi_env env, napi_callback cb, void* data) { + CallbackBundle* bundle = new CallbackBundle(); + bundle->cb = cb; + bundle->cb_data = data; + bundle->env = env; + + v8::Local cbdata = v8::External::New(env->isolate, bundle); + Reference::New(env, cbdata, 0, true, Delete, bundle, nullptr); + return cbdata; + } napi_env env; // Necessary to invoke C++ NAPI callback void* cb_data; // The user provided callback data - napi_callback function_or_getter; - napi_callback setter; + napi_callback cb; + private: + static void Delete(napi_env env, void* data, void* hint) { + CallbackBundle* bundle = static_cast(data); + delete bundle; + } }; // Base class extended by classes that wrap V8 function and property callback // info. class CallbackWrapper { public: - CallbackWrapper(napi_value this_arg, size_t args_length, void* data) + 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; @@ -448,10 +466,10 @@ class CallbackWrapper { void* _data; }; -template class CallbackWrapperBase : public CallbackWrapper { public: - CallbackWrapperBase(const Info& cbinfo, const size_t args_length) + inline CallbackWrapperBase(const v8::FunctionCallbackInfo& cbinfo, + const size_t args_length) : CallbackWrapper(JsValueFromV8LocalValue(cbinfo.This()), args_length, nullptr), @@ -461,16 +479,14 @@ class CallbackWrapperBase : public CallbackWrapper { _data = _bundle->cb_data; } - napi_value GetNewTarget() override { return nullptr; } - protected: - void InvokeCallback() { + 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->*FunctionField; + napi_callback cb = _bundle->cb; napi_value result; env->CallIntoModule([&](napi_env env) { @@ -482,19 +498,45 @@ class CallbackWrapperBase : public CallbackWrapper { } } - const Info& _cbinfo; + const v8::FunctionCallbackInfo& _cbinfo; CallbackBundle* _bundle; }; class FunctionCallbackWrapper - : public CallbackWrapperBase, - &CallbackBundle::function_or_getter> { + : public CallbackWrapperBase { public: static void Invoke(const v8::FunctionCallbackInfo& info) { FunctionCallbackWrapper cbwrapper(info); cbwrapper.InvokeCallback(); } + static inline napi_status NewFunction(napi_env env, + napi_callback cb, + void* cb_data, + v8::Local* result) { + v8::Local cbdata = v8impl::CallbackBundle::New(env, cb, cb_data); + RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); + + v8::MaybeLocal maybe_function = + v8::Function::New(env->context(), Invoke, cbdata); + CHECK_MAYBE_EMPTY(env, maybe_function, napi_generic_failure); + + *result = maybe_function.ToLocalChecked(); + return napi_clear_last_error(env); + } + + static inline napi_status NewTemplate(napi_env env, + napi_callback cb, + void* cb_data, + v8::Local* result, + v8::Local sig = v8::Local()) { + v8::Local cbdata = v8impl::CallbackBundle::New(env, cb, cb_data); + RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); + + *result = v8::FunctionTemplate::New(env->isolate, Invoke, cbdata, sig); + return napi_clear_last_error(env); + } + explicit FunctionCallbackWrapper( const v8::FunctionCallbackInfo& cbinfo) : CallbackWrapperBase(cbinfo, cbinfo.Length()) {} @@ -532,98 +574,6 @@ class FunctionCallbackWrapper } }; -class GetterCallbackWrapper - : public CallbackWrapperBase, - &CallbackBundle::function_or_getter> { - public: - static void Invoke(v8::Local property, - const v8::PropertyCallbackInfo& info) { - GetterCallbackWrapper cbwrapper(info); - cbwrapper.InvokeCallback(); - } - - explicit GetterCallbackWrapper( - const v8::PropertyCallbackInfo& cbinfo) - : CallbackWrapperBase(cbinfo, 0) {} - - /*virtual*/ - void Args(napi_value* buffer, size_t buffer_length) override { - if (buffer_length > 0) { - napi_value undefined = - v8impl::JsValueFromV8LocalValue(v8::Undefined(_cbinfo.GetIsolate())); - for (size_t i = 0; i < buffer_length; i += 1) { - buffer[i] = undefined; - } - } - } - - /*virtual*/ - void SetReturnValue(napi_value value) override { - v8::Local val = v8impl::V8LocalValueFromJsValue(value); - _cbinfo.GetReturnValue().Set(val); - } -}; - -class SetterCallbackWrapper - : public CallbackWrapperBase, - &CallbackBundle::setter> { - public: - static void Invoke(v8::Local property, - v8::Local value, - const v8::PropertyCallbackInfo& info) { - SetterCallbackWrapper cbwrapper(info, value); - cbwrapper.InvokeCallback(); - } - - SetterCallbackWrapper(const v8::PropertyCallbackInfo& cbinfo, - const v8::Local& value) - : CallbackWrapperBase(cbinfo, 1), _value(value) {} - - /*virtual*/ - void Args(napi_value* buffer, size_t buffer_length) override { - if (buffer_length > 0) { - buffer[0] = v8impl::JsValueFromV8LocalValue(_value); - - if (buffer_length > 1) { - napi_value undefined = v8impl::JsValueFromV8LocalValue( - v8::Undefined(_cbinfo.GetIsolate())); - for (size_t i = 1; i < buffer_length; i += 1) { - buffer[i] = undefined; - } - } - } - } - - /*virtual*/ - void SetReturnValue(napi_value value) override { - // Ignore any value returned from a setter callback. - } - - private: - const v8::Local& _value; -}; - -static void DeleteCallbackBundle(napi_env env, void* data, void* hint) { - CallbackBundle* bundle = static_cast(data); - delete bundle; -} - -// Creates an object to be made available to the static function callback -// wrapper, used to retrieve the native callback function and data pointer. -static -v8::Local CreateFunctionCallbackData(napi_env env, - napi_callback cb, - void* data) { - CallbackBundle* bundle = new CallbackBundle(); - bundle->function_or_getter = cb; - bundle->cb_data = data; - bundle->env = env; - v8::Local cbdata = v8::External::New(env->isolate, bundle); - Reference::New(env, cbdata, 0, true, DeleteCallbackBundle, bundle, nullptr); - - return cbdata; -} - enum WrapType { retrievable, anonymous @@ -745,22 +695,12 @@ napi_status napi_create_function(napi_env env, CHECK_ARG(env, result); CHECK_ARG(env, cb); - v8::Isolate* isolate = env->isolate; v8::Local return_value; - v8::EscapableHandleScope scope(isolate); - v8::Local cbdata = - v8impl::CreateFunctionCallbackData(env, cb, callback_data); - - RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); - - v8::Local context = env->context(); - v8::MaybeLocal maybe_function = - v8::Function::New(context, - v8impl::FunctionCallbackWrapper::Invoke, - cbdata); - CHECK_MAYBE_EMPTY(env, maybe_function, napi_generic_failure); - - return_value = scope.Escape(maybe_function.ToLocalChecked()); + v8::EscapableHandleScope scope(env->isolate); + v8::Local fn; + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction( + env, cb, callback_data, &fn)); + return_value = scope.Escape(fn); if (utf8name != nullptr) { v8::Local name_string; @@ -792,13 +732,9 @@ napi_status napi_define_class(napi_env env, v8::Isolate* isolate = env->isolate; v8::EscapableHandleScope scope(isolate); - v8::Local cbdata = - v8impl::CreateFunctionCallbackData(env, constructor, callback_data); - - RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); - - v8::Local tpl = v8::FunctionTemplate::New( - isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata); + v8::Local tpl; + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate( + env, constructor, callback_data, &tpl)); v8::Local name_string; CHECK_NEW_FROM_UTF8_LEN(env, name_string, utf8name, length); @@ -828,18 +764,12 @@ napi_status napi_define_class(napi_env env, v8::Local getter_tpl; v8::Local setter_tpl; if (p->getter != nullptr) { - v8::Local getter_data = - v8impl::CreateFunctionCallbackData(env, p->getter, p->data); - - getter_tpl = v8::FunctionTemplate::New( - isolate, v8impl::FunctionCallbackWrapper::Invoke, getter_data); + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate( + env, p->getter, p->data, &getter_tpl)); } if (p->setter != nullptr) { - v8::Local setter_data = - v8impl::CreateFunctionCallbackData(env, p->setter, p->data); - - setter_tpl = v8::FunctionTemplate::New( - isolate, v8impl::FunctionCallbackWrapper::Invoke, setter_data); + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate( + env, p->setter, p->data, &setter_tpl)); } tpl->PrototypeTemplate()->SetAccessorProperty( @@ -849,16 +779,9 @@ napi_status napi_define_class(napi_env env, attributes, v8::AccessControl::DEFAULT); } else if (p->method != nullptr) { - v8::Local cbdata = - v8impl::CreateFunctionCallbackData(env, p->method, p->data); - - RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); - - v8::Local t = - v8::FunctionTemplate::New(isolate, - v8impl::FunctionCallbackWrapper::Invoke, - cbdata, - v8::Signature::New(isolate, tpl)); + v8::Local t; + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewTemplate( + env, p->method, p->data, &t, v8::Signature::New(isolate, tpl))); tpl->PrototypeTemplate()->Set(property_name, t, attributes); } else { @@ -1263,33 +1186,16 @@ napi_status napi_define_properties(napi_env env, STATUS_CALL(v8impl::V8NameFromPropertyDescriptor(env, p, &property_name)); if (p->getter != nullptr || p->setter != nullptr) { - v8::Local local_getter; - v8::Local local_setter; + v8::Local local_getter; + v8::Local local_setter; if (p->getter != nullptr) { - v8::Local getter_data = - v8impl::CreateFunctionCallbackData(env, p->getter, p->data); - CHECK_MAYBE_EMPTY(env, getter_data, napi_generic_failure); - - v8::MaybeLocal maybe_getter = - v8::Function::New(context, - v8impl::FunctionCallbackWrapper::Invoke, - getter_data); - CHECK_MAYBE_EMPTY(env, maybe_getter, napi_generic_failure); - - local_getter = maybe_getter.ToLocalChecked(); + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction( + env, p->getter, p->data, &local_getter)); } if (p->setter != nullptr) { - v8::Local setter_data = - v8impl::CreateFunctionCallbackData(env, p->setter, p->data); - CHECK_MAYBE_EMPTY(env, setter_data, napi_generic_failure); - - v8::MaybeLocal maybe_setter = - v8::Function::New(context, - v8impl::FunctionCallbackWrapper::Invoke, - setter_data); - CHECK_MAYBE_EMPTY(env, maybe_setter, napi_generic_failure); - local_setter = maybe_setter.ToLocalChecked(); + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction( + env, p->setter, p->data, &local_setter)); } v8::PropertyDescriptor descriptor(local_getter, local_setter); @@ -1304,19 +1210,10 @@ napi_status napi_define_properties(napi_env env, return napi_set_last_error(env, napi_invalid_arg); } } else if (p->method != nullptr) { - v8::Local cbdata = - v8impl::CreateFunctionCallbackData(env, p->method, p->data); - - CHECK_MAYBE_EMPTY(env, cbdata, napi_generic_failure); - - v8::MaybeLocal maybe_fn = - v8::Function::New(context, - v8impl::FunctionCallbackWrapper::Invoke, - cbdata); - - CHECK_MAYBE_EMPTY(env, maybe_fn, napi_generic_failure); - - v8::PropertyDescriptor descriptor(maybe_fn.ToLocalChecked(), + v8::Local method; + STATUS_CALL(v8impl::FunctionCallbackWrapper::NewFunction( + env, p->method, p->data, &method)); + v8::PropertyDescriptor descriptor(method, (p->attributes & napi_writable) != 0); descriptor.set_enumerable((p->attributes & napi_enumerable) != 0); descriptor.set_configurable((p->attributes & napi_configurable) != 0);