FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

napi: Improve performance creating strings by anthony-tuininga · Pull Request #26439 · nodejs/node · GitHub

/ node Public

napi: Improve performance creating strings - #26439

Closed
anthony-tuininga wants to merge 4 commits into
nodejs:masterfrom
anthony-tuininga:master
ConversationCommits (4)ChecksFiles changed
Closed

napi: Improve performance creating strings#26439
anthony-tuininga wants to merge 4 commits into
nodejs:masterfrom
anthony-tuininga:master

Conversation

anthony-tuininga commented Mar 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 4, 2019
addaleax approved these changes Mar 4, 2019
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you!

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

addaleax commented Mar 4, 2019

Copy link
Copy Markdown
Member

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.)

addaleax reviewed Mar 4, 2019
View reviewed changes
Comment thread src/js_native_api_v8.cc

mhdawson commented Mar 4, 2019

Copy link
Copy Markdown
Member

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

mhdawson approved these changes Mar 4, 2019
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson added performance Issues and PRs related to the performance of Node.js. lts-watch-v8.x labels Mar 4, 2019

mhdawson commented Mar 4, 2019

Copy link
Copy Markdown
Member

@anthony-tuininga thanks for your work on this.

anthony-tuininga commented Mar 5, 2019

Copy link
Copy Markdown
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 commented Mar 5, 2019

Copy link
Copy Markdown
Member

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 commented Mar 5, 2019

Copy link
Copy Markdown
Contributor Author

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

mhdawson commented Mar 5, 2019

Copy link
Copy Markdown
Member

@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 commented Mar 5, 2019

Copy link
Copy Markdown
Contributor Author

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

joyeecheung commented Mar 6, 2019

Copy link
Copy Markdown
Member

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 7, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 8, 2019
n-api: improve performance creating strings
914d908 
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 commented Mar 8, 2019

Copy link
Copy Markdown
Member

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 closed this Mar 8, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 13, 2019
n-api: improve performance creating strings
14e247e 
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 mentioned this pull request Mar 13, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
n-api: improve performance creating strings
8e7204e 
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>
anthony-tuininga mentioned this pull request Apr 3, 2019
Merged
MylesBorins pushed a commit that referenced this pull request Apr 5, 2019
n-api: improve performance creating strings
2643801 
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 commented Apr 5, 2019

Copy link
Copy Markdown
Contributor

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

MylesBorins pushed a commit that referenced this pull request Apr 5, 2019
n-api: improve performance creating strings
1e76323 
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
n-api: improve performance creating strings
d3de1ed 
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 mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
jasnell jasnell approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL