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

dharesign · 2022-12-01T17:56:59Z

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

nodejs-github-bot · 2022-12-01T17:57:03Z

Review requested:

@nodejs/startup

src/node.cc

src/node_main_instance.cc

tools/install.py

src/node_main_instance.cc

dharesign · 2022-12-12T15:31:42Z

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

src/api/embed_helpers.cc

src/node_main_instance.cc

nornagon · 2022-12-12T18:17:16Z

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).

dharesign · 2022-12-12T18:53:37Z

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

dharesign · 2022-12-12T22:32:11Z

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

dharesign · 2022-12-12T22:34:05Z

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.

src/api/embed_helpers.cc

src/node.cc

src/node_main_instance.cc

tools/install.py

dharesign · 2022-12-13T02:04:13Z

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.

nodejs-github-bot · 2022-12-13T15:46:39Z

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

dharesign · 2022-12-13T16:17:31Z

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.

dharesign · 2022-12-13T16:33:53Z

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

dharesign · 2022-12-13T16:39:18Z

Submitted the following v8 bugs:

dharesign · 2022-12-13T17:57:10Z

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.

dharesign · 2022-12-13T17:58:30Z

Please re-approve running the workflows 😅

nodejs-github-bot · 2023-08-11T00:43:52Z

Landed in 7f2c810

joyeecheung · 2023-08-11T15:43:39Z

Note to backporters: this depends on #48660

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 <[email protected]> Refs: nodejs#40786 PR-URL: nodejs#45704 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

UlisesGascon · 2023-08-14T14:39:15Z

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 <[email protected]> Commit-Queue: Michael Lippautz <[email protected]> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>