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

process: js fast path for cached bindings #18365

Closed

Conversation

apapirovski
Copy link
Member

Currently, both process.binding and internalBinding have to call into C++ regardless of whether the module has been cached or not. This creates significant overhead to all binding calls and unfortunately the rest of the codebase doesn't really optimize the amount of times that bindings are required (as an example: 12 files require the async_wrap binding).

Changing all the usage of this function throughout the codebase would introduce a lot of churn (and is kind of a hassle) so instead this PR introduces a JS fast path for both functions for cases where the binding has already been cached. While micro benchmarks are not super meaningful here (it's not like we call binding that often...), this does speed up the cached call by 400%.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process, src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 25, 2018
@apapirovski
Copy link
Member Author

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Rest LGTM


process.binding = function binding(module) {
if (!module || typeof module !== 'string')
return;
Copy link
Member

Choose a reason for hiding this comment

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

It’s unfortunate, but process.binding is used in the wild too often for us to be able to just return undefined if the requested module is undefined. We should cast it to a string just as the C++ version did.

Copy link
Member

Choose a reason for hiding this comment

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

Also, when would core code call it with something that is not a string? This guard should probably just go.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

If you're adding caching in JS land, then I'd get rid of env->binding_cache_object() +env->internal_binding_cache_object() and strip down node::Binding() and node::InternalBinding() to their bare essentials.


process.binding = function binding(module) {
if (!module || typeof module !== 'string')
return;
Copy link
Member

Choose a reason for hiding this comment

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

Also, when would core code call it with something that is not a string? This guard should probably just go.

@apapirovski
Copy link
Member Author

@TimothyGu @bnoordhuis Thanks for the feedback. I've updated the PR based on your suggestions.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo comment. Binding() and InternalBinding() have quite some shared logic you could factor out.

if (typeof bindingObj[module] === 'object')
return bindingObj[module];
bindingObj[module] = getLinkedBinding(module);
return bindingObj[module];
Copy link
Member

Choose a reason for hiding this comment

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

Minor anti-pattern, looking up twice. Do this instead:

let mod = bindingObj[module];
if (typeof mod !== 'object')
  mod = bindingObj[module] = getBinding(module);
return mod;

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, don't know what I was thinking :) Updated.

@apapirovski
Copy link
Member Author

Ok, I've added one final change in terms of moving process.moduleLoadList to JS-land. I'll leave refactoring C++ Binding & InternalBinding for someone else and/or another PR.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Looks nice.

@apapirovski
Copy link
Member Author

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Nice, this also gives me a good idea where to monkey-patch the deprecate bindings....

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Nice catch.

@apapirovski
Copy link
Member Author

Landed in bb27bfe

@apapirovski apapirovski deleted the patch-binding-fast-path-js branch January 29, 2018 16:56
apapirovski added a commit that referenced this pull request Jan 29, 2018
Currently, both process.binding and internalBinding have to call into
C++ regardless of whether the module has been cached or not. This
creates significant overhead to all binding calls and unfortunately
the rest of the codebase doesn't really optimize the amount of times
that bindings are required (as an example: 12 files require the
async_wrap binding).

Changing all the usage of this function throughout the codebase would
introduce a lot of churn (and is kind of a hassle) so instead this PR
introduces a JS fast path for both functions for cases where the
binding has already been cached. While micro benchmarks are not super
meaningful here (it's not like we call binding that often...), this
does speed up the cached call by 400%.

In addition, move moduleLoadList creation and management entirely into
JS-land as it requires less code and is more efficient.

PR-URL: #18365
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Currently, both process.binding and internalBinding have to call into
C++ regardless of whether the module has been cached or not. This
creates significant overhead to all binding calls and unfortunately
the rest of the codebase doesn't really optimize the amount of times
that bindings are required (as an example: 12 files require the
async_wrap binding).

Changing all the usage of this function throughout the codebase would
introduce a lot of churn (and is kind of a hassle) so instead this PR
introduces a JS fast path for both functions for cases where the
binding has already been cached. While micro benchmarks are not super
meaningful here (it's not like we call binding that often...), this
does speed up the cached call by 400%.

In addition, move moduleLoadList creation and management entirely into
JS-land as it requires less code and is more efficient.

PR-URL: #18365
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

Does this make sense for LTS?

@joyeecheung
Copy link
Member

joyeecheung commented Mar 15, 2018

@MylesBorins I think it makes sense to backport this to v8.x, I am trying to backport #19112 to v8.x but a test fails because it depends on this to get a complete fix. @apapirovski are you working on a backport? I can help backporting this one in order to get #19112 backported.

joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Mar 15, 2018
Currently, both process.binding and internalBinding have to call into
C++ regardless of whether the module has been cached or not. This
creates significant overhead to all binding calls and unfortunately
the rest of the codebase doesn't really optimize the amount of times
that bindings are required (as an example: 12 files require the
async_wrap binding).

Changing all the usage of this function throughout the codebase would
introduce a lot of churn (and is kind of a hassle) so instead this PR
introduces a JS fast path for both functions for cases where the
binding has already been cached. While micro benchmarks are not super
meaningful here (it's not like we call binding that often...), this
does speed up the cached call by 400%.

In addition, move moduleLoadList creation and management entirely into
JS-land as it requires less code and is more efficient.

PR-URL: nodejs#18365
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

@joyeecheung if you could backport to 8.x that would be great. should we set dont-land-v6.x?

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Currently, both process.binding and internalBinding have to call into
C++ regardless of whether the module has been cached or not. This
creates significant overhead to all binding calls and unfortunately
the rest of the codebase doesn't really optimize the amount of times
that bindings are required (as an example: 12 files require the
async_wrap binding).

Changing all the usage of this function throughout the codebase would
introduce a lot of churn (and is kind of a hassle) so instead this PR
introduces a JS fast path for both functions for cases where the
binding has already been cached. While micro benchmarks are not super
meaningful here (it's not like we call binding that often...), this
does speed up the cached call by 400%.

In addition, move moduleLoadList creation and management entirely into
JS-land as it requires less code and is more efficient.

PR-URL: nodejs#18365
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

ping @joyeecheung regarding a 8.x backport

should include #19112 and #19117

joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jun 13, 2018
Currently, both process.binding and internalBinding have to call into
C++ regardless of whether the module has been cached or not. This
creates significant overhead to all binding calls and unfortunately
the rest of the codebase doesn't really optimize the amount of times
that bindings are required (as an example: 12 files require the
async_wrap binding).

Changing all the usage of this function throughout the codebase would
introduce a lot of churn (and is kind of a hassle) so instead this PR
introduces a JS fast path for both functions for cases where the
binding has already been cached. While micro benchmarks are not super
meaningful here (it's not like we call binding that often...), this
does speed up the cached call by 400%.

In addition, move moduleLoadList creation and management entirely into
JS-land as it requires less code and is more efficient.

PR-URL: nodejs#18365
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants