optimize passing arguments from js to c

[jorangreef]

See #55 (comment) for context.

[jorangreef]

@jorangreef I tested the uint32_t truncation, and AFAICT it's working as advertised. Please have a look at the PR and let me know if I've missed any scenarios!

Thanks @gabrielschulhof, I see that you asserted that overflow happens when calling napi_get_value_uint32() on a JS value that is more than 32 bits: testUint32(4294967297, 1);

What I meant was that if the JS value is more than 32 bits, I would have expected an exception to be thrown instead. However, this kind of check should rather be in napi_get_value_uint32_validated() (or something like that) to avoid slowing down napi_get_value_uint32().

I just realized that using a large ArrayBuffer to hold the arguments implies that the memory for the two Buffers is contiguous, otherwise it requires copying. Is this the case for your addon and, if not, can you rearrange the memory to make this the case?

Yes, I thought of this but the addon needs to be zero-copy, and the key and value buffer arguments would not necessarily always be the same.

@jorangreef I'm curious - are you using N-API directly, or via the node-addon-api package?

I am using N-API directly, using #include <node_api.h> running Node v10.6.0.

Thanks for all your help with this!

[jorangreef]

I know we always compare N-API to v8, but is there any chance that some of the v8 methods we're comparing against are slower than they should be?

[gabrielschulhof]

@jorangreef when I build the comparison binding in V8 I try to use the most efficient method for grabbing the arguments from the engine.

[gabrielschulhof]

@jorangreef have you tried switching from node::Buffer to ArrayBuffer? AFAICT, node::Buffer uses an ArrayBufferView which seems to be an additional level of indirection.

[gabrielschulhof]

I tested some more combinations, and it looks like if you switch to ArrayBuffer and we provide validation-free versions of napi_get_arraybuffer_info() and napi_get_value_int64() then we can nearly erase the impact of switching from V8 to N-API.

@nodejs/addon-api this supports that we should provide validation-free versions of the convert-to-native-type APIs.

[gabrielschulhof]

The code: nodejs/node@master...gabrielschulhof:55-abi-stable-node

[mhdawson]

Sorry I could look at the code, but for clarity in this issue, what do you mean by "validation-free"

[gabrielschulhof]

@mhdawson by "validation-free" I mean a version of the API where we don't call APIs such as ->IsNumber() after casting the napi_value as a v8::Local<v8::Value>. We simply assume that it's a number and retrieve the value.

Similarly, we do not validate that the napi_value is a v8::ArrayBuffer, but we simply .As<v8::ArrayBuffer>() and then proceed to retrieve the pointer to the data and the length.

We leave the validation (if any) to the addon author and document that the API doesn't perform any validation, all in the name of better performance.

[gabrielschulhof]

@mhdawson I ran individual benchmarks for each data type to test whether removing the validation would improve the performance to some significance, but the only data type for which this was the case was Int64. All the convert-to-native APIs have pretty much the same performance whether or not we validate.

I suspect that's because in napi_get_value_int64() we not only validate that the value we are asked to convert to native is indeed a JavaScript number, but we also perform some sanitization of the value:

  if (val->IsInt32()) {
    *result = val.As<v8::Int32>()->Value();
    return napi_clear_last_error(env);
  }

  RETURN_STATUS_IF_FALSE(env, val->IsNumber(), napi_number_expected);

  // v8::Value::IntegerValue() converts NaN, +Inf, and -Inf to INT64_MIN,
  // inconsistent with v8::Value::Int32Value() which converts those values to 0.
  // Special-case all non-finite values to match that behavior.
  double doubleValue = val.As<v8::Number>()->Value();
  if (std::isfinite(doubleValue)) {
    // Empty context: https://github.com/nodejs/node/issues/14379
    v8::Local<v8::Context> context;
    *result = val->IntegerValue(context).FromJust();
  } else {
    *result = 0;
}

the performance gain stems from reducing all this to just

    *result = val->IntegerValue(context).FromJust();

foregoing the special-casing mentioned in the comment.

So, a new, non-validating API really only makes sense for Int64:

[jorangreef]

I tested some more combinations, and it looks like if you switch to ArrayBuffer and we provide validation-free versions of napi_get_arraybuffer_info() and napi_get_value_int64() then we can nearly erase the impact of switching from V8 to N-API.

Thanks @gabrielschulhof for the tremendous effort.

[mhdawson]

@gabrielschulhof thanks for all of the analysis. One last question,

Did you check the perf if this part is left in

  if (val->IsInt32()) {
    *result = val.As<v8::Int32>()->Value();
    return napi_clear_last_error(env);
  }

[gabrielschulhof]

@mhdawson we can leave it in. It has no perf impact.

[mhdawson]

Looks like the related PR has landed. Closing.