-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: migrate ContextifyScript to cppgc #52295
Conversation
Review requested:
|
Refs: nodejs/node#52295 Change-Id: If0e34519c4fa83b019a0a67136e6515d85810a19 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5403888 Commit-Queue: Joyee Cheung <joyee@igalia.com> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#93690}
https://chromium-review.googlesource.com/c/v8/v8/+/5403888 has landed but given that the upstream has deprecated the current wrapper descriptor-based integration and the new API requires a series of V8 changes to upgrade to, I think it would minify the risk of bugs if we wait until 12.5 is pulled in. |
Rebased after v8 upgrade landed. New benchmark numbers:
Still need to finish some documentation about the helpers.. |
a2cc905
to
e94cf59
Compare
68814ab
to
ed0cffb
Compare
cc @nodejs/cpp-reviewers @mcollina @legendecas |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52295 +/- ##
=======================================
Coverage 87.30% 87.31%
=======================================
Files 649 650 +1
Lines 182706 182734 +28
Branches 35036 35042 +6
=======================================
+ Hits 159516 159548 +32
+ Misses 16456 16453 -3
+ Partials 6734 6733 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: #52295 Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This will need some patching to work on older versions of V8, applying the dont-land labels for now. |
template <typename T> | ||
static inline T* Unwrap(v8::Local<v8::Value> obj) { | ||
return BaseObject::FromJSObject<T>(obj); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung Might as well have removed this entirely, the point of this was to provide a shorthand but it's not really serving that purpose anymore :)
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Use a std::set<> for saving the JSGraphJSNode, since implementing a proper hash function for v8::Data is complicated and this path is only used by tests anyway, where the performance difference between std::set and std::unordered_set doesn't matter. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch adds helpers for wrapper classes based on cppgc (Oilpan) in `src/cppgc_helpers.h`, including `node::CppgcMixin` and `ASSIGN_OR_RETURN_UNWRAP_CPPGC`, which are designed to have similar interface to BaseObject helpers to help migration. They are documented in the `CppgcMixin` section in `src/README.md` To disambiguate, the global `node::Unwrap<>` has now been moved as `node::BaseObject::Unwrap<>`, and `node::Cppgc::Unwrap<>` implements a similar unwrapping mechanism for cppgc-managed wrappers. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This PR includes two commits. One adds a helper mixin
node::CppgcMixin
tosrc/cppgc_helpers.h
along with some documentation insrc/README.md
to facilitate migration fromBaseObject
tocppgc
-based memory management (Oilpan), and another migrates ContextifyScript to cppgc which will be the first wrapper class in Node.js to use it. This class is chosen because it doesn't have any externally managed data - most other wrapper classes do and they will need to wait for https://chromium-review.googlesource.com/c/v8/v8/+/5630497.For more information about migrating to Oilpan (cppgc) in Node.js, see the design doc and Chromium's documentation on Oilpan
Refs: #40786
Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit
Local benchmark numbers show a small improvement in compiling small scripts, likely due to improved GC performance: