Skip to content
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: refactor napi_set_property function for improved performance #50282

Closed
wants to merge 1 commit into from

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Oct 19, 2023

Node.js PR #50282 - N-API Performance Improvement

This pull request aims to optimize the napi_set_property function to enhance the overall performance of Node-API. The changes include:

  • The signature of the node_api_create_property_key_utf16 function has been updated to match the proposed format.
  • The node_api_create_property_key_utf16 function is now implemented using the NewString function for a more streamlined approach.
  • The napi_set_property_utf16 function, which did not provide additional advantages over node_api_create_property_key_utf16, has been removed.
  • Documentation for the new APIs has been updated, and sample tests have been added.

It's worth noting that these changes are focused on improving the overall performance of Node-API operations, especially those involving property access.

/w @vmoroz 🙏
issue: #49922

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot 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 Oct 19, 2023
@mertcanaltin mertcanaltin changed the title feat: Refactor napi_set_property function for improved performance an… feat: refactor napi_set_property function for improved performance Oct 19, 2023
@mertcanaltin mertcanaltin changed the title feat: refactor napi_set_property function for improved performance n-api: refactor napi_set_property function for improved performance Oct 19, 2023
@mertcanaltin mertcanaltin force-pushed the 49922-dev branch 2 times, most recently from 1047cc3 to f686b2e Compare October 19, 2023 19:21
@mertcanaltin mertcanaltin changed the title n-api: refactor napi_set_property function for improved performance node-api: refactor napi_set_property function for improved performance Oct 19, 2023
@vmoroz
Copy link
Member

vmoroz commented Oct 27, 2023

@mertcanaltin , thank you for working on the improvements to Node-API!
We have discussed your change last week in Node-API team meeting on 10/20/2023.

It seems that the change is targeting to address the same issues as we discussed them in #45905.
I do not believe that the new proposed napi_set_property_utf16 will make a significant perf improvement comparing with creating napi_value string with napi_create_string_utf16 and then using it with napi_set_property.

A better approach would be to create property keys with v8::NewStringType::kInternalized flag and then reusing them to set properties for multiple objects. V8 internally uses internalized strings for all properties. This way they can compare string pointers instead of the whole string. If we use a normal string to access properties, then V8 must internalize it first.

Would you be interested to implement such functions that create internalized strings?
They can improve speed for all property access operations: set, get, has, delete.
They are based on the this proposal: #45905 (comment)

napi_status node_api_create_property_key_latin1(napi_env env, const char* str, size_t length, napi_value* result);
napi_status node_api_create_property_key_utf8(napi_env env, const char* str, size_t length, napi_value* result);
napi_status node_api_create_property_key_utf16(napi_env env, const char16_t* str, size_t length, napi_value* result);
napi_status node_api_create_property_key(napi_env env, napi_value str_or_sym, napi_value* result);

The first three functions must create internalized strings from latin1, utf8, and utf16 strings.
The implementation must be the same as the napi_create_string_latin1, napi_create_string_utf8, and napi_create_string_utf16 except that they must use v8::NewStringType::kInternalized instead of v8::NewStringType::kNormal.

The last one must internalize napi_value string or symbol if they are not internalized yet.
I am not sure if symbols need to be internalized. We can return them "as-is" to start with.

@mertcanaltin
Copy link
Member Author

@mertcanaltin , thank you for working on the improvements to Node-API! We have discussed your change last week in Node-API team meeting on 10/20/2023.

It seems that the change is targeting to address the same issues as we discussed them in #45905. I do not believe that the new proposed napi_set_property_utf16 will make a significant perf improvement comparing with creating napi_value string with napi_create_string_utf16 and then using it with napi_set_property.

A better approach would be to create property keys with v8::NewStringType::kInternalized flag and then reusing them to set properties for multiple objects. V8 internally uses internalized strings for all properties. This way they can compare string pointers instead of the whole string. If we use a normal string to access properties, then V8 must internalize it first.

Would you be interested to implement such functions that create internalized strings? They can improve speed for all property access operations: set, get, has, delete. They are based on the this proposal: #45905 (comment)

