benchmark: add n-api function arguments benchmark suite by kenny-y · Pull Request #21555 · nodejs/node

kenny-y · 2018-06-27T04:42:12Z

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

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.

kenny-y · 2018-06-27T04:45:05Z

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.

CC @gabrielschulhof

gabrielschulhof · 2018-06-27T13:56:08Z

@kenny-y I think it might be better placed under benchmark/misc, like function_call, because this also provides useful data about V8 on its own, not just about V8 vs. N-API.

I'm surprised that N-API performs better in some use cases than V8. It seems strange.

gabrielschulhof · 2018-06-27T13:58:41Z

benchmark/napi/function_args/napi_binding.c

+    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);


You could combine these into napi_get_named_property(). That will internally create the string for you.

gabrielschulhof · 2018-06-27T14:04:14Z

benchmark/napi/function_args/napi_binding.c

+  status = napi_typeof(env, args[0], types);
+  assert(status == napi_ok);
+
+  assert(types[0] == napi_object);


You should add && argc == 1 into the assertion, to be on par with the V8 version.

gabrielschulhof · 2018-06-27T14:10:13Z

benchmark/napi/function_args/binding.cc

+  v8::Isolate* isolate = args.GetIsolate();
+
+  assert(args.Length() == 1 && args[0]->IsObject());
+  if (args.Length() == 1 && args[0]->IsObject()) {


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());

gabrielschulhof · 2018-06-27T14:16:09Z

Wait, NM. function_call is in napi, not misc. Still, strange about the CallWithObject results.

gabrielschulhof · 2018-06-27T14:53:58Z

benchmark/napi/function_args/binding.cc

+    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"));


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();

nodejs#21555

kenny-y · 2018-06-28T02:31:01Z

Yep, there was a PR #21046 moved function_call to an napi dir, so I also put this new one under that dir.

After the change to CallWithObject(), the trend is still the same:

mhdawson · 2018-06-29T16:04:27Z

@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 :)

gabrielschulhof

On my machine, only when you run callWithObject() 1000 times does N-API come out ahead of V8. In all other cases, V8 comes out slightly ahead. Very strange indeed.

gabrielschulhof · 2018-06-29T19:43:49Z

benchmark/napi/function_args/index.js

+
+const getArgs = (type) => {
+  return generateArgs(type.split('-')[1]);
+};


You don't need this function if you use multiple parameters (see below).

gabrielschulhof · 2018-06-29T19:45:02Z

benchmark/napi/function_args/index.js

+  ['v8', 'napi'].forEach((type) => {
+    benchTypes.push(type + '-' + arg);
+  });
+});


You don't need benchTypes either if you use multiple parameters (see below).

gabrielschulhof · 2018-06-29T19:45:57Z

benchmark/napi/function_args/index.js

+});
+
+const bench = common.createBenchmark(main, {
+  type: benchTypes,


Instead of benchTypes use argsTypes directly, and add a second parameter

engine: ['v8', 'napi'],

gabrielschulhof · 2018-06-29T19:47:42Z

benchmark/napi/function_args/index.js

+  n: [1, 1e1, 1e2, 1e3, 1e4, 1e5],
+});
+
+function main({ n, type }) {


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.

Good to know. Thanks 👍

gabrielschulhof · 2018-07-01T14:01:44Z

@kenny-y could you please also resolve the warnings?

kenny-y · 2018-07-03T00:57:50Z

@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 👍 👍 👍

gabrielschulhof

LGTM

gabrielschulhof · 2018-07-03T22:51:21Z

benchmark/napi/function_args/napi_binding.c

+    assert(status == napi_ok); \
+    status = napi_set_named_property(env, exports, name, func ## _v); \
+    assert(status == napi_ok); \
+  } while (0);


nit: The trailing backslashes must be aligned to the column of the outermost trailing backslash.

gabrielschulhof · 2018-07-03T22:53:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15718/

gabrielschulhof · 2018-07-04T02:48:08Z

benchmark/napi/function_args/napi_binding.c

+
+#define EXPORT_FUNC(name, func)                                       \
+  do {                                                                \
+    napi_value func ## _v;                                            \


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.

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.

gabrielschulhof · 2018-07-04T14:50:07Z

benchmark/napi/function_args/napi_binding.c

+                                  NULL,                               \
+                                  &js_func);                          \
+    assert(status == napi_ok);                                        \
+    status = napi_set_named_property((env),                           \


nit: Align arguments as with the previous call.

gabrielschulhof · 2018-07-05T14:44:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15738/

gabrielschulhof · 2018-07-05T17:34:39Z

@kfarnung @mhdawson may I please have another set of eyes on this?

mhdawson · 2018-07-05T21:26:25Z

Since @kfarnung beat me to it I'm happy to leave it at that :)

gabrielschulhof · 2018-07-06T01:19:42Z

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>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 27, 2018

benchmark: add n-api function arguments benchmark suite

6aa13a1

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.

gabrielschulhof reviewed Jun 27, 2018

View reviewed changes

Some fine adjustment based on the review comments in

cb758e9

nodejs#21555

Fixed wrong calc of typedarray offset (result not in use)

75e6e42

gabrielschulhof reviewed Jun 29, 2018

View reviewed changes

kenny-y added 2 commits July 2, 2018 13:55

Removing compiler warnings

173fa80

Use benchmark suite feature to match every engines with every args-types

10f3fc5

gabrielschulhof approved these changes Jul 3, 2018

View reviewed changes

Fixing macro styling issue, trailing backslash alignment.

f10939a

gabrielschulhof reviewed Jul 4, 2018

View reviewed changes

Change the local casual macro to a more formal one.

645f38e

gabrielschulhof reviewed Jul 4, 2018

View reviewed changes

Use only one param alignment style in the helper macro.

0f6b238

gabrielschulhof approved these changes Jul 5, 2018

View reviewed changes

kfarnung approved these changes Jul 5, 2018

View reviewed changes

gabrielschulhof self-assigned this Jul 6, 2018

gabrielschulhof closed this Jul 6, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

Uh oh!

Conversation

kenny-y commented Jun 27, 2018

Checklist

Uh oh!

kenny-y commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jun 27, 2018

Uh oh!

gabrielschulhof Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenny-y commented Jun 28, 2018

Uh oh!

mhdawson commented Jun 29, 2018

Uh oh!

gabrielschulhof left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jul 1, 2018

Uh oh!

kenny-y commented Jul 3, 2018

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jul 3, 2018

Uh oh!

gabrielschulhof Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jul 5, 2018

Uh oh!

gabrielschulhof commented Jul 5, 2018

Uh oh!

mhdawson commented Jul 5, 2018

Uh oh!

gabrielschulhof commented Jul 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kenny-y commented Jun 27, 2018 •

edited

Loading

gabrielschulhof Jun 27, 2018 •

edited

Loading

gabrielschulhof left a comment •

edited

Loading

gabrielschulhof Jul 4, 2018 •

edited

Loading