From 4911c4e9fa4d97c85be8e20ef3cdefb8ba301719 Mon Sep 17 00:00:00 2001 From: Kenny Yuan Date: Thu, 7 Jun 2018 10:01:47 +0800 Subject: [PATCH] n-api: improve runtime perf of n-api func call Added a new struct CallbackBundle to eliminate all GetInternalField() calls. The principle is to store all required data inside a C++ struct, and then store the pointer in the JavaScript object. Before this change, the required data are stored in the JavaScript object in 3 or 4 seperate pointers. For every napi fun call, 3 of them have to be fetched out, which are 3 GetInternalField() calls; after this change, the C++ struct will be directly fetched out by using v8::External::Value(), which is faster. Profiling data show that GetInternalField() is slow. On an i7-4770K (3.50GHz) box, a C++ V8-binding fun call is 8 ns, before this change, napi fun call is 36 ns; after this change, napi fun call is 20 ns. The above data are measured using a modified benchmark in 'benchmark/misc/function_call'. The modification adds an indicator of the average time of a "chatty" napi fun call (max 50M runs). This change will speed up chatty case 1.8x (overall), and will cut down the delay of napi mechanism to approx. 0.5x. Background: a simple C++ binding function (e.g. receiving little from JS, doing little and returning little to JS) is called 'chatty' case for JS<-->C++ fun call routine. This improvement also applies to getter/setter fun calls. Backport-PR-URL: https://github.com/nodejs/node/pull/21733 PR-URL: https://github.com/nodejs/node/pull/21072 Reviewed-By: Anna Henningsen Reviewed-By: Gabriel Schulhof --- Makefile | 1 + benchmark/misc/function_call/binding.gyp | 4 + benchmark/misc/function_call/index.js | 13 +- benchmark/misc/function_call/napi_binding.c | 28 ++++ src/node_api.cc | 167 +++++++++----------- 5 files changed, 121 insertions(+), 92 deletions(-) create mode 100644 benchmark/misc/function_call/napi_binding.c diff --git a/Makefile b/Makefile index 9e1dbdbded9f60..93e1831421de07 100644 --- a/Makefile +++ b/Makefile @@ -263,6 +263,7 @@ test-check-deopts: all $(PYTHON) tools/test.py --mode=release --check-deopts parallel sequential -J benchmark/misc/function_call/build/Release/binding.node: all \ + benchmark/misc/function_call/napi_binding.c \ benchmark/misc/function_call/binding.cc \ benchmark/misc/function_call/binding.gyp $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \ diff --git a/benchmark/misc/function_call/binding.gyp b/benchmark/misc/function_call/binding.gyp index 3bfb84493f3e87..ac122ed1a07962 100644 --- a/benchmark/misc/function_call/binding.gyp +++ b/benchmark/misc/function_call/binding.gyp @@ -1,5 +1,9 @@ { 'targets': [ + { + 'target_name': 'napi_binding', + 'sources': [ 'napi_binding.c' ] + }, { 'target_name': 'binding', 'sources': [ 'binding.cc' ] diff --git a/benchmark/misc/function_call/index.js b/benchmark/misc/function_call/index.js index 6a2595d2ae188d..b377a0039a9067 100644 --- a/benchmark/misc/function_call/index.js +++ b/benchmark/misc/function_call/index.js @@ -19,6 +19,15 @@ try { } const cxx = binding.hello; +let napi_binding; +try { + napi_binding = require('./build/Release/napi_binding'); +} catch (er) { + console.error('misc/function_call/index.js NAPI-Binding failed to load'); + process.exit(0); +} +const napi = napi_binding.hello; + var c = 0; function js() { return c++; @@ -27,14 +36,14 @@ function js() { assert(js() === cxx()); const bench = common.createBenchmark(main, { - type: ['js', 'cxx'], + type: ['js', 'cxx', 'napi'], millions: [1, 10, 50] }); function main(conf) { const n = +conf.millions * 1e6; - const fn = conf.type === 'cxx' ? cxx : js; + const fn = conf.type === 'cxx' ? cxx : conf.type === 'napi' ? napi : js; bench.start(); for (var i = 0; i < n; i++) { fn(); diff --git a/benchmark/misc/function_call/napi_binding.c b/benchmark/misc/function_call/napi_binding.c new file mode 100644 index 00000000000000..4becc7f8371278 --- /dev/null +++ b/benchmark/misc/function_call/napi_binding.c @@ -0,0 +1,28 @@ +#include +#include + +static int32_t increment = 0; + +static napi_value Hello(napi_env env, napi_callback_info info) { + napi_value result; + napi_status status = napi_create_int32(env, increment++, &result); + assert(status == napi_ok); + return result; +} + +static napi_value Init(napi_env env, napi_value exports) { + napi_value hello; + napi_status status = + napi_create_function(env, + "hello", + NAPI_AUTO_LENGTH, + Hello, + NULL, + &hello); + assert(status == napi_ok); + status = napi_set_named_property(env, exports, "hello", hello); + assert(status == napi_ok); + return exports; +} + +NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/src/node_api.cc b/src/node_api.cc index 6990d1ccd904cf..ce54e22d55440e 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -23,15 +23,9 @@ struct napi_env__ { loop(_loop) {} ~napi_env__() { last_exception.Reset(); - wrap_template.Reset(); - function_data_template.Reset(); - accessor_data_template.Reset(); } v8::Isolate* isolate; v8::Persistent last_exception; - v8::Persistent wrap_template; - v8::Persistent function_data_template; - v8::Persistent accessor_data_template; napi_extended_error_info last_error; int open_handle_scopes = 0; int open_callback_scopes = 0; @@ -41,19 +35,6 @@ struct napi_env__ { #define NAPI_PRIVATE_KEY(context, suffix) \ (node::Environment::GetCurrent((context))->napi_ ## suffix()) -#define ENV_OBJECT_TEMPLATE(env, prefix, destination, field_count) \ - do { \ - if ((env)->prefix ## _template.IsEmpty()) { \ - (destination) = v8::ObjectTemplate::New(isolate); \ - (destination)->SetInternalFieldCount((field_count)); \ - (env)->prefix ## _template.Reset(isolate, (destination)); \ - } else { \ - (destination) = v8::Local::New( \ - isolate, env->prefix ## _template); \ - } \ - } while (0) - - #define RETURN_STATUS_IF_FALSE(env, condition, status) \ do { \ if (!(condition)) { \ @@ -510,15 +491,51 @@ class TryCatch : public v8::TryCatch { //=== Function napi_callback wrapper ================================= -static const int kDataIndex = 0; -static const int kEnvIndex = 1; +// TODO(somebody): these constants can be removed with relevant changes +// in CallbackWrapperBase<> and CallbackBundle. +// Leave them for now just to keep the change set and cognitive load minimal. +static const int kFunctionIndex = 0; // Used in CallbackBundle::cb[] +static const int kGetterIndex = 0; // Used in CallbackBundle::cb[] +static const int kSetterIndex = 1; // Used in CallbackBundle::cb[] +static const int kCallbackCount = 2; // Used in CallbackBundle::cb[] + // Max is "getter + setter" case + +// Use this data structure to associate callback data with each N-API function +// exposed to JavaScript. The structure is stored in a v8::External which gets +// passed into our callback wrapper. This reduces the performance impact of +// calling through N-API. +// Ref: benchmark/misc/function_call +// Discussion (incl. perf. data): https://github.com/nodejs/node/pull/21072 +class CallbackBundle { + public: + ~CallbackBundle() { + handle.ClearWeak(); + handle.Reset(); + } -static const int kFunctionIndex = 2; -static const int kFunctionFieldCount = 3; + // Bind the lifecycle of `this` C++ object to a JavaScript object. + // We never delete a CallbackBundle C++ object directly. + void BindLifecycleTo(v8::Isolate* isolate, v8::Local target) { + handle.Reset(isolate, target); + handle.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); + } -static const int kGetterIndex = 2; -static const int kSetterIndex = 3; -static const int kAccessorFieldCount = 4; + napi_env env; // Necessary to invoke C++ NAPI callback + void* cb_data; // The user provided callback data + napi_callback cb[kCallbackCount]; // Max capacity is 2 (getter + setter) + v8::Persistent handle; // Die with this JavaScript object + + private: + static void WeakCallback(v8::WeakCallbackInfo const& info) { + // Use the "WeakCallback mechanism" to delete the C++ `bundle` object. + // This will be called when the v8::External containing `this` pointer + // is being GC-ed. + CallbackBundle* bundle = info.GetParameter(); + if (bundle != nullptr) { + delete bundle; + } + } +}; // Base class extended by classes that wrap V8 function and property callback // info. @@ -550,10 +567,10 @@ class CallbackWrapperBase : public CallbackWrapper { : CallbackWrapper(JsValueFromV8LocalValue(cbinfo.This()), args_length, nullptr), - _cbinfo(cbinfo), - _cbdata(v8::Local::Cast(cbinfo.Data())) { - _data = v8::Local::Cast(_cbdata->GetInternalField(kDataIndex)) - ->Value(); + _cbinfo(cbinfo) { + _bundle = reinterpret_cast( + v8::Local::Cast(cbinfo.Data())->Value()); + _data = _bundle->cb_data; } napi_value GetNewTarget() override { return nullptr; } @@ -562,13 +579,10 @@ class CallbackWrapperBase : public CallbackWrapper { void InvokeCallback() { napi_callback_info cbinfo_wrapper = reinterpret_cast( static_cast(this)); - napi_callback cb = reinterpret_cast( - v8::Local::Cast( - _cbdata->GetInternalField(kInternalFieldIndex))->Value()); - napi_env env = static_cast( - v8::Local::Cast( - _cbdata->GetInternalField(kEnvIndex))->Value()); + // All other pointers we need are stored in `_bundle` + napi_env env = _bundle->env; + napi_callback cb = _bundle->cb[kInternalFieldIndex]; napi_value result; NAPI_CALL_INTO_MODULE_THROW(env, result = cb(env, cbinfo_wrapper)); @@ -579,7 +593,7 @@ class CallbackWrapperBase : public CallbackWrapper { } const Info& _cbinfo; - const v8::Local _cbdata; + CallbackBundle* _bundle; }; class FunctionCallbackWrapper @@ -701,25 +715,16 @@ class SetterCallbackWrapper // 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) { - v8::Isolate* isolate = env->isolate; - v8::Local context = isolate->GetCurrentContext(); +v8::Local CreateFunctionCallbackData(napi_env env, + napi_callback cb, + void* data) { + CallbackBundle* bundle = new CallbackBundle(); + bundle->cb[kFunctionIndex] = cb; + bundle->cb_data = data; + bundle->env = env; + v8::Local cbdata = v8::External::New(env->isolate, bundle); + bundle->BindLifecycleTo(env->isolate, cbdata); - v8::Local otpl; - ENV_OBJECT_TEMPLATE(env, function_data, otpl, v8impl::kFunctionFieldCount); - v8::Local cbdata = otpl->NewInstance(context).ToLocalChecked(); - - cbdata->SetInternalField( - v8impl::kEnvIndex, - v8::External::New(isolate, static_cast(env))); - cbdata->SetInternalField( - v8impl::kFunctionIndex, - v8::External::New(isolate, reinterpret_cast(cb))); - cbdata->SetInternalField( - v8impl::kDataIndex, - v8::External::New(isolate, data)); return cbdata; } @@ -727,36 +732,18 @@ v8::Local CreateFunctionCallbackData(napi_env env, // callback wrapper, used to retrieve the native getter/setter callback // function and data pointer. static -v8::Local CreateAccessorCallbackData(napi_env env, - napi_callback getter, - napi_callback setter, - void* data) { - v8::Isolate* isolate = env->isolate; - v8::Local context = isolate->GetCurrentContext(); - - v8::Local otpl; - ENV_OBJECT_TEMPLATE(env, accessor_data, otpl, v8impl::kAccessorFieldCount); - v8::Local cbdata = otpl->NewInstance(context).ToLocalChecked(); - - cbdata->SetInternalField( - v8impl::kEnvIndex, - v8::External::New(isolate, static_cast(env))); - - if (getter != nullptr) { - cbdata->SetInternalField( - v8impl::kGetterIndex, - v8::External::New(isolate, reinterpret_cast(getter))); - } - - if (setter != nullptr) { - cbdata->SetInternalField( - v8impl::kSetterIndex, - v8::External::New(isolate, reinterpret_cast(setter))); - } +v8::Local CreateAccessorCallbackData(napi_env env, + napi_callback getter, + napi_callback setter, + void* data) { + CallbackBundle* bundle = new CallbackBundle(); + bundle->cb[kGetterIndex] = getter; + bundle->cb[kSetterIndex] = setter; + bundle->cb_data = data; + bundle->env = env; + v8::Local cbdata = v8::External::New(env->isolate, bundle); + bundle->BindLifecycleTo(env->isolate, cbdata); - cbdata->SetInternalField( - v8impl::kDataIndex, - v8::External::New(isolate, data)); return cbdata; } @@ -1025,7 +1012,7 @@ napi_status napi_create_function(napi_env env, v8::Isolate* isolate = env->isolate; v8::Local return_value; v8::EscapableHandleScope scope(isolate); - v8::Local cbdata = + v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, cb, callback_data); RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); @@ -1065,7 +1052,7 @@ napi_status napi_define_class(napi_env env, v8::Isolate* isolate = env->isolate; v8::EscapableHandleScope scope(isolate); - v8::Local cbdata = + v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, constructor, callback_data); RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); @@ -1101,7 +1088,7 @@ napi_status napi_define_class(napi_env env, // This code is similar to that in napi_define_properties(); the // difference is it applies to a template instead of an object. if (p->getter != nullptr || p->setter != nullptr) { - v8::Local cbdata = v8impl::CreateAccessorCallbackData( + v8::Local cbdata = v8impl::CreateAccessorCallbackData( env, p->getter, p->setter, p->data); tpl->PrototypeTemplate()->SetAccessor( @@ -1112,7 +1099,7 @@ napi_status napi_define_class(napi_env env, v8::AccessControl::DEFAULT, attributes); } else if (p->method != nullptr) { - v8::Local cbdata = + v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, p->method, p->data); RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); @@ -1474,7 +1461,7 @@ napi_status napi_define_properties(napi_env env, v8impl::V8PropertyAttributesFromDescriptor(p); if (p->getter != nullptr || p->setter != nullptr) { - v8::Local cbdata = v8impl::CreateAccessorCallbackData( + v8::Local cbdata = v8impl::CreateAccessorCallbackData( env, p->getter, p->setter, @@ -1493,7 +1480,7 @@ 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 = + v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, p->method, p->data); RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);