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: include vm context in the embedded snapshot #44252

Closed
wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 16, 2022

The first commit comes from #44258

vm: make ContextifyContext template context-independent

Instead of creating an object template for every ContextifyContext,
we now create one object template that can be reused by all
contexts. The native pointer can be obtained through an embdder
pointer field in the creation context of the receiver in the
interceptors, because the interceptors are only meant to be invoked
on the global object of the contextified contexts. This makes
the ContextifyContext template context-independent and therefore
snapshotable.

vm: include vm context in the embedded snapshot

Include a minimally initialized contextify context in the embedded
snapshot. This paves the way for user-land vm context snapshots.

vm: avoid unnecessary property getter interceptor calls

Access to the global object from within a vm context is intercepted
so it's slow, therefore we should try to avoid unnecessary access
to it during the initialization of vm contexts.

  • Remove the Atomics.wake deletion as V8 now does not install it
    anymore.
  • Move the Intl.v8BreakIterator deletion into the snapshot.
  • Do not query the Object prototype if --disable-proto is not set.

This should speed up the creation of vm contexts by about ~12%.

Refs: #44014
Refs: #37476

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@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. needs-ci PRs that need a full CI run. labels Aug 16, 2022
@mscdex
Copy link
Contributor

mscdex commented Aug 16, 2022

s/embeded/embedded in commit message

src/node_contextify.cc Show resolved Hide resolved
src/node_contextify.cc Outdated Show resolved Hide resolved
src/node_contextify.cc Show resolved Hide resolved
@joyeecheung joyeecheung force-pushed the snapshot-context branch 2 times, most recently from c32efba to 5e2a212 Compare August 17, 2022 10:25
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 17, 2022

Rebased on top of #44258 and added a commit that makes use of the snapshot to speed up the initialization of the vm contexts a bit.

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

From the CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1174

20:50:54                                                                       confidence improvement accuracy (*)    (**)   (***)
20:50:55 vm/create-context.js n=1000                                                  ***      7.34 %       ±2.99%  ±3.97%  ±5.17%
20:50:55 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=1000                     -6.14 %       ±6.57%  ±8.75% ±11.40%
20:50:55 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=1000                      1.28 %      ±12.90% ±17.18% ±22.38%
20:50:55 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=1000                     -6.66 %       ±8.39% ±11.16% ±14.53%
20:50:55 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=1000                     -1.86 %      ±11.60% ±15.44% ±20.10%
20:50:55 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=1000                -0.87 %       ±7.80% ±10.38% ±13.52%
20:50:55 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=1000                 3.06 %      ±22.00% ±29.27% ±38.10%
20:50:55 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=1000                -1.53 %       ±9.74% ±12.96% ±16.87%
20:50:55 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=1000                -0.26 %      ±12.00% ±15.97% ±20.79%

It seems that I needed to bump up the number of iterations (--set n=1000) to get a stable number from the benchmarks though.

@mscdex mscdex changed the title vm: include vm context in the embeded snapshot vm: include vm context in the embedded snapshot Aug 17, 2022
@joyeecheung joyeecheung added review wanted PRs that need reviews. vm Issues and PRs related to the vm subsystem. labels Aug 18, 2022
@goloveychuk
Copy link

goloveychuk commented Aug 18, 2022

I've found that (inside build snapshot)

const vm = require('vm')

const context = vm.createContext();
console.log(context, vm.isContext(context))
const global2 = vm.runInContext(
    'this',
    context,
)

throws an error

TypeError: The "contextifiedObject" argument must be an vm.Context. Received an instance of Object

should it be fixed in this PR or next?

@joyeecheung
Copy link
Member Author

@goloveychuk My plan is to implement user-land vm context it in another PR (because that requires more refactoring to the lifetime management of the bindings), this only adds support for vm context in the embedded snapshot, which isn't that complex.

@goloveychuk

This comment was marked as off-topic.

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Aug 18, 2022
@joyeecheung

This comment was marked as off-topic.

@goloveychuk

This comment was marked as off-topic.

@joyeecheung

This comment was marked as off-topic.

@joyeecheung
Copy link
Member Author

Opened #44277 and transferred the discussions there, in case this PR goes too off-topic. @goloveychuk please use that thread for discussions about user-land modules instead, thanks

legendecas added a commit that referenced this pull request Aug 19, 2022
Extract common context embedder tag checks to ContextEmbedderTag so
that verifying if a context is a node context doesn't need to access
private members of node::Environment.

PR-URL: #44258
Refs: #44179
Refs: #44252
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Instead of creating an object template for every ContextifyContext,
we now create one object template that can be reused by all
contexts. The native pointer can be obtained through an embdder
pointer field in the creation context of the receiver in the
interceptors, because the interceptors are only meant to be invoked
on the global object of the contextified contexts. This makes
the ContextifyContext template context-independent and therefore
snapshotable.
Include a minimally initialized contextify context in the embedded
snapshot. This paves the way for user-land vm context snapshots.
Access to the global object from within a vm context is intercepted
so it's slow, therefore we should try to avoid unnecessary access
to it during the initialization of vm contexts.

