napi: add support for napi_set_named_property_len function

[mertcanaltin]

This pull request addresses issue #52979, where developers encounter issues with the napi_set_named_property function treating '\0' characters as terminators rather than values. To solve this problem, the napi_set_named_property_len function has been introduced as a new variant. This function accepts a string and its length, allowing developers to use '\0' characters as values when setting named properties on JavaScript objects.

in addition notes:

This change includes updates to the documentation to reflect the addition of the new function.
Tests have been added to ensure the correctness of the implementation.
The stability of this feature is marked as experimental and may change in future releases.

Test Plan:

Run existing unit tests to ensure no regressions occur.
Add new unit tests specifically targeting the behavior of the napi_set_named_property_len function.
Manually test the functionality by integrating it into existing code where '\0' characters are used as property values.

[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]

Defining the same macro twice will cause compiler warnings or errors. Please group the new node_api_create_property_key_utf8 API with the previous node_api_create_property_key_utf16.

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

[mertcanaltin]

Hi @KevinEady,
Some might argue that the existing node_api_create_property_key_utf16 and napi_set_property functions are sufficient. However, these functions come with additional performance costs and increased code complexity. The new function presents a more efficient and effective solution. I would love to hear any suggestions you might have.

[mertcanaltin]

thank you very much for reviewing me I was at a conference this week so I could not give a quick answer but I will give an update today thank you all very much

[mhdawson]

We discussed in the Node-api team meeting again today, and consensus seems to be to implement the suggestion in #52984 (comment), and only add the new property key creation functions, and no others.

[mertcanaltin]

greetings I'm very sorry I'm late getting back here, I sent a few edits today

[vmoroz]

@mertcanaltin , thank you for making the progress with the PR.
Based on the discussions in Node-API meetings and the comments above:

• Could you please remove the new napi_set_named_property_len function. The team sees this function as redundant. Developers can implement a simple helper function in their code today by using napi_create_string_utf8 and napi_set_property functions to allow using \0 in the name. Since Node-API is implemented by other JS runtimes such as Deno and Bun, and on top of other JS engines such as Hermes, adding simple helper functions unnecessary increases the API surface.
• As an alternative to the original purpose of this PR, it would be great to retarget it to add the new node_api_create_property_key_latin1 and node_api_create_property_key_utf8 functions. These functions enable creation of optimized strings that can be reused for the efficient property access. Their implementation can follow the same set of changes as in your previous PR node-api: refactor napi_set_property function for improved performance  #50282.
• It would be great to add a simple benchmark test that shows that reusing a property name created by node_api_create_property_key_utf8 is faster than the one created by napi_create_string_utf8 function.

Please let me know if I can help with the implementation.

[mertcanaltin]

I did these steps @vmoroz

• Removed the napi_set_named_property_len function based on discussions in Node-API meetings.
• Added node_api_create_property_key_latin1 and node_api_create_property_key_utf8 functions for optimized property key creation.
• Updated the test file to include new tests for node_api_create_property_key_latin1 and node_api_create_property_key_utf8.
• Implemented a benchmark test (BenchmarkPropertyKeyUtf8) to compare the performance of node_api_create_property_key_utf8 with napi_create_string_utf8.
• The new benchmark test shows that reusing a property name created by node_api_create_property_key_utf8 is faster than using napi_create_string_utf8.

[mertcanaltin]

I wonder if you have a suggestion about test failures @vmoroz

PR changed

[vmoroz]

I wonder if you have a suggestion about test failures @vmoroz

The error says that the module is not found. It is possible that it is failed to build. I will check it out and see how it can be fixed.

[vmoroz]

I did these steps @vmoroz

• Removed the napi_set_named_property_len function based on discussions in Node-API meetings.
• Added node_api_create_property_key_latin1 and node_api_create_property_key_utf8 functions for optimized property key creation.
• Updated the test file to include new tests for node_api_create_property_key_latin1 and node_api_create_property_key_utf8.
• Implemented a benchmark test (BenchmarkPropertyKeyUtf8) to compare the performance of node_api_create_property_key_utf8 with napi_create_string_utf8.
• The new benchmark test shows that reusing a property name created by node_api_create_property_key_utf8 is faster than using napi_create_string_utf8.

@mertcanaltin , are you sure that the latest commit in this PR has all the latest code?

• The code is missing the implementation for node_api_create_property_key_latin1 and there are no tests or docs for it.
• Docs for napi_set_named_property_len are not removed.
• The benchmark test must be in the benchmark/napi folder. E.g. see the string test there.

[mertcanaltin]

greetings, I tried to verify the structure, but I got a Cannot find package error @vmoroz

➜  node git:(dev-52979) ✗ make test
ninja -C out/Release 
ninja: Entering directory `out/Release'
ninja: no work to do.
if [ ! -r node ] || [ ! -L node ]; then ln -fs out/Release/node node; fi
/Applications/Xcode.app/Contents/Developer/usr/bin/make -s tooltest
.....
----------------------------------------------------------------------
Ran 5 tests in 0.002s

OK
/Applications/Xcode.app/Contents/Developer/usr/bin/make -s test-doc
node:internal/modules/esm/resolve:210
  const resolvedOption = FSLegacyMainResolve(packageJsonUrlString, packageConfig.main, baseStringified);
                         ^

Error: Cannot find package '/Users/mert/Desktop/openSource/node/tools/doc/node_modules/rehype-raw/package.json' imported from /Users/mert/Desktop/openSource/node/tools/doc/generate.mjs
    at legacyMainResolve (node:internal/modules/esm/resolve:210:26)
    at packageResolve (node:internal/modules/esm/resolve:829:14)
    at moduleResolve (node:internal/modules/esm/resolve:915:18)
    at defaultResolve (node:internal/modules/esm/resolve:1123:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:526:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:249:38)
    at ModuleJob._link (node:internal/modules/esm/module_job:126:49) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v23.0.0-pre
make[2]: *** [out/doc/api/addons.html] Error 1
make[1]: *** [doc-only] Error 2
make: *** [test] Error 2
➜  node git:(dev-52979) ✗ 

[mertcanaltin]

in addition, markdown formatter was getting error, I corrected it by using make format-md