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 performance #49922

Open
mmomtchev opened this issue Sep 28, 2023 · 6 comments
Open

Node-API performance #49922

mmomtchev opened this issue Sep 28, 2023 · 6 comments
Labels
feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale performance Issues and PRs related to the performance of Node.js.

Comments

@mmomtchev
Copy link
Contributor

mmomtchev commented Sep 28, 2023

What is the problem this feature will solve?

Improve performance when creating large V8 objects which is critical since these must always be created on the main thread, blocking the event loop.

Currently, the overhead of each method is very significant - especially for the primitive operations such as simply setting a property.

As a very typical example of the current problem, you can consider these two methods for setting a property on an object:

napi_status napi_set_property(napi_env env,
                                          napi_value object,
                                          napi_value key,
                                          napi_value value);
napi_status napi_set_named_property(napi_env env,
                                          napi_value object,
                                          const char* utf8name,
                                          napi_value value);

Naively, one expects that using the first one will be faster when the key is already UTF-16. However calling this method requires to first call napi_create_string_utf16 and the combined overhead of the two calls offsets the single call of the second method when the key is not very long (which is usually the case).

What is the feature you are proposing to solve the problem?

Step 1: napi_set_named_property that takes UTF-16 (not needed, see below)

Step 2: Methods that can create in one call a whole array/object from a C data structure - especially ones for strings already encoded in UTF-16

What alternatives have you considered?

No response

@mmomtchev mmomtchev added the feature request Issues that request new features to be added to Node.js. label Sep 28, 2023
@mmomtchev
Copy link
Contributor Author

mmomtchev commented Sep 28, 2023

Also this should probably use an union and be inline:

memcpy(static_cast<void*>(&local), &v, sizeof(v));

PS (sorry it already is inline in recent versions)

@bnoordhuis
Copy link
Member

At least with V8, it doesn't matter if you pass in UTF-16, it'll convert it to latin1 / one-byte if possible. The way it checks that is actually pretty expensive so you're probably worse off ceteris paribus.

A version of napi_set_named_property that takes latin1 is a better bet. That way at least you avoid UTF-8 decoding overhead.

Aside: memcpy is a legal way of doing type punning in C++ (for POD types anyway.)

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Sep 28, 2023

Yes, I confirm. There is no way to pass pre-encoded UTF16 data to V8?

Answer: v8::String::ExternalStringResource is the way to go

@tniessen tniessen added the performance Issues and PRs related to the performance of Node.js. label Sep 28, 2023
@mertcanaltin
Copy link
Member

Could this be a solution? I tried to do something #50282

@vmoroz
Copy link
Member

vmoroz commented Oct 27, 2023

This issue seems to be related to the issue #45905 where we looked at different ways to improve the perf for object creation.
Having a dedicated function that creates a data object from a struct should significantly improve the perf.

To improve property access we must add APIs to create internalized strings which V8 can use directly without any additional preprocessing.

Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Apr 25, 2024
@mhdawson mhdawson added never-stale Mark issue so that it is never considered stale and removed stale labels Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale performance Issues and PRs related to the performance of Node.js.
Projects
Status: Awaiting Triage
Development

No branches or pull requests

6 participants