napi_status node_api_create_property_key_latin1(napi_env env, const char* str, size_t length, napi_value* result);
napi_status node_api_create_property_key_utf8(napi_env env, const char* str, size_t length, napi_value* result);
napi_status node_api_create_property_key_utf16(napi_env env, const char16_t* str, size_t length, napi_value* result);
napi_status node_api_create_property_key(napi_env env, napi_value str_or_sym, napi_value* result);

The first three functions must create internalized strings from latin1, utf8, and utf16 strings. The implementation must be the same as the napi_create_string_latin1, napi_create_string_utf8, and napi_create_string_utf16 except that they must use v8::NewStringType::kInternalized instead of v8::NewStringType::kNormal.

The last one must internalize napi_value string or symbol if they are not internalized yet. I am not sure if symbols need to be internalized. We can return them "as-is" to start with.

Thank you very much for reviewing and detailing, I will be working on it again with an update in the future

@mertcanaltin
Copy link
Member Author

This commit refactors the napi_set_property_utf16 function to use internalized string property keys, which can lead to better performance. It implements @vmoroz suggestion to create internalized string property keys for accessing object properties. Internalized keys improve speed for property access operations like set, get, has, and delete by allowing V8 to compare string pointers instead of the entire string. This change aligns with the proposal in issue #45905.

New functions added:

  • node_api_create_property_key_utf16 to create internalized property keys from UTF-16 strings.
  • napi_set_property_utf16 now uses internalized property keys for setting object properties.

This change is expected to enhance the overall performance of Node-API operations involving property access.

Fixes: #50282

@vmoroz
Copy link
Member

vmoroz commented Nov 6, 2023

This commit refactors the napi_set_property_utf16 function to use internalized string property keys, which can lead to better performance. It implements @vmoroz suggestion to create internalized string property keys for accessing object properties. Internalized keys improve speed for property access operations like set, get, has, and delete by allowing V8 to compare string pointers instead of the entire string. This change aligns with the proposal in issue #45905.

New functions added:

  • node_api_create_property_key_utf16 to create internalized property keys from UTF-16 strings.
  • napi_set_property_utf16 now uses internalized property keys for setting object properties.

This change is expected to enhance the overall performance of Node-API operations involving property access.

Fixes: #50282

@mertcanaltin , thank you for starting to implement the property key creation functions!
Could you make the node_api_create_property_key_utf16 function signature to match the proposed signature? It is currently missing the length of the string. The implementation should follow the same pattern as the napi_create_string_utf16.

I believe the napi_set_property_utf16 is not needed because it does not bring any value on top of the node_api_create_property_key_utf16 function. The full power of node_api_create_property_key_utf16 is that it helps reusing created string as a key, while napi_set_property_utf16 eliminates this advantage by forcing us to lookup for the key every time.

Please have a look at the PR #48339 where we added creating external strings.
This PR must use the NewString function added in that PR which makes the implementation quite simple.
You can follow the #48339 pattern to define new string APIs, changing docs, and adding new unit tests.

@mertcanaltin
Copy link
Member Author

greetings thank you very much for your suggestions, I applied it, now I think I need to add a test and update the document @vmoroz

@vmoroz
Copy link
Member

vmoroz commented Nov 6, 2023

greetings thank you very much for your suggestions, I applied it, now I think I need to add a test and update the document @vmoroz

Awesome! Could you use the NewString function introduced in PR #48339 for the implementation?
There are a number of different checks that needed to be done and they are all covered by the NewString implementation.
We just need to give it the right lambda. E.g.

napi_status NAPI_CDECL node_api_create_property_key_utf16(napi_env env,
                                                const char16_t* str,
                                                size_t length,
                                                napi_value* result) {
  return v8impl::NewString(env, str, length, result, [&](v8::Isolate* isolate) {
    return v8::String::NewFromTwoByte(isolate,
                                      reinterpret_cast<const uint16_t*>(str),
                                      v8::NewStringType::kInternalized,
                                      static_cast<int>(length));
  });
}

@mertcanaltin
Copy link
Member Author

greetings thank you very much for your suggestions, I applied it, now I think I need to add a test and update the document @vmoroz

