Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

napi: create napi_env as a real structure tied to the isolate #206

Conversation

gabrielschulhof
Copy link
Collaborator

  1. We define struct napi_env__ to include the isolate, the last
    exception, and the info about the last error.

  2. We instantiate one struct napi_env__ during module registration, but
    only if there isn't one already attached to the isolate, perhaps from
    the registration of a pervious module.

  3. the last exception and the info about the last error are stored in
    the napi_env__ struct which is associated with the isolate via
    v8::Isolate::SetData().

Fixes #198

@gabrielschulhof gabrielschulhof force-pushed the 198-per-isolate-environment branch from 0617d8d to 7fdbdf6 Compare March 28, 2017 22:25
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this pull request Mar 28, 2017
1. We define struct napi_env__ to include the isolate, the last
exception, and the info about the last error.

2. We instantiate one struct napi_env__ during module registration, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
@gabrielschulhof
Copy link
Collaborator Author

addaleax

This comment was marked as off-topic.

@boingoing
Copy link

Looks pretty good and I think we could stash additional persistents in struct napi_env for things like caching Symbol.hasInstance.

@gabrielschulhof
Copy link
Collaborator Author

@jasongin @addaleax what about simply using a global std::map<Isolate*, napi_env> in src/node_api.cc for now? After all, it's almost certain that only one item will be added to that map, and there never will be thousands of items added to it.

@gabrielschulhof gabrielschulhof force-pushed the 198-per-isolate-environment branch from 7fdbdf6 to 8de9c90 Compare March 29, 2017 06:59
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this pull request Mar 29, 2017
1. We define struct napi_env__ to include the isolate, the last
exception, and the info about the last error.

2. We instantiate one struct napi_env__ during module registration, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
@gabrielschulhof
Copy link
Collaborator Author

gabrielschulhof commented Mar 29, 2017

Also, given @mhdawson's comment at #204 (comment) about not necessarily tying napi_env one-to-one to Isolate* I would say that having the std::map would be good, because different implementations could use different keys instead of Isolate*, but the bottom line is they all may have to keep track of and distinguish between multiple instances of napi_env.

For now, I think we can tie to Isolate* one-to-one, but if we implement the mapping using a std::map then we can change the mapping later on without modifications to node.

@gabrielschulhof
Copy link
Collaborator Author

gabrielschulhof commented Mar 29, 2017

OTOH, can we have one napi_env per module? If so, I believe we might be able to get away without tacking anything on to the Isolate* or onto anything else. After all, there are only two places where control passes into N-API:

  1. Module registration
  2. The thunk

So, if we create the napi_env during module registration, it comes back to us during napi_create_function(), and we can store it in the FunctionCallbackData for all functions we create.

If we go this way, we may end up creating multiple napi_env objects with exactly the same content, especially since we know that for now there is only one Isolate*, but OTOH we get complete independence from any global structure and we require only the opportunity to add context to functions we expose to JS - which is indispensable anyway, and which V8 provides to us with style via SetInternalField().

@gabrielschulhof
Copy link
Collaborator Author

I've created an alternative implementation using the per-module napi_env:
f4bbfee...gabrielschulhof:198-per-module-environment
The trade-off is that we are creating more napi_env objects than necessary because we are creating them per-module rather than per-whatever-it-is-in-the-environment-we-should-key-to, OTOH we are divorcing ourselves even further from node, because we encapsulate all that we need from node (the Isolate*) in one place and one place alone.

This assumes of course that, once you expose a function to JS, it will always be called with the same Isolate* and that if it is to be called with a different Isolate* then the module will be re-registered using that Isolate*. Even if not, though, we can simply re-initialize our exising napi_env before we call the napi_callback:

...
env->isolate = isolate;
cb(env, cbinfo_wrapper);
...

gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this pull request Mar 29, 2017
1. We define struct napi_env__ to include the isolate, the last
exception, and the info about the last error.

2. We instantiate one struct napi_env__ during module registration, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
@gabrielschulhof gabrielschulhof force-pushed the 198-per-isolate-environment branch from 8de9c90 to 920ed54 Compare March 29, 2017 08:32
@gabrielschulhof
Copy link
Collaborator Author

@gabrielschulhof
Copy link
Collaborator Author

Actually, since we can't really go with isolate->GetData(<anything>) and we can't modify node because we want this to work with versions of node we haven't modified, I think the only approach is to go with the per-module environment.

1. We define struct napi_env__ to include the isolate, the last
exception, and the info about the last error.

2. We instantiate one struct napi_env__ during module registration, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
@gabrielschulhof gabrielschulhof force-pushed the 198-per-isolate-environment branch from 920ed54 to 5f0c112 Compare March 29, 2017 10:27
@gabrielschulhof
Copy link
Collaborator Author

@gabrielschulhof
Copy link
Collaborator Author

@gabrielschulhof gabrielschulhof force-pushed the 198-per-isolate-environment branch from e5e21c0 to 72a2efb Compare March 29, 2017 10:50
@gabrielschulhof
Copy link
Collaborator Author

@jasongin
Copy link
Member

Can we close this PR? I think #208 is a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants