nodejs · gabrielschulhof · Jun 7, 2018
diff --git a/Makefile b/Makefile
@@ -260,6 +260,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
@@ -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
@@ -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
@@ -0,0 +1,28 @@
+#include <assert.h>
+#include <node_api.h>
+
+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
@@ -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<v8::Value> last_exception;
-  v8::Persistent<v8::ObjectTemplate> wrap_template;
-  v8::Persistent<v8::ObjectTemplate> function_data_template;
-  v8::Persistent<v8::ObjectTemplate> 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<v8::ObjectTemplate>::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<v8::Value> 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<v8::Value> handle;  // Die with this JavaScript object
+
+ private:
+  static void WeakCallback(v8::WeakCallbackInfo<CallbackBundle> 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<v8::Object>::Cast(cbinfo.Data())) {
-    _data = v8::Local<v8::External>::Cast(_cbdata->GetInternalField(kDataIndex))
-                ->Value();
+        _cbinfo(cbinfo) {
+    _bundle = reinterpret_cast<CallbackBundle*>(
+        v8::Local<v8::External>::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<napi_callback_info>(
         static_cast<CallbackWrapper*>(this));
-    napi_callback cb = reinterpret_cast<napi_callback>(
-        v8::Local<v8::External>::Cast(
-            _cbdata->GetInternalField(kInternalFieldIndex))->Value());
 
-    napi_env env = static_cast<napi_env>(
-        v8::Local<v8::External>::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<v8::Object> _cbdata;
+  CallbackBundle* _bundle;
 };
 
 class FunctionCallbackWrapper
@@ -701,62 +715,35 @@ 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<v8::Object> CreateFunctionCallbackData(napi_env env,
-                                                 napi_callback cb,
-                                                 void* data) {
-  v8::Isolate* isolate = env->isolate;
-  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+v8::Local<v8::Value> 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<v8::Value> cbdata = v8::External::New(env->isolate, bundle);
+  bundle->BindLifecycleTo(env->isolate, cbdata);
 
-  v8::Local<v8::ObjectTemplate> otpl;
-  ENV_OBJECT_TEMPLATE(env, function_data, otpl, v8impl::kFunctionFieldCount);
-  v8::Local<v8::Object> cbdata = otpl->NewInstance(context).ToLocalChecked();
-
-  cbdata->SetInternalField(
-      v8impl::kEnvIndex,
-      v8::External::New(isolate, static_cast<void*>(env)));
-  cbdata->SetInternalField(
-      v8impl::kFunctionIndex,
-      v8::External::New(isolate, reinterpret_cast<void*>(cb)));
-  cbdata->SetInternalField(
-      v8impl::kDataIndex,
-      v8::External::New(isolate, data));
   return cbdata;
 }
 
 // Creates an object to be made available to the static getter/setter
 // callback wrapper, used to retrieve the native getter/setter callback
 // function and data pointer.
 static
-v8::Local<v8::Object> CreateAccessorCallbackData(napi_env env,
-                                                 napi_callback getter,
-                                                 napi_callback setter,
-                                                 void* data) {
-  v8::Isolate* isolate = env->isolate;
-  v8::Local<v8::Context> context = isolate->GetCurrentContext();
-
-  v8::Local<v8::ObjectTemplate> otpl;
-  ENV_OBJECT_TEMPLATE(env, accessor_data, otpl, v8impl::kAccessorFieldCount);
-  v8::Local<v8::Object> cbdata = otpl->NewInstance(context).ToLocalChecked();
-
-  cbdata->SetInternalField(
-      v8impl::kEnvIndex,
-      v8::External::New(isolate, static_cast<void*>(env)));
-
-  if (getter != nullptr) {
-    cbdata->SetInternalField(
-        v8impl::kGetterIndex,
-        v8::External::New(isolate, reinterpret_cast<void*>(getter)));
-  }
-
-  if (setter != nullptr) {
-    cbdata->SetInternalField(
-        v8impl::kSetterIndex,
-        v8::External::New(isolate, reinterpret_cast<void*>(setter)));
-  }
+v8::Local<v8::Value> 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<v8::Value> 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<v8::Function> return_value;
   v8::EscapableHandleScope scope(isolate);
-  v8::Local<v8::Object> cbdata =
+  v8::Local<v8::Value> 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<v8::Object> cbdata =
+  v8::Local<v8::Value> 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<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
+      v8::Local<v8::Value> 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<v8::Object> cbdata =
+      v8::Local<v8::Value> 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<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
+      v8::Local<v8::Value> 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<v8::Object> cbdata =
+      v8::Local<v8::Value> cbdata =
           v8impl::CreateFunctionCallbackData(env, p->method, p->data);
 
       RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);