Awesome! Could you use the NewString function introduced in PR #48339 for the implementation? There are a number of different checks that needed to be done and they are all covered by the NewString implementation. We just need to give it the right lambda. E.g.

napi_status NAPI_CDECL node_api_create_property_key_utf16(napi_env env,
                                                const char16_t* str,
                                                size_t length,
                                                napi_value* result) {
  return v8impl::NewString(env, str, length, result, [&](v8::Isolate* isolate) {
    return v8::String::NewFromTwoByte(isolate,
                                      reinterpret_cast<const uint16_t*>(str),
                                      v8::NewStringType::kInternalized,
                                      static_cast<int>(length));
  });
}

thank you for your suggestion I updated as you said @vmoroz

@vmoroz
Copy link
Member

vmoroz commented Nov 10, 2023

@mertcanaltin , perfect!
The next step is to add the function to the js_native_api.h, add a unit test, and add documentation.
Please see PR #48339 where other new string-related functions were added.

@mertcanaltin
Copy link
Member Author

I made the additions as you said @vmoroz 🚀 🕺

@mertcanaltin
Copy link
Member Author

I think I have a problem with the napiVersion

@vmoroz
Copy link
Member

vmoroz commented Nov 12, 2023

@mertcanaltin. it looks great!
Please follow the code pattern in PR #48339:

  • All new APIs must be wrapped in #ifdef NAPI_EXPERIMENTAL
  • The docs for the new APIs must use added: REPLACEME
  • New test cases must be added to test_string test.

Also, please put the code for the new string APIs next to the string creation APIs added in PR #48339.

@mertcanaltin
Copy link
Member Author

these steps were great I hope I succeeded I made an update @vmoroz 🚀

doc/api/n-api.md Outdated Show resolved Hide resolved
test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved
test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved
test/js-native-api/test_string/test.js Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2023
@nodejs-github-bot
Copy link
Collaborator

return v8::String::NewFromTwoByte(isolate,
reinterpret_cast<const uint16_t*>(str),
v8::NewStringType::kInternalized,
static_cast<int>(length));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to cast to int here? We don't cast to int in the regular version of this function.

Copy link
Member Author

@mertcanaltin mertcanaltin Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I created this space based on the recommendation of @vmoroz, as mentioned in pull request #48339.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielschulhof , we are getting warnings in MSVC compiler if types are mismatch.
The NewFromTwoByte expects type int which is signed 32-bit integer.
In this function length is size_t which is unsigned 64-bit integer for 64 platforms.
MSVC warns about possible precision loss when the cast implicitly here.
The explicit cast suppresses the warning as we explicitly express our intent.

Ideally, we must add the cast in other places too to avoid the MSVC warnings.
But it should be another PR since currently Node.js has a lot of MSVC warnings anyway.

test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved
@vmoroz vmoroz added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Dec 22, 2023
@mertcanaltin mertcanaltin added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Dec 22, 2023
@mertcanaltin
Copy link
Member Author

I removed my tag so as not to do something wrong, maybe the whole pipeline should go through and then I thought I should do it. 🤔

@vmoroz vmoroz added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2023
@nodejs-github-bot
Copy link
Collaborator

@mertcanaltin
Copy link
Member Author

I wonder if I should do an update here @vmoroz 🚀

@vmoroz
Copy link
Member

vmoroz commented Dec 31, 2023

I wonder if I should do an update here @vmoroz 🚀

@mertcanaltin , it seems that CI has some flaky tests. I see other PRs have the same issue.
We can try to rerun the tests and hope that they pass.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

mhdawson pushed a commit that referenced this pull request Jan 5, 2024
PR-URL: #50282
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@mhdawson
Copy link
Member

mhdawson commented Jan 5, 2024

Landed in 59e7444

@mertcanaltin thanks for your work and patience on this one.

@mhdawson mhdawson closed this Jan 5, 2024
@mertcanaltin
Copy link
Member Author

thank you very much for your support ❤️

marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Jan 12, 2024
PR-URL: nodejs#50282
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Medhansh404 pushed a commit to Medhansh404/node that referenced this pull request Jan 19, 2024
PR-URL: nodejs#50282
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
PR-URL: #50282
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50282
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50282
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants