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: make ContextifyContext a BaseObject #44796

Closed
wants to merge 5 commits into from

Conversation

joyeecheung
Copy link
Member

src: refactor BaseObject methods

  • Wrap the initialization of the kSlot and kEmbedderType fields
    into a BaseObject::SetInternalFields() method.
  • Move the tagging of kEmbedderType field into
    BaseObject::TagNodeObject()
  • Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
    that only needs IsolateData.
    This makes it easier to create BaseObject subclasses.

benchmark: add vm context global proxy benchmark

vm: make ContextifyContext a BaseObject

Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.

@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 Sep 26, 2022
Local<Context> context;
if (!args.This()->GetCreationContext().ToLocal(&context)) {
if (!object->GetCreationContext().ToLocal(&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.

I tried using a private symbol from the receiver to the wrapper and then unwrap to get to the ContextifyContext*, which is a bit slower than the current approach, so we are still using the context embedder field here. In any case, as long as the context is alive, the sandbox must be alive (through the kSandboxObject internal field), then the wrapper must be alive (through the contextify_context_private_symbol), so the ContextifyContext is also alive. On the other hand as long as the ContextifyContext is alive, the wrapper is alive, then the context must also be alive (through the constructor).

@legendecas legendecas added the vm Issues and PRs related to the vm subsystem. label Sep 26, 2022
src/node_contextify.h Outdated Show resolved Hide resolved
- Wrap the initialization of the kSlot and kEmbedderType fields
  into a BaseObject::SetInternalFields() method.
- Move the tagging of kEmbedderType field into
  BaseObject::TagNodeObject()
- Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
  that only needs IsolateData.
This makes it easier to create BaseObject subclasses.
Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.
This reverts commit b9042c87b60184a37d9c8674ec8c8b29b2a8ab5e.
@nodejs-github-bot
Copy link
Contributor

@legendecas
Copy link
Member

Linter is complaining.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot
Copy link
Contributor

jasnell
jasnell approved these changes Oct 2, 2022
@nodejs-github-bot
Copy link
Contributor

joyeecheung added a commit that referenced this pull request Oct 4, 2022
- Wrap the initialization of the kSlot and kEmbedderType fields
  into a BaseObject::SetInternalFields() method.
- Move the tagging of kEmbedderType field into
  BaseObject::TagNodeObject()
- Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
  that only needs IsolateData.
This makes it easier to create BaseObject subclasses.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Oct 4, 2022
PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Oct 4, 2022
Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in 00e5617...5b1bcf8

@joyeecheung joyeecheung closed this Oct 4, 2022
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Oct 10, 2022
@danielleadams
Copy link
Member

Hi @joyeecheung, can you open up a backport PR for this to v18.x-staging? Thank you.

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Oct 18, 2022
- Wrap the initialization of the kSlot and kEmbedderType fields
  into a BaseObject::SetInternalFields() method.
- Move the tagging of kEmbedderType field into
  BaseObject::TagNodeObject()
- Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
  that only needs IsolateData.
This makes it easier to create BaseObject subclasses.

PR-URL: nodejs#44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Oct 18, 2022
PR-URL: nodejs#44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Oct 18, 2022
Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.

PR-URL: nodejs#44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Oct 24, 2022
`contextify_global_private_symbol` is no longer used. It was used
to hold a new context's global object, but the approach has been
changed to hold a ContextifyContext wrapper using
`contextify_context_private_symbol`.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #45128
Refs: #44796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
`contextify_global_private_symbol` is no longer used. It was used
to hold a new context's global object, but the approach has been
changed to hold a ContextifyContext wrapper using
`contextify_context_private_symbol`.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #45128
Refs: #44796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
joyeecheung added a commit that referenced this pull request Nov 1, 2022
- Wrap the initialization of the kSlot and kEmbedderType fields
  into a BaseObject::SetInternalFields() method.
- Move the tagging of kEmbedderType field into
  BaseObject::TagNodeObject()
- Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
  that only needs IsolateData.
This makes it easier to create BaseObject subclasses.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Nov 1, 2022
Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Nov 4, 2022
- Wrap the initialization of the kSlot and kEmbedderType fields
  into a BaseObject::SetInternalFields() method.
- Move the tagging of kEmbedderType field into
  BaseObject::TagNodeObject()
- Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
  that only needs IsolateData.
This makes it easier to create BaseObject subclasses.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Nov 4, 2022
Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
`contextify_global_private_symbol` is no longer used. It was used
to hold a new context's global object, but the approach has been
changed to hold a ContextifyContext wrapper using
`contextify_context_private_symbol`.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #45128
Refs: #44796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
`contextify_global_private_symbol` is no longer used. It was used
to hold a new context's global object, but the approach has been
changed to hold a ContextifyContext wrapper using
`contextify_context_private_symbol`.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #45128
Refs: #44796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
`contextify_global_private_symbol` is no longer used. It was used
to hold a new context's global object, but the approach has been
changed to hold a ContextifyContext wrapper using
`contextify_context_private_symbol`.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #45128
Refs: #44796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
- Wrap the initialization of the kSlot and kEmbedderType fields
  into a BaseObject::SetInternalFields() method.
- Move the tagging of kEmbedderType field into
  BaseObject::TagNodeObject()
- Add a variant of BaseObject::MakeLazilyInitializedJSTemplate()
  that only needs IsolateData.
This makes it easier to create BaseObject subclasses.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
Instead of adding a reference to the ContextifyContext by using
a v8::External, we make ContextifyContext a weak BaseObject that
whose wrapper is referenced by the sandbox via a private symbol.
This makes it easier to snapshot the contexts, in addition to
reusing the BaseObject lifetime management for ContextifyContexts.

PR-URL: #44796
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
`contextify_global_private_symbol` is no longer used. It was used
to hold a new context's global object, but the approach has been
changed to hold a ContextifyContext wrapper using
`contextify_context_private_symbol`.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #45128
Refs: #44796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. 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. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants