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

vm: lazily initialize primordials for vm contexts #31738

Closed

Conversation

joyeecheung
Copy link
Member

Lazily initialize primordials when cross-context support for
builtins is needed to fix the performance regression in context
creation.

Fixes: #29842

local benchmark results

                            confidence improvement accuracy (*)    (**)   (***)
 vm/create-context.js n=100        ***    485.26 %      ±15.48% ±21.32% ±29.29%
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

Lazily initialize primordials when cross-context support for
builtins is needed to fix the performance regression in context
creation.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 11, 2020
return InitializePrimordials(context);
}

bool InitializePrimordials(Local<Context> context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This block contains only indentation change

@nodejs-github-bot
Copy link
Collaborator

@@ -407,6 +407,7 @@ MaybeLocal<Object> GetPerContextExports(Local<Context> context) {
Local<Object> exports = Object::New(isolate);
if (context->Global()->SetPrivate(context, key, exports).IsNothing())
return MaybeLocal<Object>();
InitializePrimordials(context);
Copy link
Member Author

@joyeecheung joyeecheung Feb 11, 2020

Choose a reason for hiding this comment

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

This is only hit for vm contexts and main contexts created without snapshot support so there's no additional cost for the main context built with snapshot support

@joyeecheung
Copy link
Member Author

@@ -405,7 +405,8 @@ MaybeLocal<Object> GetPerContextExports(Local<Context> context) {
return handle_scope.Escape(existing_value.As<Object>());

Local<Object> exports = Object::New(isolate);
if (context->Global()->SetPrivate(context, key, exports).IsNothing())
if (context->Global()->SetPrivate(context, key, exports).IsNothing() ||
!InitializePrimordials(context))
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, forgot to handle the return value of InitializePrimordials in the initial commit, fixed.

@addaleax
Copy link
Member

16:41:47                             confidence improvement accuracy (*)    (**)   (***)
16:41:47  vm/create-context.js n=100        ***    371.94 %      ±21.00% ±28.29% ±37.54%

🙂

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2020
@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Feb 19, 2020
Lazily initialize primordials when cross-context support for
builtins is needed to fix the performance regression in context
creation.

PR-URL: #31738
Fixes: #29842
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in e6c2277. Thanks!

codebytere pushed a commit that referenced this pull request Feb 27, 2020
Lazily initialize primordials when cross-context support for
builtins is needed to fix the performance regression in context
creation.

PR-URL: #31738
Fixes: #29842
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
@codebytere
Copy link
Member

@joyeecheung if this should go to v12.x can you please open a manual backport? There are some conflicts i'd rather not chance myself.

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Lazily initialize primordials when cross-context support for
builtins is needed to fix the performance regression in context
creation.

PR-URL: nodejs#31738
Fixes: nodejs#29842
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Lazily initialize primordials when cross-context support for
builtins is needed to fix the performance regression in context
creation.

PR-URL: #31738
Fixes: #29842
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Significant slowdown of createContext and runInContext
7 participants