vm: migrate ContextifyScript to cppgc by joyeecheung · Pull Request #52295 · nodejs/node · GitHub

This PR includes two commits. One adds a helper mixin node::CppgcMixin to src/cppgc_helpers.h along with some documentation in src/README.md to facilitate migration from BaseObject to cppgc-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:

                                                                                                                                    confidence improvement accuracy (*)   (**)  (***)
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/snapshot/typescript.js' type='with-dynamic-import-callback'                    0.37 %       ±0.51% ±0.69% ±0.91%
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/snapshot/typescript.js' type='without-dynamic-import-callback'          *      1.01 %       ±0.87% ±1.18% ±1.56%
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/syntax/good_syntax.js' type='with-dynamic-import-callback'            ***      6.45 %       ±1.39% ±1.86% ±2.44%
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/syntax/good_syntax.js' type='without-dynamic-import-callback'         ***      8.69 %       ±0.99% ±1.32% ±1.72%

Review requested:

@nodejs/gyp
@nodejs/security-wg
@nodejs/v8-update

Blocked on https://chromium-review.googlesource.com/c/v8/v8/+/5403888 (and maybe https://chromium-review.googlesource.com/c/v8/v8/+/5447348 too)

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:

❯ node-benchmark-compare script.csv
                                                                                                                                    confidence improvement accuracy (*)   (**)  (***)
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/snapshot/typescript.js' type='with-dynamic-import-callback'                    0.37 %       ±0.51% ±0.69% ±0.91%
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/snapshot/typescript.js' type='without-dynamic-import-callback'          *      1.01 %       ±0.87% ±1.18% ±1.56%
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/syntax/good_syntax.js' type='with-dynamic-import-callback'            ***      6.45 %       ±1.39% ±1.86% ±2.44%
vm/compile-script-in-isolate-cache.js n=1000 filename='test/fixtures/syntax/good_syntax.js' type='without-dynamic-import-callback'         ***      8.69 %       ±0.99% ±1.32% ±1.72%

Still need to finish some documentation about the helpers..

cc @nodejs/cpp-reviewers @mcollina @legendecas

CI: https://ci.nodejs.org/job/node-test-pull-request/61347/

Codecov Report

Attention: Patch coverage is 82.60870% with 12 lines in your changes missing coverage. Please review.

Project coverage is 87.31%. Comparing base (097dcfc) to head (b08778a).
Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
src/heap_utils.cc	66.66%	2 Missing and 3 partials ⚠️
src/cppgc_helpers.h	84.21%	1 Missing and 2 partials ⚠️
src/node_contextify.cc	85.71%	0 Missing and 3 partials ⚠️
src/node_contextify.h	0.00%	1 Missing ⚠️

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

Files with missing lines	Coverage Δ
src/base_object.h	88.88% <100.00%> (ø)
src/crypto/crypto_keys.cc	72.50% <100.00%> (-0.30%)	⬇️
src/crypto/crypto_util.cc	68.35% <100.00%> (+0.08%)	⬆️
src/node_file-inl.h	88.08% <100.00%> (ø)
src/node_messaging.cc	84.07% <100.00%> (+0.01%)	⬆️
src/stream_wrap.cc	87.61% <100.00%> (ø)
src/udp_wrap.cc	78.66% <100.00%> (ø)
src/node_contextify.h	75.00% <0.00%> (-10.72%)	⬇️
src/cppgc_helpers.h	84.21% <84.21%> (ø)
src/node_contextify.cc	80.73% <85.71%> (+0.18%)	⬆️
... and 1 more

... and 25 files with indirect coverage changes

lgtm

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.

@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 :)

nodejs-github-bot added 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 Mar 31, 2024

joyeecheung mentioned this pull request Mar 31, 2024

Usage of Oilpan / V8 C++ Garbage Collector #40786

Closed

This was referenced May 13, 2024

vm: add experimental NodeRealm implementation #47855

Closed

v8::WrapperDescriptor is deprecated nodejs/node-v8#283

Closed

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jun 18, 2024

joyeecheung force-pushed the cppgc-data branch from a2cc905 to e94cf59 Compare August 18, 2024 01:40

joyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Aug 18, 2024

joyeecheung mentioned this pull request Aug 18, 2024

wip: crypto: use cppgc to manage Hash #51017

Closed

joyeecheung force-pushed the cppgc-data branch 4 times, most recently from 68814ab to ed0cffb Compare August 22, 2024 13:59

joyeecheung changed the title wip: migrate ContextifyScript to cppgc vm: migrate ContextifyScript to cppgc Aug 22, 2024

joyeecheung marked this pull request as ready for review August 22, 2024 14:05

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2024

mcollina approved these changes Aug 22, 2024

View reviewed changes

legendecas reviewed Aug 22, 2024

View reviewed changes

Comment thread src/node_contextify.h Outdated

Comment thread src/cppgc_helpers.h Outdated

legendecas reviewed Aug 22, 2024

View reviewed changes

Comment thread src/node_contextify.cc Outdated

legendecas reviewed Aug 22, 2024

View reviewed changes

Comment thread src/cppgc_helpers.h Outdated

jasnell reviewed Aug 23, 2024

View reviewed changes

Comment thread src/node_contextify.h Outdated

nodejs-github-bot closed this Aug 30, 2024

RafaelGSS mentioned this pull request Aug 30, 2024

v22.8.0 proposal #54560

Merged

joyeecheung added dont-land-on-v18.x dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Aug 30, 2024

addaleax reviewed Aug 30, 2024

View reviewed changes

aduh95 mentioned this pull request Sep 12, 2024

doc: fix typo in CppgcMixin docs #54762

Merged

aduh95 mentioned this pull request Oct 14, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

IlyasShabi mentioned this pull request Jul 28, 2026

src: fix heap value deduplication in embedder graph #64801

Merged

+                template <typename T>
+                static inline T* Unwrap(v8::Local<v8::Value> obj) {
+                  return BaseObject::FromJSObject<T>(obj);
+                }

vm: migrate ContextifyScript to cppgc by joyeecheung · Pull Request #52295 · nodejs/node · GitHub

Uh oh!

Uh oh!

vm: migrate ContextifyScript to cppgc - #52295

vm: migrate ContextifyScript to cppgc#52295joyeecheung wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom joyeecheung:cppgc-datajoyeecheung/node:cppgc-dataCopy head branch name to clipboard

Conversation

joyeecheung commented Mar 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 31, 2024

Uh oh!

joyeecheung commented Apr 12, 2024

Uh oh!

joyeecheung commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Aug 18, 2024

Uh oh!

joyeecheung commented Aug 22, 2024

Uh oh!

nodejs-github-bot commented Aug 22, 2024

Uh oh!

codecov Bot commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Aug 30, 2024

Uh oh!

addaleax Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vm: migrate ContextifyScript to cppgc#52295
joyeecheung wants to merge 3 commits into
nodejs:mainfrom
joyeecheung:cppgc-data

joyeecheung commented Mar 31, 2024 •
edited
Loading

joyeecheung commented May 3, 2024 •
edited
Loading

codecov Bot commented Aug 22, 2024 •
edited
Loading