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

n-api: optimize number API performance #14573

Closed
wants to merge 0 commits into from

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented Aug 1, 2017

  • Add separate APIs for creating different kinds of numbers, because creating a V8 number value from an integer is faster than creating one from a double. This is a breaking change, however it should not be considered semver-major because N-API is still experimental.
  • When getting number values, avoid getting the current context because the context will not actually be used and is slightly expensive to obtain.
  • When creating values, don't use v8::TryCatch (NAPI_PREAMBLE macro), because these functions have no possibility of executing JS code.

Refs: #14379 (optimizations 1, 2, and 4)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Aug 1, 2017
@jasongin jasongin added the performance Issues and PRs related to the performance of Node.js. label Aug 1, 2017
src/node_api.h Outdated
@@ -127,9 +127,18 @@ NAPI_EXTERN napi_status napi_create_array(napi_env env, napi_value* result);
NAPI_EXTERN napi_status napi_create_array_with_length(napi_env env,
size_t length,
napi_value* result);
NAPI_EXTERN napi_status napi_create_number(napi_env env,
NAPI_EXTERN napi_status napi_create_double(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep supporting the name napi_create_number to avoid the – minor – breakage that this creates.

That can be as simple as

#define napi_create_number napi_create_double

in the header and

#undef napi_create_number
napi_status napi_create_number(napi_env env, double value, napi_value* result) {
  return napi_create_double(env, value, result);
}

in the source file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, I know this is not pretty, but we should be prepared to see legacy fallbacks accumulate in N-API (and that’s okay, it’s literally N-API’s purpose to provide stability).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be other breaking changes before N-API exits experimental status. While I acknowledge that sometime soon we need to stabilize and stop making breaking changes, I think it might be a bit too soon.

src/node_api.cc Outdated
@@ -830,13 +828,13 @@ static napi_status napi_clear_last_error(napi_env env) {
return napi_ok;
}

static napi_status napi_set_last_error(napi_env env, napi_status error_code,
static inline
napi_status napi_set_last_error(napi_env env, napi_status error_code,
uint32_t engine_error_code,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation here and below is off now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@mhdawson
Copy link
Member

mhdawson commented Aug 3, 2017

I think we should draw the line soon (say end of Aug) at which point we'll avoid breaking changes, but at this point I'm thinking since we know there will be a few others we might as well make this one as well.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me subject to fixing up the alignment as pointed out by @cjihrig

@jasongin
Copy link
Member Author

jasongin commented Aug 3, 2017

src/node_api.cc Outdated
v8::Local<v8::Context> context = isolate->GetCurrentContext();
*result = val->Int32Value(context).FromJust();
v8::Local<v8::Context> context; // empty context
*result = val->Int32Value(context).FromJust();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to document why we are passing an empty context. A link to #14379 shall suffice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jasongin
Copy link
Member Author

jasongin commented Aug 7, 2017

@jasongin
Copy link
Member Author

jasongin commented Aug 8, 2017

CI failures were unrelated.

Landed as af70c3b

@jasongin jasongin closed this Aug 8, 2017
jasongin added a commit to jasongin/nodejs that referenced this pull request Aug 8, 2017
 - Add separate APIs for creating different kinds of numbers,
   because creating a V8 number value from an integer is faster
   than creating one from a double.
 - When getting number values, avoid getting the current context
   because the context will not actually be used and is expensive
   to obtain.
 - When creating values, don't use v8::TryCatch (NAPI_PREAMBLE),
   because these functions have no possibility of executing JS code.

Refs: nodejs#14379
PR-URL: nodejs#14573
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasongin jasongin deleted the napi-number-perf branch August 8, 2017 00:34
addaleax pushed a commit that referenced this pull request Aug 10, 2017
 - Add separate APIs for creating different kinds of numbers,
   because creating a V8 number value from an integer is faster
   than creating one from a double.
 - When getting number values, avoid getting the current context
   because the context will not actually be used and is expensive
   to obtain.
 - When creating values, don't use v8::TryCatch (NAPI_PREAMBLE),
   because these functions have no possibility of executing JS code.

Refs: #14379
PR-URL: #14573
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 13, 2017
@addaleax addaleax mentioned this pull request Aug 13, 2017
addaleax added a commit that referenced this pull request Aug 13, 2017
Notable changes

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
addaleax added a commit that referenced this pull request Aug 14, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
evanlucas pushed a commit that referenced this pull request Aug 15, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs/node#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs/node#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs/node#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs/node#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs/node#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs/node#14558)

PR-URL: nodejs/node#14811
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
 - Add separate APIs for creating different kinds of numbers,
   because creating a V8 number value from an integer is faster
   than creating one from a double.
 - When getting number values, avoid getting the current context
   because the context will not actually be used and is expensive
   to obtain.
 - When creating values, don't use v8::TryCatch (NAPI_PREAMBLE),
   because these functions have no possibility of executing JS code.

Refs: nodejs#14379
PR-URL: nodejs#14573
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
 - Add separate APIs for creating different kinds of numbers,
   because creating a V8 number value from an integer is faster
   than creating one from a double.
 - When getting number values, avoid getting the current context
   because the context will not actually be used and is expensive
   to obtain.
 - When creating values, don't use v8::TryCatch (NAPI_PREAMBLE),
   because these functions have no possibility of executing JS code.

Refs: #14379
Backport-PR-URL: #19447
PR-URL: #14573
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. performance Issues and PRs related to the performance of Node.js. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants