n-api: Re-use napi_env between modules #14217

gabrielschulhof · 2017-07-13T08:50:00Z

Create a std::map keyed on the isolate pointer and storing one napi_env
per isolate and protect it with a mutex.

Re: #13872 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

n-api

@bnoordhuis @mhdawson this is what sharing the napi_env would look like with a global static and a mutex.

bnoordhuis · 2017-07-13T09:21:03Z

src/node_api.cc

@@ -705,6 +706,27 @@ bool FindWrapper(v8::Local<v8::Object> obj,
  return true;
 }

+std::map<v8::Isolate*, napi_env> napi_env_map;
+uv_once_t napi_env_once = UV_ONCE_INIT;
+uv_mutex_t napi_env_mutex;


node::Mutex (and use node::Mutex::ScopedLock below), then you won't need a uv_once_t either.

bnoordhuis · 2017-07-13T09:27:51Z

src/node_api.cc

+  uv_mutex_lock(&napi_env_mutex);
+  result = napi_env_map[isolate];
+  if (result == nullptr) {
+    result = napi_env_map[isolate] = new napi_env__(isolate);


Are napi_env instances immortal? If not, there should be code somewhere to remove it from the map again.

I guess we need a refcount then, because we need to know, for each isolate, how many modules are using the napi_env.

We shall also need a v8impl::release_env() to dereference/delete. However, these only makes sense when we have a module unload strategy.

I have changed the comment about v8impl::get_env() to remind of this.

bnoordhuis · 2017-07-13T09:28:35Z

test/addons-napi/test_env_sharing/compare_env.c

+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &external, NULL, NULL));
+  NAPI_CALL(env, napi_get_value_external(env, external, &external_data));
+  NAPI_CALL(env,
+    napi_get_boolean(env, ((napi_env)external_data) == env, &return_value));


Style nit: can you line up the arguments in this file and others?

mhdawson · 2017-07-13T12:52:05Z

src/node_api.cc

@@ -718,7 +740,7 @@ void napi_module_register_cb(v8::Local<v8::Object> exports,

  // Create a new napi_env for this module. Once module unloading is supported
  // we shall have to call delete on this object from there.
-  napi_env env = new napi_env__(context->GetIsolate());
+  napi_env env = v8impl::get_env(context->GetIsolate());


The comment should probably change now as we'd no longer delete the env when a module is unloaded.

mhdawson · 2017-07-13T12:55:07Z

Can you change the title for the commit comment so that Re-use is lower case -> re-use

jasongin · 2017-07-13T16:56:30Z

I think this isn't going to work when modules static-link node_api.cc from node-addon-api to target node versions without N-API built-in. In that case each module would have a separate napi_env_map instance.

gabrielschulhof · 2017-07-19T12:43:44Z

@jasongin perhaps then we need a public API for per-isolate data that is better than what V8 offers.

gabrielschulhof · 2017-07-19T13:21:50Z

@bnoordhuis @mhdawson I have now addressed your review comments.

I'm not sure I should be implementing the napi_env refcounting until we actually have a solution for unloading modules.

As for the interoperability with statically linked node-addon-api versions mentioned by @jasongin, I think we've already broken that with the stricter wrapping, because we perform the type check based on the value of the string pointer, not its contents.

jasongin · 2017-07-19T18:45:23Z

I think we've already broken that with the stricter wrapping, because we perform the type check based on the value of the string pointer, not its contents.

I think that is not actually a problem, because there should never be a need for a module to unwrap an instance that was wrapped by a different module.

gabrielschulhof · 2017-07-19T21:42:18Z

@TimothyGu's idea about using v8::Private to attach private properties to objects gave me the idea that we can place the napi_env in an External and attach it to the global object using such a v8::Private hidden property. That would obviate the need for a solution such as the one in this PR, and it would solve the problem of modules loaded via various incarnations of node-addon-api.

TBH, if we could attach such hidden properties to the global object we'd essentially have the per-~~isolate~~context storage mechanism we need.

In terms of implementation, from https://github.com/nodejs/nan/blob/master/nan_private.h it looks like v8::Private is available in node 8 and node 6 but not node 4. So, for node-addon-api we'd have to #ifdef the way nan #ifdefs to cover both cases. This means that the contents of node_api.cc would irrevocably diverge between node and node-addon-api.

RFC @bnoordhuis @mhdawson @jasongin

gabrielschulhof · 2017-07-19T21:44:30Z

@jasongin I wouldn't dismiss the idea of cross-module wrapping/unwrapping so easily. I can imagine a scenario where we have two different packages depending on the same N-API module and the application passing objects originating from one to the other and vice versa.

gabrielschulhof · 2017-07-20T20:24:58Z

@mhdawson @bnoordhuis @jasongin I re-wrote this patch using @TimothyGu's idea regarding v8::Private. This seems to be a much better solution. It's cleaner, it does not use a mutex, and it'll work across both built-in and external versions of N-API, provided the external version is ABI-compatible with the internal version - that is, we keep node_api.cc well in sync in order to make sure that the contents of an instance of napi_env is the same both externally and internally.

gabrielschulhof · 2017-07-20T20:28:33Z

Oh, and it takes care of the eventuality of module unloading, and the eventuality of a module having to be loaded multiple times for multiple contexts as foretold by @bnoordhuis.

TimothyGu · 2017-07-21T03:29:06Z

src/node_api.cc

+  auto maybe_key = v8::String::NewFromUtf8(isolate, "N-API Environment",
+      v8::NewStringType::kInternalized);
+  CHECK(!maybe_key.IsEmpty());
+  auto key = v8::Private::ForApi(isolate, maybe_key.ToLocalChecked());


You should try to cache the key somehow to avoid having to create a new string and getting it from the V8 internal hash map on every run. GC is a non-concern as v8::Privates created through ForApi will never be GC'd.

That's a bit of a chicken-and-egg problem. I mean, I could create a std::map keyed on the isolate and store these v8::Privates but then I might as well go with the previous approach of storing the napi_env in the map's value.

This code runs once as part of DLopen() for a given module and then never again for that module. Given this, is it worth caching this value?

Actually, storing the napi_env instead of the v8::Private means I lose the ability to pass a napi_env created internally to node-addon-api, so I wouldn't revert to the previous approach. Still, I would only introduce a mutex if caching it really is worth it for the load-module use case.

If module loading performance isn't a concern, I wouldn't worry about it. FWIW it's also not necessary to use a mutex as ForApi is idempotent.

I thought the point of caching the v8::Private was to avoid calling ForApi more than once per new isolate. Or did you mean that I should cache the result of v8::String::NewFromOneByte?

If I use a std::map I must protect it with a mutex because another thread might be in the process of depositing a new value or it might be in the process of erasing an unused value.

I suppose you mean the kind of caching that I see in node.cc, like env->exports_string(). I guess maybe I should start looking into using node::Environment.

TimothyGu · 2017-07-21T03:29:27Z

src/node_api.cc

+  auto isolate = context->GetIsolate();
+  auto global = context->Global();
+
+  auto maybe_key = v8::String::NewFromUtf8(isolate, "N-API Environment",


NewFromOneByte would suffice.

TimothyGu · 2017-07-21T03:31:03Z

src/node_api.cc

+    auto external = v8::External::New(isolate, result);
+    auto maybe_set = global->SetPrivate(context, key, external);
+    CHECK(maybe_set.IsJust());
+    CHECK(maybe_set.FromJust());


These two checks can be combined as CHECK(maybe_set.FromJust(false)).

AFAICT FromJust() doesn't take any parameters. I tried to combine, but I get

In file included from ../src/node_internals.h:28:0, from ../src/node.h:173, from ../src/node_buffer.h:25, from ../src/node_api.cc:11: ../src/node_api.cc: In function ‘napi_env__* {anonymous}::v8impl::get_env(v8::Local<v8::Context>)’: ../src/node_api.cc:735:35: error: no matching function for call to ‘v8::Maybe<bool>::FromJust(bool)’ CHECK(maybe_set.FromJust(false)); ^ ../src/util.h:107:44: note: in definition of macro ‘UNLIKELY’ #define UNLIKELY(expr) __builtin_expect(!!(expr), 0) ^~~~ ../src/node_api.cc:735:5: note: in expansion of macro ‘CHECK’ CHECK(maybe_set.FromJust(false)); ^~~~~ In file included from ../src/node.h:63:0, from ../src/node_buffer.h:25, from ../src/node_api.cc:11: ../deps/v8/include/v8.h:8014:15: note: candidate: T v8::Maybe<T>::FromJust() const [with T = bool] V8_INLINE T FromJust() const { ^~~~~~~~ ../deps/v8/include/v8.h:8014:15: note: candidate expects 0 arguments, 1 provided

Oops I meant FromMaybe(false)

TimothyGu

LGTM. Don't worry about the Private thing, not sure what I was thinking.

TimothyGu · 2017-07-21T14:16:12Z

src/node_api.cc

+  delete data.GetParameter();
+}
+
+napi_env get_env(v8::Local<v8::Context> context) {


All other functions in the v8impl namespace uses CamelCase. Might be good to do that here etc.

TimothyGu · 2017-07-21T14:16:52Z

test/addons-napi/test_env_sharing/test.js

+'use strict';
+
+const common = require('../../common');
+const store_env = require(`./build/${common.buildType}/store_env`);


camelCase in JS

bnoordhuis

LGTM modulo style nits.

bnoordhuis · 2017-07-21T14:27:05Z

src/node_api.cc

+  auto global = context->Global();
+
+  auto maybe_key = v8::String::NewFromOneByte(isolate,
+      reinterpret_cast<const uint8_t *>("N-API Environment"),


Nit: no space before *

bnoordhuis · 2017-07-21T14:28:13Z

src/node_api.cc

+
+  auto maybe_value = global->GetPrivate(context, key);
+  CHECK(!maybe_value.IsEmpty());
+  auto value = maybe_value.ToLocalChecked();


Just an observation but .ToLocalChecked() also does the equivalent of CHECK(!maybe_value.IsEmpty()).

bnoordhuis · 2017-07-21T14:35:59Z

src/node_api.cc

+    auto external = v8::External::New(isolate, result);
+    auto maybe_set = global->SetPrivate(context, key, external);
+    CHECK(maybe_set.FromMaybe(false));
+    v8::Persistent<v8::External> persistent(isolate, external);


This works because ~Persistent() currently doesn't call .Reset() but v8.h indicates that may change in the future. Not sure what to do about that but I figured you should know.

bnoordhuis · 2017-07-21T14:36:27Z

test/addons-napi/test_env_sharing/compare_env.c

+
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &external, NULL, NULL));
+  NAPI_CALL(env, napi_get_value_external(env, external, &data));
+  NAPI_CALL(env, napi_get_boolean(env, ((napi_env)data) == env, &return_value));


static_cast?

gabrielschulhof · 2017-07-21T20:46:13Z

@TimothyGu I have updated to camelCase.

@bnoordhuis I have removed the Persistent and replaced it with one of our self-destructing v8impl::Reference objects.

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Re: nodejs#13872 (comment)

gabrielschulhof · 2017-07-22T08:47:18Z

I changed the commit message subject to contain no uppercase letters.

gabrielschulhof · 2017-07-25T18:23:56Z

If this is good to go, may I have a CI run please?

TimothyGu · 2017-07-26T00:39:26Z

Seems like the CI is having some issues, though.

gabrielschulhof · 2017-07-26T17:35:41Z

I think the CI is fixed now. Could somebody please submit this PR?

gabrielschulhof · 2017-07-26T17:36:05Z

Oh, NM. Thanks!

gabrielschulhof · 2017-07-26T23:13:31Z

https://ci.nodejs.org/job/node-test-pull-request/9351/ completed successfully a while ago. It shouldn't still show as pending in this PR.

addaleax · 2017-07-26T23:15:30Z

@gabrielschulhof CI status reporting is broken, don’t worry about it (not sure whether somebody’s looking into it or not).

gabrielschulhof · 2017-07-26T23:18:44Z

@addaleax NP. I'm just eager to get this landed, that's all :)

addaleax · 2017-07-26T23:23:33Z

I’m going to sleep soon but if it’s not landed tomorrow I can do that.

Also, just nominated you for push & CI access, that should be a bit more practical if you like :)

gabrielschulhof · 2017-07-26T23:59:19Z

Wow! I'm honoured! Thanks!

TimothyGu · 2017-07-26T23:59:51Z

Landed in 9926dfe.

I also amended the commit message to reference #14367 rather than #13872 (comment), as the approach we ended up taking was documented in #14367.

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Refs: #14367 PR-URL: #14217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

TimothyGu · 2017-07-27T00:00:32Z

And also, I'd second Anna's nomination :)

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Refs: #14367 PR-URL: #14217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Refs: nodejs#14367 PR-URL: nodejs#14217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Refs: #14367 Backport-PR-URL: #19447 PR-URL: #14217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 13, 2017

bnoordhuis requested changes Jul 13, 2017

View reviewed changes

mhdawson reviewed Jul 13, 2017

View reviewed changes

gabrielschulhof force-pushed the shared_env branch 2 times, most recently from 248260d to 6a58919 Compare July 19, 2017 13:12

gabrielschulhof force-pushed the shared_env branch from 6a58919 to f7174d0 Compare July 20, 2017 20:21

TimothyGu reviewed Jul 21, 2017

View reviewed changes

gabrielschulhof force-pushed the shared_env branch 3 times, most recently from c775e90 to 9ff9cf7 Compare July 21, 2017 14:04

TimothyGu approved these changes Jul 21, 2017

View reviewed changes

bnoordhuis approved these changes Jul 21, 2017

View reviewed changes

gabrielschulhof force-pushed the shared_env branch from 9ff9cf7 to d057bfc Compare July 21, 2017 20:43

TimothyGu approved these changes Jul 22, 2017

View reviewed changes

n-api: re-use napi_env between modules

cdb7f13

Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Re: nodejs#13872 (comment)

gabrielschulhof force-pushed the shared_env branch from d057bfc to cdb7f13 Compare July 22, 2017 08:46

TimothyGu closed this Jul 26, 2017

gabrielschulhof deleted the shared_env branch July 27, 2017 00:08

kfarnung mentioned this pull request Aug 1, 2017

Cache napi_env per runtime nodejs/node-chakracore#353

Open

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the dont-land-on-v6.x label Aug 16, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

n-api: Re-use napi_env between modules #14217

n-api: Re-use napi_env between modules #14217

Conversation

gabrielschulhof commented Jul 13, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jul 13, 2017

jasongin commented Jul 13, 2017

gabrielschulhof commented Jul 19, 2017

gabrielschulhof commented Jul 19, 2017

jasongin commented Jul 19, 2017

gabrielschulhof commented Jul 19, 2017

gabrielschulhof commented Jul 19, 2017

gabrielschulhof commented Jul 20, 2017

gabrielschulhof commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Jul 21, 2017

gabrielschulhof commented Jul 22, 2017 • edited Loading

gabrielschulhof commented Jul 25, 2017

TimothyGu commented Jul 26, 2017

gabrielschulhof commented Jul 26, 2017

gabrielschulhof commented Jul 26, 2017

gabrielschulhof commented Jul 26, 2017

addaleax commented Jul 26, 2017

gabrielschulhof commented Jul 26, 2017

addaleax commented Jul 26, 2017

gabrielschulhof commented Jul 26, 2017

TimothyGu commented Jul 26, 2017

TimothyGu commented Jul 27, 2017

gabrielschulhof Jul 21, 2017 •

edited

Loading

gabrielschulhof commented Jul 22, 2017 •

edited

Loading