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

N-API: Reuse ObjectTemplate used for napi_wrap() #13999

Closed

Conversation

gabrielschulhof
Copy link
Contributor

V8 caches and does not subsequently release ObjectTemplate instances.
Thus, we need to store the ObjectTemplate from which we derive object
instances we use for napi_wrap() in a persistent in the napi_env.

Re nodejs/node-addon-api#70 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

N-API

@nodejs-github-bot 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 Jun 30, 2017
@gabrielschulhof
Copy link
Contributor Author

This means we will have one ObjectTemplate per N-API module, because we have one napi_env per N-API module.

@gabrielschulhof
Copy link
Contributor Author

Whoops! Wrong template :) BRB

@gabrielschulhof
Copy link
Contributor Author

Hmmm ... we actually use more than one ObjectTemplate. Need to store 'em all on the napi_env.

@gabrielschulhof
Copy link
Contributor Author

OK, now we're good. Three ObjectTemplate instances stored on the napi_env. So, three ObjectTemplate instances per module. Granted, they're lazy, so an ObjectInstance is only created if

  • a module does any wrapping (not necessarily though often)
  • a module implements accessors (perhaps more rarely)
  • a module exports native functions (almost always)

src/node_api.cc Outdated
v8::Local<v8::Object> wrapper =
wrapperTemplate->NewInstance(context).ToLocalChecked();
wrapper_template->NewInstance(context).ToLocalChecked();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you indent by 4 spaces?

V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

mhdawson commented Jul 6, 2017

@mhdawson
Copy link
Member

mhdawson commented Jul 6, 2017

Landed as 29df1a8

@mhdawson mhdawson closed this Jul 6, 2017
mhdawson pushed a commit that referenced this pull request Jul 6, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
@gabrielschulhof gabrielschulhof deleted the reuse-wrap-template branch July 6, 2017 18:39
addaleax pushed a commit that referenced this pull request Jul 11, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
@addaleax
Copy link
Member

@gabrielschulhof @mhdawson Just so you know, if you don’t provide the subsystem in the commit message in lowercase the tooling won’t group it properly in the changelog – not a big deal, just something for the future. :)

@addaleax addaleax mentioned this pull request Jul 11, 2017
@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017

if you don’t provide the subsystem in the commit message in lowercase the tooling won’t group it properly in the changelog – not a big deal, just something for the future. :)

If we don't have any uppercase subsystems (which I think is the case) then maybe core-validate-commit could check for that.

@evanlucas
Copy link
Contributor

it checks for a certain set of subsystems, so it should have failed for that as well.

  ✖  29df1a87fa230bc86f460620173b9fedd2919f68
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      metadata is at end of message             metadata-end
     ✔  0:0      reviewers are valid                       reviewers
     ✖  0:0      Invalid subsystem: "N-API"                subsystem
     ✔  0:0      Title is <= 50 columns.                   title-length

:]

addaleax pushed a commit that referenced this pull request Jul 18, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: nodejs#13999
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

Backport-PR-URL: #19447
PR-URL: #13999
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.