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: Improve performance creating strings #26439

Closed
wants to merge 4 commits into from
Closed

napi: Improve performance creating strings #26439

wants to merge 4 commits into from

Conversation

anthony-tuininga
Copy link
Contributor

@anthony-tuininga anthony-tuininga commented Mar 4, 2019

Improve performance creating strings using N-API by ensuring that the strings are not internalized (see Node issue #26437).

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

… strings

are not internalized (see Node issue #26437).
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 4, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you!

@addaleax addaleax added the node-api Issues and PRs related to the Node-API. label Mar 4, 2019
@addaleax
Copy link
Member

addaleax commented Mar 4, 2019

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

(Travis is going to complain about the commit message, but you can ignore that, it’s information for the person who lands the commit, not necessarily the author.)

@mhdawson
Copy link
Member

mhdawson commented Mar 4, 2019

This seems to confirm the preference for kNormal as the safer default choice. https://groups.google.com/forum/#!topic/v8-users/xoqi4ee8x74

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 mhdawson added performance Issues and PRs related to the performance of Node.js. lts-watch-v8.x labels Mar 4, 2019
@mhdawson
Copy link
Member

mhdawson commented Mar 4, 2019

@anthony-tuininga thanks for your work on this.

@anthony-tuininga
Copy link
Contributor Author

You're welcome. Thanks for the quick turnaround on the approvals! Would love to see this released in 8.x and 10.x as well so that our users won't see a performance hit when upgrading to the next version of our module (in which we hope to use N-API).

Let me know if you want me to add the checks in the other string creation methods yet or if that should be deferred for another issue and PR?

@addaleax
Copy link
Member

addaleax commented Mar 5, 2019

Let me know if you want me to add the checks in the other string creation methods yet or if that should be deferred for another issue and PR?

If it’s all the same to you, I’d rather do it now/here. :)

@anthony-tuininga
Copy link
Contributor Author

Ok. I'll add the checks here. One moment.

@mhdawson
Copy link
Member

mhdawson commented Mar 5, 2019

@anthony-tuininga I agree with wanting to get this back to 10.x and 8.x if possible. I've already added the tags so that we consider doing that.

@anthony-tuininga
Copy link
Contributor Author

Great. Let me know if you need anything further from me.

@joyeecheung
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 7, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 8, 2019
Improve performance creating strings using N-API by ensuring that the
strings are not internalized.

Added test cases for latin-1 and utf-16 strings.

PR-URL: nodejs#26439
Fixes: nodejs#26437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 8, 2019

Landed in 914d908

@anthony-tuininga congratulations to your first commit to Node.js! 🎉

I fixed the commit message to adhere to our commit guidelines while landing.

@BridgeAR BridgeAR closed this Mar 8, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 13, 2019
Improve performance creating strings using N-API by ensuring that the
strings are not internalized.

Added test cases for latin-1 and utf-16 strings.

PR-URL: nodejs#26439
Fixes: nodejs#26437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
Improve performance creating strings using N-API by ensuring that the
strings are not internalized.

Added test cases for latin-1 and utf-16 strings.

PR-URL: #26439
Fixes: #26437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 5, 2019
Improve performance creating strings using N-API by ensuring that the
strings are not internalized.

Added test cases for latin-1 and utf-16 strings.

PR-URL: #26439
Fixes: #26437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

I've landed on 10.x and 8.x

MylesBorins pushed a commit that referenced this pull request Apr 5, 2019
Improve performance creating strings using N-API by ensuring that the
strings are not internalized.

Added test cases for latin-1 and utf-16 strings.

PR-URL: #26439
Fixes: #26437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2019
Improve performance creating strings using N-API by ensuring that the
strings are not internalized.

Added test cases for latin-1 and utf-16 strings.

PR-URL: #26439
Fixes: #26437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants