node-api: add support for UTF-8 and Latin-1 property keys

[mertcanaltin]

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.

[legendecas]

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.

[legendecas]

Thank you for working on this. Request for change on implementation as comments above.

[gabrielschulhof]

@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 node_api_create_property_key_utf16 for the other two string encodings because it would encourage add-on maintainers to create property keys that are treated efficiently by the engine, thus increasing the performance of code that has to create large numbers of objects.

In addition, it would be advisable for add-on maintainers to create a property key once, and then reuse the resulting napi_value in subsequent calls to napi_set_property and/or napi_define_properties in order to avoid repeatedly creating the same string.

This is certainly less convenient for those writing the code, however, it increases the performance of the resulting add-ons.

[mertcanaltin]

@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 node_api_create_property_key_utf16 for the other two string encodings because it would encourage add-on maintainers to create property keys that are treated efficiently by the engine, thus increasing the performance of code that has to create large numbers of objects.

In addition, it would be advisable for add-on maintainers to create a property key once, and then reuse the resulting napi_value in subsequent calls to napi_set_property and/or napi_define_properties in order to avoid repeatedly creating the same string.

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

[mertcanaltin]

I created a node_api_create_property_key_utf8 based on what you said

node_api_create_proper-
ty_key_utf8
node_api
_create_proper-
ty_key_ascil

I wonder if we can get a performance as you say by making different string encodings?

[vmoroz]

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.
Note that the true advantage from using node_api_create_property_key_utf8 and node_api_create_property_key_utf16 must be seen only when the same property key is reused for setting/getting a property multiple times.

[KevinEady]

I am hesitant of adding these APIs, as the functionality to do what you want already exists using both node_api_create_property_key_utf16() and napi_set_property(). I do not follow the argument of "an extra napi call" as outlined in #52979 (comment), as the team discussed it in a Node-API meeting and decided the additional call would be minimal overhead.

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.

[jasnell]

PR is currently blocked from landing by unreliable CI.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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 <[email protected]> (@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 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/52984
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52984
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
--------------------------------------------------------------------------------
   ℹ  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/.ncu

https://github.com/nodejs/node/actions/runs/10854452945

[jasnell]

Landed in 75e4d0d

[legendecas]

@jasnell you got ahead of me with manually landing it in minutes... Thanks for landing it!