src: remove dependency on wrapper-descriptor-based cpp heap #284

joyeecheung · 2024-05-20T15:44:21Z

As V8 is moving towards built-in CppHeap creation, change the management so that the automatic CppHeap creation on Node.js's end is also enforced at Isolate creation time.

If embedder uses NewIsolate(), either they use IsolateSettings::cpp_heap to specify a CppHeap that will be owned by V8, or if it's not configured, Node.js will create a CppHeap that will be owned by V8.
If the embedder uses SetIsolateUpForNode(), IsolateSettings::cpp_heap will be ignored (as V8 has deprecated attaching CppHeap post-isolate-creation). The embedders need to ensure that the v8::Isolate has a CppHeap attached while it's still used by Node.js, preferably using v8::CreateParams.

See https://issues.chromium.org/issues/42203693 for details. In future version of V8, this CppHeap will be created by V8 if not provided, and we can remove our own "if no CppHeap provided, create one" code in NewIsolate().

This also deprecates node::SetCppgcReference() in favor of v8::Object::Wrap() since the wrapper descriptor is no longer relevant. It is still kept as a compatibility layer for addons that need to also work on Node.js versions without v8::Object::Wrap().

Fixes: #283
Fixes: nodejs/node#52718

Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 12.7. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

dllexport introduces issues when compiling with MSVC. PR-URL: nodejs/node#47251 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

It introduces process hangs on some platforms because Node.js doesn't tear down V8 correctly. Disable it while we work on a solution. Refs: nodejs/node#47297 Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13902 PR-URL: nodejs/node#47450 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14221 PR-URL: nodejs/node#49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

We are not ready to compile with C++20 support yet. This is only a DCHECK that can be removed without affecting the behavior of release builds. PR-URL: nodejs/node#49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs/node#49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs/node#52293 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

After enabling -std:c++20 on Windows, patch is now much smaller. PR-URL: nodejs/node#52465 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

On Windows debug builds, it is not allowed to dereference empty iterators. Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5331688 PR-URL: nodejs/node#52465 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Namely: - `v8::ObjectTemplate::SetAccessor(v8::Local<v8::String>, ...);` - `v8::ObjectTemplate::SetNativeDataProperty` with `AccessControl` Refs: v8/v8@46c241e Refs: v8/v8@6ec8839 Co-authored-by: Michaël Zasso <targos@protonmail.com>

Refs: v8/v8@5495678

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5300826

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5447267 Co-authored-by: Michaël Zasso <targos@protonmail.com>

Refs: v8/v8@e30e228

Refs: v8/v8@f8fa220

As V8 is moving towards built-in CppHeap creation, change the management so that the automatic CppHeap creation on Node.js's end is also enforced at Isolate creation time. 1. If embedder uses NewIsolate(), either they use IsolateSettings::cpp_heap to specify a CppHeap that will be owned by V8, or if it's not configured, Node.js will create a CppHeap that will be owned by V8. 2. If the embedder uses SetIsolateUpForNode(), IsolateSettings::cpp_heap will be ignored (as V8 has deprecated attaching CppHeap post-isolate-creation). The embedders need to ensure that the v8::Isolate has a CppHeap attached while it's still used by Node.js, preferably using v8::CreateParams. See https://issues.chromium.org/issues/42203693 for details. In future version of V8, this CppHeap will be created by V8 if not provided, and we can remove our own "if no CppHeap provided, create one" code in NewIsolate().

joyeecheung · 2024-05-20T15:45:00Z

cc @codebytere @addaleax @legendecas

src/node.h

src/env.cc

test/cctest/test_cppgc.cc

src/env.cc

addaleax · 2024-05-20T22:01:35Z

src/node.h

+  //
+  // See https://issues.chromium.org/issues/42203693. In future version
+  // of V8, this CppHeap will be created by V8 if not provided.
+  v8::CppHeap* cpp_heap = nullptr;


This needs a flag in IsolateSettingsFlags (similar to e.g. ALLOW_MODIFY_CODE_GENERATION_FROM_STRINGS_CALLBACK), as otherwise it's ABI-breaking? Or do you want to do that when backporting this change?

I think the new tracing mechanism is already ABI breaking (wrappers are now getting a new layout and a slot in their headers), so this can't be backported anyway?

Also this is part of the migration that builds on top of the new v8::Object::Wrap/v8::Object::Unwrap APIs that are replacement of the deprecated APIs, which won't be available until 12.5 with https://issues.chromium.org/issues/328117814 (I suspect the series of changes are already ABI-breaking in themselves somehow....or it would be very difficult to backport all the related patches in a non-ABI breaking way to older releases), this PR is baking for the 12.5 upgrade, see #283

targos · 2024-05-21T08:11:01Z

@joyeecheung The canary branch in this repo is updated/force-pushed automatically every day. We need to land canary patches on https://github.com/nodejs/node/tree/canary-base (I just updated it in case you need very recent V8 changes for your patch).

joyeecheung · 2024-05-21T15:31:08Z

Thanks for the reminder, moving to nodejs/node#53086

nodejs-github-bot and others added 18 commits May 20, 2024 06:03

deps: update V8 to 12.7.81

762866b

build: reset embedder string to "-node.0"

7cc3286

src: update NODE_MODULE_VERSION to 128

b40b861

Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 12.7. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

deps: patch V8 to avoid duplicated zlib symbol

c4073d4

PR-URL: nodejs/node#49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

deps: silence internal V8 deprecation warning

0cf4413

PR-URL: nodejs/node#52293 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

src: do not use deprecated V8 API

c04e097

Namely: - `v8::ObjectTemplate::SetAccessor(v8::Local<v8::String>, ...);` - `v8::ObjectTemplate::SetNativeDataProperty` with `AccessControl` Refs: v8/v8@46c241e Refs: v8/v8@6ec8839 Co-authored-by: Michaël Zasso <targos@protonmail.com>

build: include v8-sandbox.h header in distribution

d76ae0d

Refs: v8/v8@5495678

src: add source location to v8::TaskRunner

aa408fa

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5300826

build: disable ICF for mksnapshot

b999e0e

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5447267 Co-authored-by: Michaël Zasso <targos@protonmail.com>

test: update v8-stats test for V8 12.6

c18002a

Refs: v8/v8@e30e228

tools: update V8 gypfiles for 12.6

667c0a5

Refs: v8/v8@f8fa220

joyeecheung commented May 20, 2024

View reviewed changes

src/node.h Show resolved Hide resolved

legendecas mentioned this pull request May 20, 2024

src: attach CppHeap to an v8::Isolate with isolate params nodejs/node#53038

Closed

fixup! src: remove dependency on wrapper-descriptor-based cpp heap

144cd73

legendecas approved these changes May 20, 2024

View reviewed changes

src/env.cc Outdated Show resolved Hide resolved

test/cctest/test_cppgc.cc Show resolved Hide resolved

joyeecheung commented May 20, 2024

View reviewed changes

src/env.cc Outdated Show resolved Hide resolved

fixup! src: remove dependency on wrapper-descriptor-based cpp heap

5bfd1f2

addaleax reviewed May 20, 2024

View reviewed changes

nodejs-github-bot force-pushed the canary branch from 667c0a5 to 9dc716f Compare May 21, 2024 07:29

joyeecheung mentioned this pull request May 21, 2024

src: remove dependency on wrapper-descriptor-based cpp heap nodejs/node#53086

Closed

joyeecheung closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remove dependency on wrapper-descriptor-based cpp heap #284

src: remove dependency on wrapper-descriptor-based cpp heap #284

joyeecheung commented May 20, 2024 •

edited

Loading

joyeecheung commented May 20, 2024

addaleax May 20, 2024

joyeecheung May 21, 2024 •

edited

Loading

joyeecheung May 21, 2024 •

edited

Loading

targos commented May 21, 2024

joyeecheung commented May 21, 2024

src: remove dependency on wrapper-descriptor-based cpp heap #284

src: remove dependency on wrapper-descriptor-based cpp heap #284

Conversation

joyeecheung commented May 20, 2024 • edited Loading

joyeecheung commented May 20, 2024

addaleax May 20, 2024

Choose a reason for hiding this comment

joyeecheung May 21, 2024 • edited Loading

Choose a reason for hiding this comment

joyeecheung May 21, 2024 • edited Loading

Choose a reason for hiding this comment

targos commented May 21, 2024

joyeecheung commented May 21, 2024

joyeecheung commented May 20, 2024 •

edited

Loading

joyeecheung May 21, 2024 •

edited

Loading

joyeecheung May 21, 2024 •

edited

Loading