src,tools: initialize cppgc by dharesign · Pull Request #45704 · nodejs/node · GitHub

This patch:

Initializes cppgc in InitializeOncePerProcess() when
kNoInitializeCppgc is not set
Create a CppHeap and attach it to the Isolate when
there isn't one already during IsolateData initialization.
The CppHeap is detached and terminated when IsolateData
is freed.
Publishes the cppgc headers in the tarball.

This allows C++ addons to start using cppgc to manage objects.

A helper node::SetCppgcReference() is also added to help addons
enable cppgc tracing in a user-defined object.

Refs: #40786
Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit

Review requested:

@nodejs/startup

In Electron, the cppgc heap will be initialized by Chromium. I don't think anything in this PR clashes with what Electron will want.

When they initialize node, they can use kNoInitializeCppgc here:
https://github.com/electron/electron/blob/4e661842879ae0cb01a9fc251d56c8cabc958466/shell/app/node_main.cc#L161-L165

They don't use NodeMainInstance or CommonEnvironmentSetup.

CC: @codebytere / @nornagon for awareness

FYI I believe we don't currently initialize cppgc in the main process in Electron (though Blink does of course initialize it). So we would want to be able to initialize Node with cppgc initialization in the main process (either by doing it ourselves or through this initialization option), and without cppgc initialization in the renderer process (since that would clash with Blink's initialization).

Erp, I hit the close button by mistake. Please re-approve running the workflows >_<

The previous test runs seemed to fail, and actually running my local build of node likewise fails. It seems to be due to a bug in v8:

https://github.com/v8/v8/blob/main/src/heap/cppgc/heap-base.cc#L104

I believe this should be delgate_ not delegate: the latter is moved from.

CC: @mlippautz

The previous test runs seemed to fail, and actually running my local build of node likewise fails. It seems to be due to a bug in v8:

https://github.com/v8/v8/blob/main/src/heap/cppgc/heap-base.cc#L104

I believe this should be delgate_ not delegate: the latter is moved from.

CC: @mlippautz

Yep, changing that fixes my local build.

The previous test runs seemed to fail, and actually running my local build of node likewise fails. It seems to be due to a bug in v8:
https://github.com/v8/v8/blob/main/src/heap/cppgc/heap-base.cc#L104
I believe this should be delgate_ not delegate: the latter is moved from.
CC: @mlippautz

Yep, changing that fixes my local build.

I added a second commit to this PR which fixes the above. We can switch the commit to cherry-pick the v8 commit once it gets fixed there. Hopefully the CI checks will proceed now.

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

Node is still segfaulting when I run it locally:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100a5c14c node`cppgc::internal::MarkerBase::EnterAtomicPause(cppgc::EmbedderStackState) [inlined] cppgc::internal::SingleThreadedHandle::Cancel(this=<unavailable>) at task-handle.h:27:20 [opt]
   24
   25  	  void Cancel() {
   26  	    DCHECK(is_cancelled_);
-> 27  	    *is_cancelled_ = true;
   28  	  }
   29
   30  	  bool IsCanceled() const {
Target 0: (node) stopped.
(lldb) frame sel 1
frame #1: 0x0000000100a5c144 node`cppgc::internal::MarkerBase::EnterAtomicPause(this=0x0000000106808200, stack_state=kNoHeapPointers) at marker.cc:260:33 [opt]
   257 	    // Cancel remaining incremental tasks. Concurrent marking jobs are left to
   258 	    // run in parallel with the atomic pause until the mutator thread runs out
   259 	    // of work.
-> 260 	    incremental_marking_handle_.Cancel();
   261 	    heap().stats_collector()->UnregisterObserver(
   262 	        incremental_marking_allocation_observer_.get());
   263 	    incremental_marking_allocation_observer_.reset();
(lldb) print incremental_marking_handle_
(cppgc::internal::MarkerBase::IncrementalMarkingTaskHandle) $0 = {
  is_cancelled_ = nullptr {
    __ptr_ = 0x0000000000000000
  }
}

