Using v8::Private::ForApi for N-API object wrapping

[TimothyGu]

• Version: master
• Platform: all
• Subsystem: n-api

Currently, N-API's object wrapping uses v8::External with a const char* as a brand check. However, v8::Externals are known to be inefficient, and the fact that one such External is created for every wrapped object only worsens the problem. On the other hand, V8 provides the mechanism necessary for cross-napi_env brand checks in v8::Private::ForApi, which is like a C++-private version of Symbol.for(), and v8::Object::HasPrivate/SetPrivate. Handles to these v8::Privates can then be cached per napi_env in a Persistent. Similar features are also available in earlier V8 versions, as implemented in @kkoopa's NAN.

I'd be willing to cook up a PR for this.

/cc @nodejs/n-api @gabrielschulhof @jasongin

[gabrielschulhof]

@TimothyGu

I'm thinking that we can use this mechanism to attach a hidden property to isolate->GetCurrentContext()->Global() such that its value is an External containing the napi_env. That way we could achieve napi_env sharing across modules without the need for a std::map and a node::Mutex.

How kosher is it to attach such hidden properties to the global object?

[addaleax]

@gabrielschulhof I’m not sure how that relates to the issue above, but in any case: That sounds fine as long as you’re aware that Node code can run in different contexts with different globals.

[gabrielschulhof]

@addaleax are modules loaded per-context or per-isolate? That is, if there is one isolate with two different contexts, can the same module instance be used in both?

The relation to this issue is that ideally, we'd type-check via FunctionTemplate::HasInstance() and we'd store FunctionTemplates in the napi_env for the various object instances we need to create. We currently store ObjectTemplate instances in the napi_env, and that forces us to use weaker type checking.

However, we need to make sure that we never end up with a situation where an instance created from one FunctionTemplate in one module ends up being used in a different module wherein there is another napi_env with its own copy of a FunctionTemplate serving the same purpose, because then HasInstance() will return false even though it should return true because conceptually the instance is an instance of the FunctionTemplate, just the one from a different napi_env.

[gabrielschulhof]

Aha!

// FIXME(bnoordhuis) Not multi-context ready. TBD how to resolve the conflict
// when two contexts try to load the same shared object. Maybe have a shadow
// cache that's a plain C list or hash table that's shared across contexts?

[TimothyGu]

@gabrielschulhof Well, can't comment about the process of dynamic loading from different contexts, but v8::Private::ForApi() is isolate-global so the v8::Private you get in one context is the exact same as the v8::Private you get from another context.

About reusing napi_env for the purpose of using FunctionTemplate::HasInstance, beware that IIRC HasInstance doesn't check prototypes of the object like n-api currently does.

I don't see any problem with attaching hidden properties to the global object. Not like hidden properties are, you know, visible from JS, or even C++ if the consumer is not actively trying to find it.

[gabrielschulhof]

@addaleax under what circumstances does Node create multiple contexts and run JS code in them?

[addaleax]

@gabrielschulhof It’s programatically available in the built-in vm module, anybody can create new contexts as they wish

[bnoordhuis]

It's also used in projects like electron and nw.js that create a context per tab.

[TimothyGu]

@gabrielschulhof I think this issue could be closed now, right?

[gabrielschulhof]

Actually, `napi_wrap()` isn't using it yet. It's only being used for the `napi_env` so far.

…

On Wed, Aug 9, 2017 at 5:06 AM, Timothy Gu ***@***.***> wrote: @gabrielschulhof <https://github.com/gabrielschulhof> I think this issue could be closed now, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14367 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0cWjB9eUkyYFHtARLc9pnKZnGVjtks5sWRQSgaJpZM4OcyJd> .