-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
node-api: add support for UTF-8 and Latin-1 property keys #52984
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer adding node_api_set_named_property(napi_env env, napi_value object, const char* utf8name, size_t name_length, napi_value value)
(which is the new naming pattern) and deprecating napi_set_named_property
once the new one promoted to stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this. Request for change on implementation as comments above.
ab3f4c8
to
9c1a0bb
Compare
@mertcanaltin we discussed this PR at today's Node-API team meeting. We have come to the conclusion that, although this new API adds convenience for developers, it actually encourages a pattern of runtime inefficiency by making it easy to create properties with keys for which the engine is unaware that they are not just regular strings but property keys, and thus it doesn't optimize them. We believe it would be better to pivot this PR to creating additional variants of In addition, it would be advisable for add-on maintainers to create a property key once, and then reuse the resulting This is certainly less convenient for those writing the code, however, it increases the performance of the resulting add-ons. |
First of all, thank you very much for your review and support, do you think I can continue to develop this pr I would be very happy if you have any suggestions ❤️🙏 @gabrielschulhof |
I created a node_api_create_property_key_utf8 based on what you said node_api_create_proper- I wonder if we can get a performance as you say by making different string encodings? |
@mertcanaltin , there are a few Node-API benchmarks here: https://github.com/nodejs/node/tree/main/benchmark/napi. A new one can be created to benchmark different ways to set and get property values. |
I am hesitant of adding these APIs, as the functionality to do what you want already exists using both Adding new APIs has more overreaching effects than just Node.js itself, as other JavaScript engines must implement the new APIs as well. Since this functionality already exists and can be performed in a trivial manner, I 👎 this addition. |
2ea43c3
to
c63e56d
Compare
did the rebase, many thanks for your review |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
PR is currently blocked from landing by unreliable CI. |
This comment was marked as outdated.
This comment was marked as outdated.
Commit Queue failed- Loading data for nodejs/node/pull/52984 ✔ Done loading data for nodejs/node/pull/52984 ----------------------------------- PR info ------------------------------------ Title node-api: add support for UTF-8 and Latin-1 property keys (#52984) Author Mert Can Altin <mertgold60@gmail.com> (@mertcanaltin) Branch mertcanaltin:dev-52979 -> nodejs:main Labels c++, node-api, author ready, needs-ci, commit-queue-squash Commits 1 - node-api: add support for UTF-8 and Latin-1 property keys Committers 1 - Mert Can Altin <mert.altin@trendyol.com> PR-URL: https://github.com/nodejs/node/pull/52984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 14 May 2024 18:50:26 GMT ✔ Approvals: 3 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52984#pullrequestreview-2205518627 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/52984#pullrequestreview-2279534350 ✔ - Vladimir Morozov (@vmoroz): https://github.com/nodejs/node/pull/52984#pullrequestreview-2255528826 ⚠ GitHub cannot link the author of 'node-api: add support for UTF-8 and Latin-1 property keys' to their GitHub account. ⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork ✘ Last GitHub CI failed ℹ Last Full PR CI on 2024-09-13T15:07:38Z: https://ci.nodejs.org/job/node-test-pull-request/62402/ - Querying data for job/node-test-pull-request/62402/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/10854452945 |
PR-URL: #52984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Landed in 75e4d0d |
@jasnell you got ahead of me with manually landing it in minutes... Thanks for landing it! |
PR-URL: #52984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
PR-URL: #52984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
PR-URL: #52984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
This commit addresses issue #52979. It introduces new functions for optimized property key creation:
node_api_create_property_key_utf8
node_api_create_property_key_latin1
These functions help in creating property keys that allow more efficient property access when reused multiple times.
This commit also includes:
Updates to documentation to reflect the addition of the new functions.
New tests to ensure the correctness of the implementation.
Additionally, this work is related to the previous PR node-api: refactor napi_set_property function for improved performance #50282, which covered the utf16 case.