It looks like foreground_task_runner_ is nullptr, so MarkerBase::ScheduleIncrementalTaskRunner exits early before initializing incremental_marking_handle_. Later, MarkerBase::EnterAtomicPause assumes that incremental_marking_handle_ is set such that it can call .Cancel(), but it's not.

Looks like there's no isolate_ set in the CppgcPlatformAdapter, which implies CppHeap::AttachIsolate has not been called, though it has.

After digging a bit, it's because CppHeap::AttachIsolate does static_cast<CppgcPlatformAdapter*>(platform()), where platform() in this case is actually PlatformWithPageAllocator. The static_cast is therefore invalid.

CC: @mlippautz

Submitted the following v8 bugs:

I added a commit to add SetIsolate as a virtual method on the cppgc::Platform protocol. Not sure if that's the direction v8 will go to fix the issue, but it should help get us past that issue and check the rest of the CI passes.

Please re-approve running the workflows 😅

Landed in 7f2c810

Note to backporters: this depends on #48660

This PR didn't land cleanly because of #48660 in Node 20.x. Can you backport it?

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

This patch: - Initializes cppgc in InitializeOncePerProcess() when kNoInitializeCppgc is not set - Create a CppHeap and attach it to the Isolate when there isn't one already during IsolateData initialization. The CppHeap is detached and terminated when IsolateData is freed. - Publishes the cppgc headers in the tarball. This allows C++ addons to start using cppgc to manage objects. A helper node::SetCppgcReference() is also added to help addons enable cppgc tracing in a user-defined object. Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> Refs: nodejs#40786 PR-URL: nodejs#45704 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

This patch: - Initializes cppgc in InitializeOncePerProcess() when kNoInitializeCppgc is not set - Create a CppHeap and attach it to the Isolate when there isn't one already during IsolateData initialization. The CppHeap is detached and terminated when IsolateData is freed. - Publishes the cppgc headers in the tarball. This allows C++ addons to start using cppgc to manage objects. A helper node::SetCppgcReference() is also added to help addons enable cppgc tracing in a user-defined object. Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> Refs: nodejs#40786 PR-URL: nodejs#45704 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

This patch: - Initializes cppgc in InitializeOncePerProcess() when kNoInitializeCppgc is not set - Create a CppHeap and attach it to the Isolate when there isn't one already during IsolateData initialization. The CppHeap is detached and terminated when IsolateData is freed. - Publishes the cppgc headers in the tarball. This allows C++ addons to start using cppgc to manage objects. A helper node::SetCppgcReference() is also added to help addons enable cppgc tracing in a user-defined object. Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> Refs: nodejs#40786 PR-URL: nodejs#45704 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

This patch: - Initializes cppgc in InitializeOncePerProcess() when kNoInitializeCppgc is not set - Create a CppHeap and attach it to the Isolate when there isn't one already during IsolateData initialization. The CppHeap is detached and terminated when IsolateData is freed. - Publishes the cppgc headers in the tarball. This allows C++ addons to start using cppgc to manage objects. A helper node::SetCppgcReference() is also added to help addons enable cppgc tracing in a user-defined object. Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> Refs: nodejs#40786 PR-URL: nodejs#45704 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: #43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: #45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: #48660 Backport-PR-URL: #49187 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit

This patch: - Initializes cppgc in InitializeOncePerProcess() when kNoInitializeCppgc is not set - Create a CppHeap and attach it to the Isolate when there isn't one already during IsolateData initialization. The CppHeap is detached and terminated when IsolateData is freed. - Publishes the cppgc headers in the tarball. This allows C++ addons to start using cppgc to manage objects. A helper node::SetCppgcReference() is also added to help addons enable cppgc tracing in a user-defined object. Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> Refs: #40786 PR-URL: #45704 Backport-PR-URL: #49187 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> PR-URL: #48660 Refs: v8/v8@9327503

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) #48660 and #45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49185

This change broke NodeJS on big-endian ppc64 on Linux for me. (see: #58277)

I don't think V8 tests for ppc64be on their CI, nor does Node.js support ppc64be + Linux, from the stack trace it looks like a V8 support issue and the fix probably won't be in Node.js.

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 Dec 1, 2022

dharesign mentioned this pull request Dec 1, 2022

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

Closed

dharesign commented Dec 1, 2022

View reviewed changes

Comment thread src/node.cc Outdated

dharesign commented Dec 1, 2022

View reviewed changes

Comment thread src/node_main_instance.cc Outdated

dharesign commented Dec 1, 2022

View reviewed changes

Comment thread tools/install.py Outdated

dharesign commented Dec 1, 2022

View reviewed changes

Comment thread src/node_main_instance.cc Outdated

dharesign force-pushed the initialize-cppgc branch from 662d1aa to 3860b21 Compare December 9, 2022 22:29

dharesign marked this pull request as ready for review December 10, 2022 01:00

dharesign force-pushed the initialize-cppgc branch from 3860b21 to 97c248b Compare December 12, 2022 15:16

addaleax reviewed Dec 12, 2022

View reviewed changes

Comment thread src/api/embed_helpers.cc Outdated

Comment thread src/node_main_instance.cc Outdated

dharesign closed this Dec 12, 2022

dharesign reopened this Dec 12, 2022

dharesign force-pushed the initialize-cppgc branch from 97c248b to a4f84b2 Compare December 12, 2022 19:09

joyeecheung reviewed Dec 12, 2022

View reviewed changes

Comment thread src/api/embed_helpers.cc Outdated

Comment thread src/node.cc Outdated

Comment thread src/node_main_instance.cc Outdated

Comment thread tools/install.py Outdated

dharesign force-pushed the initialize-cppgc branch from a4f84b2 to a00a060 Compare December 13, 2022 01:44

nodejs-github-bot merged commit 7f2c810 into nodejs:main Aug 11, 2023

RafaelGSS added the backport-requested-v20.x label Aug 14, 2023

joyeecheung mentioned this pull request Aug 15, 2023

[v20.x backport] cppgc related changes #49187

Closed

UlisesGascon mentioned this pull request Aug 22, 2023

v20.6.0 proposal #49185

Merged

RafaelGSS mentioned this pull request Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

src,tools: initialize cppgc by dharesign · Pull Request #45704 · nodejs/node · GitHub

Uh oh!

Uh oh!

src,tools: initialize cppgc - #45704

src,tools: initialize cppgc#45704nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom dharesign:initialize-cppgcdharesign/node:initialize-cppgcCopy head branch name to clipboard

Conversation

dharesign commented Dec 1, 2022 • edited by joyeecheung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dharesign commented Dec 12, 2022

Uh oh!

Uh oh!

Uh oh!

nornagon commented Dec 12, 2022

Uh oh!

dharesign commented Dec 12, 2022

Uh oh!

dharesign commented Dec 12, 2022

Uh oh!

dharesign commented Dec 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dharesign commented Dec 13, 2022

Uh oh!

nodejs-github-bot commented Dec 13, 2022

Uh oh!

dharesign commented Dec 13, 2022

Uh oh!

dharesign commented Dec 13, 2022

Uh oh!

dharesign commented Dec 13, 2022

Uh oh!

dharesign commented Dec 13, 2022

Uh oh!

dharesign commented Dec 13, 2022

Uh oh!

nodejs-github-bot commented Aug 11, 2023

Uh oh!

joyeecheung commented Aug 11, 2023

Uh oh!

UlisesGascon commented Aug 14, 2023

Uh oh!

glaubitz commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

src,tools: initialize cppgc#45704
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
dharesign:initialize-cppgc

dharesign commented Dec 1, 2022 •
edited by joyeecheung
Loading

glaubitz commented May 17, 2025 •
edited
Loading

joyeecheung commented May 18, 2025 •
edited
Loading