napi: Improve performance creating strings#26439
Uh oh!
There was an error while loading. Please reload this page.
Conversation
… strings are not internalized (see Node issue #26437).
nodejs-github-bot added the c++ 
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.) |
Uh oh!
There was an error while loading. Please reload this page.
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 |
mhdawson added performance 
mhdawson commented Mar 4, 2019
@anthony-tuininga thanks for your work on this. |
anthony-tuininga commented Mar 5, 2019
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
If it’s all the same to you, I’d rather do it now/here. :) |
anthony-tuininga commented Mar 5, 2019
Ok. I'll add the checks here. One moment. |
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 commented Mar 5, 2019
Great. Let me know if you need anything further from me. |
joyeecheung commented Mar 6, 2019
BridgeAR added the author ready 
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#26439Fixes: nodejs#26437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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. |
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#26439Fixes: nodejs#26437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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: #26439Fixes: #26437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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: #26439Fixes: #26437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Apr 5, 2019
I've landed on 10.x and 8.x |
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: #26439Fixes: #26437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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: #26439Fixes: #26437 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
8 participants
Improve performance creating strings using N-API by ensuring that the strings are not internalized (see Node issue #26437).
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes