napi: add support for napi_set_named_property_len function #52984

mertcanaltin · 2024-05-14T18:50:26Z

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.

doc/api/n-api.md

Trott · 2024-05-14T20:03:27Z

doc/api/n-api.md

+flexible property names, especially when dealing with virtual module
+names or other scenarios where '\0' characters are used as part of the name.
+
+Note: This function is experimental and its stability may change in future releases.


Since there's a stability banner, this would seem to be superfluous. Maybe remove it?

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.

src/js_native_api_v8.cc

legendecas

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

test/js-native-api/test_string/test_string.c

gabrielschulhof · 2024-05-17T14:35:21Z

doc/api/n-api.md

@@ -3134,6 +3134,35 @@ overhead in creating/storing strings with this method.
 The JavaScript `string` type is described in
 [Section 6.1.4][] of the ECMAScript Language Specification.

+#### `napi_set_named_property_len`


New APIs should have the prefix node_api_*, i.e. this should be renamed to node_api_set_named_property_len.

doc/api/n-api.md

src/js_native_api.h

Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>

gabrielschulhof · 2024-05-17T16:14:13Z

@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 · 2024-05-17T16:16:45Z

@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

… dev-52979

mertcanaltin · 2024-05-17T18:46:03Z

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 · 2024-05-24T13:57:10Z

src/js_native_api.h

@@ -118,6 +118,22 @@ NAPI_EXTERN napi_status NAPI_CDECL node_api_create_property_key_utf16(
    napi_env env, const char16_t* str, size_t length, napi_value* result);
 #endif  // NAPI_EXPERIMENTAL

+#ifdef NAPI_EXPERIMENTAL
+#define NODE_API_EXPERIMENTAL_HAS_PROPERTY_KEYS


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 · 2024-05-24T14:00:26Z

src/js_native_api_v8.cc

@@ -1718,6 +1718,46 @@ napi_status NAPI_CDECL node_api_create_property_key_utf16(napi_env env,
  });
 }

+napi_status node_api_create_property_key_utf8(napi_env env,


Please implement the node_api_create_property_key_utf8 the same way as the node_api_create_property_key_utf16 implemented using the v8impl::NewString helper function.
The current implementation is missing a few checks and uses v8::NewStringType::kNormal instead of v8::NewStringType::kInternalized which differentiates V8 normal string creation from a V8 string used as a property key.

vmoroz · 2024-05-24T14:03:12Z

test/js-native-api/test_string/test_string.c

@@ -310,6 +310,93 @@ static napi_value TestPropertyKeyUtf16AutoLength(napi_env env,
                         auto_length);
 }

+static napi_value TestPropertyKeyUtf8(napi_env env, napi_callback_info info) {


It would be great to follow the same pattern for test implementation as TestPropertyKeyUtf16 and TestPropertyKeyUtf16AutoLength

vmoroz · 2024-05-24T14:05:26Z

test/js-native-api/test_string/test_string.c

@@ -310,6 +310,93 @@ static napi_value TestPropertyKeyUtf16AutoLength(napi_env env,
                         auto_length);
 }

+static napi_value TestPropertyKeyUtf8(napi_env env, napi_callback_info info) {


The new test methods must be exposed to JS in the end of this code file and then used from JS test code.

vmoroz · 2024-05-24T14:15:24Z

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 · 2024-05-30T10:07:36Z

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 requested a review from legendecas May 14, 2024 18:50

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels May 14, 2024

mertcanaltin requested review from vmoroz and joyeecheung May 14, 2024 18:50

mertcanaltin force-pushed the dev-52979 branch from e417dab to 20834e7 Compare May 14, 2024 18:54

mertcanaltin requested a review from anonrig May 14, 2024 18:56

Trott reviewed May 14, 2024

View reviewed changes

doc/api/n-api.md Show resolved Hide resolved

Trott reviewed May 14, 2024

View reviewed changes

legendecas reviewed May 14, 2024

View reviewed changes

src/js_native_api_v8.cc Show resolved Hide resolved

legendecas requested changes May 14, 2024

View reviewed changes

test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved

mertcanaltin requested review from legendecas and Trott May 15, 2024 15:08

mertcanaltin force-pushed the dev-52979 branch from a9306d0 to c32d314 Compare May 15, 2024 15:19

node-api: add support for napi_set_named_property_len function

9c1a0bb

mertcanaltin force-pushed the dev-52979 branch from ab3f4c8 to 9c1a0bb Compare May 15, 2024 15:30

mertcanaltin changed the title ~~napi: dd support for napi_set_named_property_len function~~ napi: add support for napi_set_named_property_len function May 15, 2024

mertcanaltin requested a review from gabrielschulhof May 15, 2024 15:30

lint cpp

c9ee714

gabrielschulhof reviewed May 17, 2024

View reviewed changes

mertcanaltin and others added 2 commits May 17, 2024 17:40

Update n-api.md

1579a8a

Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>

Update js_native_api.h

5c46e68

Co-authored-by: Gabriel Schulhof <gabrielschulhof@gmail.com>

Mert Can Altin added 4 commits May 17, 2024 19:47

rename api name

82c3690

Merge branch 'dev-52979' of https://github.com/mertcanaltin/node into…

fe76c9e

… dev-52979

lint

5f2cf3f

create node_api_create_property_key_utf8

02a9931

Mert Can Altin added 2 commits May 17, 2024 21:24

added doc for node_api_create_property_key_utf8

37f9f6b

added test for node_api_create_property_key_utf8

e565690

mertcanaltin requested a review from gabrielschulhof May 17, 2024 18:46

vmoroz reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

napi: add support for napi_set_named_property_len function #52984

napi: add support for napi_set_named_property_len function #52984

mertcanaltin commented May 14, 2024

Trott May 14, 2024

legendecas left a comment

legendecas left a comment

gabrielschulhof May 17, 2024

gabrielschulhof commented May 17, 2024

mertcanaltin commented May 17, 2024 •

edited

mertcanaltin commented May 17, 2024 •

edited

vmoroz May 24, 2024

vmoroz May 24, 2024

vmoroz May 24, 2024

vmoroz May 24, 2024

vmoroz commented May 24, 2024

KevinEady commented May 30, 2024

napi: add support for napi_set_named_property_len function #52984

Are you sure you want to change the base?

napi: add support for napi_set_named_property_len function #52984

Conversation

mertcanaltin commented May 14, 2024

Trott May 14, 2024

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

gabrielschulhof May 17, 2024

Choose a reason for hiding this comment

gabrielschulhof commented May 17, 2024

mertcanaltin commented May 17, 2024 • edited

mertcanaltin commented May 17, 2024 • edited

vmoroz May 24, 2024

Choose a reason for hiding this comment

vmoroz May 24, 2024

Choose a reason for hiding this comment

vmoroz May 24, 2024

Choose a reason for hiding this comment

vmoroz May 24, 2024

Choose a reason for hiding this comment

vmoroz commented May 24, 2024

KevinEady commented May 30, 2024

mertcanaltin commented May 17, 2024 •

edited

mertcanaltin commented May 17, 2024 •

edited