Skip to content

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

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:reuse-wrap-template
Closed

N-API: Reuse ObjectTemplate used for napi_wrap()#13999
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:reuse-wrap-template

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Jun 30, 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() 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

gabrielschulhof commented Jun 30, 2017

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

gabrielschulhof commented Jun 30, 2017

Whoops! Wrong template :) BRB

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Jun 30, 2017

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

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Jun 30, 2017

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)

bnoordhuis
bnoordhuis approved these changes Jun 30, 2017
View reviewed changes
src/node_api.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Jun 30, 2017

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?

N-API: Reuse ObjectTemplate instances
4ec417d 
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)
mhdawson
mhdawson approved these changes Jul 6, 2017
View reviewed changes
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

CI run: https://ci.nodejs.org/job/node-test-pull-request/9010/

@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
@mhdawson
N-API: Reuse ObjectTemplate instances
29df1a8 
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
@addaleax
N-API: Reuse ObjectTemplate instances
f56365e 
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

addaleax commented Jul 11, 2017

@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
Merged
@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

evanlucas commented Jul 11, 2017

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
@addaleax
N-API: Reuse ObjectTemplate instances
53c52ac 
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
@Fishrock123
N-API: Reuse ObjectTemplate instances
0bce0ab 
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
N-API: Reuse ObjectTemplate instances
35b3c61 
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
@MylesBorins
N-API: Reuse ObjectTemplate instances
6ba38e8 
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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@digitalinfinity digitalinfinity digitalinfinity approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.