nodejs
vm: migrate ContextifyScript to cppgc
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: https://github.com/nodejs/node/issues/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)
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..
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.
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%) |
:arrow_down: |
| src/crypto/crypto_util.cc | 68.35% <100.00%> (+0.08%) |
:arrow_up: |
| src/node_file-inl.h | 88.08% <100.00%> (ø) |
|
| src/node_messaging.cc | 84.07% <100.00%> (+0.01%) |
:arrow_up: |
| 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%) |
:arrow_down: |
| src/cppgc_helpers.h | 84.21% <84.21%> (ø) |
|
| src/node_contextify.cc | 80.73% <85.71%> (+0.18%) |
:arrow_up: |
| ... and 1 more |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
The heap snapshot tests need to be updated since now we don't have a global list of them under Node / Environment (which is kind of the point) . It's probably simpler to just track them with a test fixture that generates a stack root referencing the targets.
Updated the test with a fixture modified from https://chromium.googlesource.com/v8/v8/+/b00e995fb212737802810384ba2b868d0d92f7e5/test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc#134 (I personally find it a lot easier to debug than the validateSnapshotNodes() helper that has been used to verify BaseObjects. Maybe it would be nice to migrate BaseObject tests over too).
It seems the new hash function supporting v8::Data for JSGraphNode doesn't work well with std::unordered_set in MSVC, but then we don't need to implement it if we use a std::set instead anyway, and this path is only used by the heap snapshot tests so the performance difference between std::unordered_set and std::set doesn't matter. Updated to just use std::set.
Landed in e9cd4766e39d96693320be9ce0a1044c450e8675...3b0617dd19070ebb59149b091452858aeb7b233d
This will need some patching to work on older versions of V8, applying the dont-land labels for now.