- Remove the Atomics.wake deletion as V8 now does not install it
  anymore.
- Move the Intl.v8BreakIterator deletion into the snapshot.
- Do not query the Object prototype if --disable-proto is not set.

This should speed up the creation of vm contexts by about ~12%.
RafaelGSS added a commit that referenced this pull request Sep 5, 2022
Notable changes:

* cli:
  * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366
* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 5, 2022
Notable changes:

* cli:
  * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366
* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 6, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 6, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) [#44355](#44355)
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) [#44045](#44045)
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) [#44416](#44416)
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) [#44255](#44255)
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) [#44362](#44362)
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) [#44252](#44252)

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 6, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Sep 6, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) nodejs#44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) nodejs#44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) nodejs#44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) nodejs#44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) nodejs#44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) nodejs#44252

PR-URL: nodejs#44521
RafaelGSS added a commit that referenced this pull request Sep 6, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) nodejs#44355
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) nodejs#44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) nodejs#44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) nodejs#44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) nodejs#44252

PR-URL: nodejs#44521
RafaelGSS added a commit that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 7, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
RafaelGSS added a commit that referenced this pull request Sep 8, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) #44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) #44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) #44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) #44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) #44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) #44252

PR-URL: #44521
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Extract common context embedder tag checks to ContextEmbedderTag so
that verifying if a context is a node context doesn't need to access
private members of node::Environment.

PR-URL: nodejs#44258
Refs: nodejs#44179
Refs: nodejs#44252
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Instead of creating an object template for every ContextifyContext,
we now create one object template that can be reused by all
contexts. The native pointer can be obtained through an embdder
pointer field in the creation context of the receiver in the
interceptors, because the interceptors are only meant to be invoked
on the global object of the contextified contexts. This makes
the ContextifyContext template context-independent and therefore
snapshotable.

PR-URL: nodejs#44252
Refs: nodejs#44014
Refs: nodejs#37476
Reviewed-By: Chengzhong Wu <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Include a minimally initialized contextify context in the embedded
snapshot. This paves the way for user-land vm context snapshots.

PR-URL: nodejs#44252
Refs: nodejs#44014
Refs: nodejs#37476
Reviewed-By: Chengzhong Wu <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Access to the global object from within a vm context is intercepted
so it's slow, therefore we should try to avoid unnecessary access
to it during the initialization of vm contexts.

- Remove the Atomics.wake deletion as V8 now does not install it
  anymore.
- Move the Intl.v8BreakIterator deletion into the snapshot.
- Do not query the Object prototype if --disable-proto is not set.

This should speed up the creation of vm contexts by about ~12%.

PR-URL: nodejs#44252
Refs: nodejs#44014
Refs: nodejs#37476
Reviewed-By: Chengzhong Wu <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
Notable changes:

* doc:
  * add daeyeon to collaborators (Daeyeon Jeong) nodejs#44355
* lib:
  * (SEMVER-MINOR) add diagnostics channel for process and worker (theanarkh) nodejs#44045
* os:
  * (SEMVER-MINOR) add machine method (theanarkh) nodejs#44416
* report:
  * (SEMVER-MINOR) expose report public native apis (Chengzhong Wu) nodejs#44255
* src:
  * (SEMVER-MINOR) expose environment RequestInterrupt api (Chengzhong Wu) nodejs#44362
* vm:
  * include vm context in the embedded snapshot (Joyee Cheung) nodejs#44252

PR-URL: nodejs#44521
@juanarbol
Copy link
Member

This is not landing cleanly in v16.x

juanarbol pushed a commit that referenced this pull request Oct 10, 2022
Extract common context embedder tag checks to ContextEmbedderTag so
that verifying if a context is a node context doesn't need to access
private members of node::Environment.

PR-URL: #44258
Refs: #44179
Refs: #44252
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
Extract common context embedder tag checks to ContextEmbedderTag so
that verifying if a context is a node context doesn't need to access
private members of node::Environment.

PR-URL: #44258
Refs: #44179
Refs: #44252
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
Extract common context embedder tag checks to ContextEmbedderTag so
that verifying if a context is a node context doesn't need to access
private members of node::Environment.

PR-URL: nodejs/node#44258
Refs: nodejs/node#44179
Refs: nodejs/node#44252
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
Extract common context embedder tag checks to ContextEmbedderTag so
that verifying if a context is a node context doesn't need to access
private members of node::Environment.

PR-URL: nodejs/node#44258
Refs: nodejs/node#44179
Refs: nodejs/node#44252
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
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. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants