Skip to content

Conversation

@joyeecheung
Copy link
Member

@joyeecheungjoyeecheung 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-botnodejs-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

s/embeded/embedded in commit message

@joyeecheungjoyeecheungforce-pushed the snapshot-context branch 2 times, most recently from c32efba to 5e2a212CompareAugust 17, 2022 10:25
@joyeecheung
Copy link
MemberAuthor

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
MemberAuthor

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.

@mscdexmscdex changed the title vm: include vm context in the embeded snapshotvm: include vm context in the embedded snapshotAug 17, 2022
@joyeecheungjoyeecheung 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)

constvm=require('vm')constcontext=vm.createContext();console.log(context,vm.isContext(context))constglobal2=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
MemberAuthor

@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.

@joyeecheungjoyeecheung 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
MemberAuthor

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%.
@RafaelGSSRafaelGSS mentioned this pull request Sep 5, 2022
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 freeto 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 / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.review wantedPRs that need reviews.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@nodejs-github-bot@mscdex@goloveychuk@juanarbol@legendecas