Repository navigation
napi_create_string_utf8() performance issue #26437
Description
- Version: v12.0.0-pre
- Platform: Linux
- Subsystem: N-API
In working on the N-API implementation for the node-oracledb driver for Oracle Database it was discovered that the performance between the original V8/NAN implementation and the N-API implementation is dramatically different in some situations. I tracked it down to the fact that the napi_create_string_utf8() uses string type kInternalized instead of kNormal. For my example, creating strings of length 4 MB resulted in almost 4x more time required to create the string than with the original implementation. If I make the following change:
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index 022ce104eb..9da9bb2b2a 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -23,7 +23,7 @@
(len == NAPI_AUTO_LENGTH) || len <= INT_MAX, \
napi_invalid_arg); \
auto str_maybe = v8::String::NewFromUtf8( \
- (env)->isolate, (str), v8::NewStringType::kInternalized, \
+ (env)->isolate, (str), v8::NewStringType::kNormal, \
static_cast<int>(len)); \
CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure); \
(result) = str_maybe.ToLocalChecked(); \the performance becomes indistinguishable.
So, can this be patched? Or is there a particular reason for using the string type kInternalized? Since this is only particularly noticeable for large strings, the other possibility would be to check the length and only use kNormal if the length is greater than a particular size -- not sure if that would be better?
I tried creating a buffer and converting that to a string by using napi_create_buffer() and napi_coerce_to_string(). That works better but is still more than 2x slower.
Activity
anthony-tuininga commentedon Mar 4, 2019
An additional note: strings < 100 bytes in length are virtually indistinguishable. The performance decrease become somewhat noticeable around 1,000 bytes and at about 10,000 bytes is very noticeable at about 2x slower. Past 30,000 bytes the performance is around 4x slower and seems to stay at about that ratio after that -- at least up to 16 MB in size.
addaleax commentedon Mar 4, 2019
Generally, your suggested patch makes sense. It should be applied for all of the
napi_create_string_*methods, and it would be great if you could open a pull request.One issue is that
CHECK_NEW_FROM_UTF8()orCHECK_NEW_FROM_UTF8_LEN()are used in quite a number of places, and it actually does make sense to usekInternalizedfor a lot of its call sites.So, I would suggest to have
napi_create_string_utf8()look more likenapi_create_string_latin1(), with the NewStringType inline in the function’s source code (and changed tokNormal), and to leave the macro itself as it is for now?anthony-tuininga commentedon Mar 4, 2019
Sure. I'm not that familiar with the Node.js internals so took the easiest route. I'll take a closer look now and create a pull request on which you can comment further.
anthony-tuininga commentedon Mar 4, 2019
Ok. Take a look and let me know if that more closely aligns to what you were expecting? Let me know if any changes are required.
n-api: improve performance creating strings
anthony-tuininga commentedon Mar 9, 2019
I'm not sure what the process is for getting this fix backported to v10.x and v8.x. If it would help to have PRs created for each branch, please let me know.
BridgeAR commentedon Mar 9, 2019
@anthony-tuininga the PRs are pulled in the other released automatically, if they have no conflict with the target branch. If there are conflicts, the releasing person is going to leave a comment that a manual backport is required.
anthony-tuininga commentedon Mar 9, 2019
Ok. Thanks for the explanation!
n-api: improve performance creating strings
n-api: improve performance creating strings
n-api: improve performance creating strings