-
Notifications
You must be signed in to change notification settings - Fork 29k
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
benchmark: add n-api function arguments benchmark suite #21555
Conversation
This benchmark suite is added to measure the performance of n-api function call with various type/number of arguments. The cases in this suite are carefully selected to efficiently show the performance trend.
The below charts are generated by this benchmark suite: Generally speaking, it's better to cover more combination of types and numbers of arguments, but from my observation, the trend is the same. So I filtered out most of the combination and selected the cases. |
@kenny-y I think it might be better placed under I'm surprised that N-API performs better in some use cases than V8. It seems strange. |
status = napi_create_string_utf8(env, "reduce", strlen("reduce"), &key); | ||
assert(status == napi_ok); | ||
status = napi_get_property(env, args[0], key, &value); | ||
assert(status == napi_ok); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could combine these into napi_get_named_property()
. That will internally create the string for you.
status = napi_typeof(env, args[0], types); | ||
assert(status == napi_ok); | ||
|
||
assert(types[0] == napi_object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add && argc == 1
into the assertion, to be on par with the V8 version.
v8::Isolate* isolate = args.GetIsolate(); | ||
|
||
assert(args.Length() == 1 && args[0]->IsObject()); | ||
if (args.Length() == 1 && args[0]->IsObject()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to save the result of args.Length() == 1 && args[0]->IsObject()
first and then both assert()
it and use it as the if-statement condition. For clarity, you could assert(argumentsCorrect && "argument length is 1 and the first argument is an object")
, where bool argumentsCorrect = (args.Length() == 1 && args[0]->IsObject());
Wait, NM. |
Local<Value> map = obj->Get(String::NewFromUtf8(isolate, "map")); | ||
Local<Value> operand = obj->Get(String::NewFromUtf8(isolate, "operand")); | ||
Local<Value> data = obj->Get(String::NewFromUtf8(isolate, "data")); | ||
Local<Value> reduce = obj->Get(String::NewFromUtf8(isolate, "reduce")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
N-API uses the maybe version of Get()
and String::NewFromUtf8()
because the others are deprecated, so, these should be changed to
Local<Context> context = isolate->GetCurrentContext();
and then, for each property,
MaybeLocal<String> map_string = String::NewFromUtf8(isolate,
"map",
NewStringType::kNormal);
assert(!map_string.IsEmpty());
MaybeLocal<value> maybe_map = obj->Get(context, map_string.ToLocalChecked());
assert(!maybe_map.IsEmpty());
Local<Value> map = maybe_map.ToLocalChecked();
Yep, there was a PR #21046 moved After the change to |
@kenny-y thanks for creating this. I won't have time to take a good look until at least next week but looks like @gabrielschulhof is already commenting :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
const getArgs = (type) => { | ||
return generateArgs(type.split('-')[1]); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this function if you use multiple parameters (see below).
['v8', 'napi'].forEach((type) => { | ||
benchTypes.push(type + '-' + arg); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need benchTypes
either if you use multiple parameters (see below).
}); | ||
|
||
const bench = common.createBenchmark(main, { | ||
type: benchTypes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of benchTypes
use argsTypes
directly, and add a second parameter
engine: ['v8', 'napi'],
n: [1, 1e1, 1e2, 1e3, 1e4, 1e5], | ||
}); | ||
|
||
function main({ n, type }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this {n, engine, type}
, and then you don't need to concatenate or split the arguments. The benchmark will generate all combinations of the three arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Thanks 👍
@kenny-y could you please also resolve the warnings? |
@mhdawson Thanks. My original purpose to create this benchmark was to find optimization opportunities, like the previous one #21072, but none had been identified yet... @gabrielschulhof helped me a lot on the previous one, as well as this PR 👍 👍 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
assert(status == napi_ok); \ | ||
status = napi_set_named_property(env, exports, name, func ## _v); \ | ||
assert(status == napi_ok); \ | ||
} while (0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The trailing backslashes must be aligned to the column of the outermost trailing backslash.
|
||
#define EXPORT_FUNC(name, func) \ | ||
do { \ | ||
napi_value func ## _v; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you can safely declare
napi_status status;
napi_value js_func;
here and pass js_func
to napi_create_function()
and napi_set_named_property()
because the do { ... } while (0)
scope protects them, and then each invocation of the macro is self-contained. Then there's no need to func ## _v
, nor to assume that napi_status status
is declared outside the macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and please pass exports
into the macro as a parameter, and put brackets around env
, exports
, and func
when you use them inside the macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
NULL, \ | ||
&js_func); \ | ||
assert(status == napi_ok); \ | ||
status = napi_set_named_property((env), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Align arguments as with the previous call.
Since @kfarnung beat me to it I'm happy to leave it at that :) |
Landed in 3314b3a. |
This benchmark suite is added to measure the performance of n-api function call with various type/number of arguments. The cases in this suite are carefully selected to efficiently show the performance trend. PR-URL: #21555 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
This benchmark suite is added to measure the performance of n-api function call with various type/number of arguments. The cases in this suite are carefully selected to efficiently show the performance trend. PR-URL: #21555 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
This benchmark suite is added to measure the performance of n-api
function call with various type/number of arguments. The cases in
this suite are carefully selected to efficiently show the performance
trend.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes