src,tools: initialize cppgc #45704

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

Initialize cppgc, create a CppHeap, and attach it to the Isolate.

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

Refs: #40786

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

Review requested:

@nodejs/startup

src/node.cc

dharesign · 2022-12-01T18:02:49Z

src/node_main_instance.cc

+          WrapperDescriptor(
+              BaseObject::kEmbedderType,
+              BaseObject::kSlot,
+              0x90df


A recent commit made kEmbedderType and kSlot the correct values here (8b0e5b19bd0). The embedder id in that commit was specified as 0x90de. When Chromium initializes the CppHeap its embedder id is 0x1 (kEmbedderBlink). Pre-cppgc it used to be the case that for embedder tracing you had to specify the embedder id when you create objects, and it would need to match. With cppgc, you just use cppgc::MakeGarbageCollected<> and I believe it uses the values that were specified when the CppHeap was created. As a result, add-ons using cppgc don't need to be aware of the value chosen by Node. Given though that the reason 0x90de was picked was so that non-cppgc objects don't get mixed up with cppgc objects, we probably want to use a different embedder id than 0x90de which Node is currently using for non-cppgc objects. I just picked the next number here.

dharesign · 2022-12-01T18:06:46Z

tools/install.py

@@ -197,15 +197,56 @@ def files(action):
 def headers(action):
  def wanted_v8_headers(files_arg, dest):
    v8_headers = [
+      'deps/v8/include/cppgc/allocation.h',


It might be better to go back to excluding the headers you don't want to export?

Whilst I'm here though, I'll mention that we actually do make use of the v8-inspector.h and v8-inspector-protocol.h headers, so we have a local patch to include those also. Those were the only headers that used to be in the disallow list (See: 38f3238).

I think the concern is that we don't want to accidentally expand the API surface. If we have a disallow list we might fail to notice when V8 adds new headers and then breaks the API by removing them. If the list is explicit it helps us notice about changes like this and we can make some fixups or adjust the timing of the V8 upgrade.

I don't understand the subject enough to have an opinion on whether we should expose all these additional headers, but I'm afraid that doing so will effectively block us from doing semver-minor V8 upgrades in the future.

dharesign · 2022-12-01T18:07:44Z

src/node_main_instance.cc

@@ -84,6 +88,17 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
  SetIsolateCreateParamsForNode(isolate_params_.get());
  Isolate::Initialize(isolate_, *isolate_params_);

+  cpp_heap_ = CppHeap::Create(


Not sure if this should be optional, or conditional on the earlier kNoInitializeCppgc?

Here it doesn’t make a difference since for NodeMainInstance, we are in control of how the process is initialized. For the code in CommonEnvironmentSetup this is probably a valid question though – I assume this doesn’t work if Node.js was initialized with kNoInitializeCppgc? Does it crash or just do nothing?

I think it crashes.

InitializeProcess sets up the global GC info table: https://github.com/v8/v8/blob/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/src/heap/cppgc/platform.cc#LL63
https://github.com/v8/v8/blob/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/src/heap/cppgc/gc-info-table.cc#LL55

Later code will just assume it's available:
https://github.com/v8/v8/blob/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/src/heap/cppgc/gc-info-table.h#L104-L106

I guess the assumption with kNoInitializeNodeV8Platform is that the embedder will do the required initialization. Here it's going to be similar. But this is new initialization existing embedders will need to perform (if they're explicitly asking node not to).

Can you add a test for kNoInitializeCppgc, similar to the one in embedtest.cc? I assume doing CppHeap::Create() and Isolate:: AttachCppHeap() right after CommonEnvironmentSetup::Create() would be timely enough? It would be good to know when we add something in CommonEnvironmentSetup::Create() that makes it no longer the case though.

I think for NodeMainInstance() we can probably just do this unconditionally for the add-ons. It does not seem to be necessary for embedders who use NodeMainInstance() and also enable cppgc for add-ons (do they exist?) to customize this part.

I can add kNoInitializeCppgc to embedtest.cc and have it call cppgc::InitializeProcess manually. CppHeap::Create() and Isolate::AttachCppHeap() happen internally in CommonEnvironmentSetup however. Are you saying you don't want that to be the case?

I think it makes sense, if Node starts allowing embedders to use cppgc, to expect that embedders specifying kNoInitializeCppgc need to manually initialize it, similar to how they are currently required to initialize v8.

for embedders who use NodeMainInstance() and also enable cppgc for add-ons (do they exist?)

I wouldn’t spend too much time worrying about embedders who don’t stick to the public API 🙂

Are you saying you don't want that to be the case?

I meant that we can condition the CppHeap::Create() and Isolate::AttachCppHeap() calls in CommonEnvironmentSetup::Create() with kNoInitializeCppgc - so, users who set kNoInitializeCppgc would have to call these themselves after CommonEnvironmentSetup::Create(), which I think is what you meant by "Here it's going to be similar"? And if so we should test that doing something like:

auto setup = CommonEnvironmentSetup::Create(platform, errors, args, exec_args, node::EnvironmentFlags::kNoInitializeCppgc); auto cpp_heap = CppHeap::Create(...); setup->isolate()->AttachCppHeap(cpp_heap.get()); // Before CommonEnvironmentSetup goes out of scope setup->isolate()->DetachCppHeap(); cpp_heap->Terminate();

is actually working.

Oh, I misread, there is only one flag for the process initialization, so it's not propagated to environments. So I guess we can just add a flag for cppgc creation to IsolateSettingsFlags, and then do the calls unconditionally in CommonEnvironmentSetup::Create() (this means that the user cannot use CommonEnvironmentSetup if they want to configure the cppgc heap, but that's probably fine).

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

addaleax · 2022-12-12T18:15:17Z

src/api/embed_helpers.cc

+          WrapperDescriptor(
+              BaseObject::kEmbedderType,
+              BaseObject::kSlot,
+              0x90df)


Would embedders which don’t use CommonEnvironmentSetup have to re-use the same value here? What happens if they call InitializeOncePerProcess() but don’t call isolate->AttachCppHeap() (which is what currently would happen)?

Would it make sense to expose a function to embedders that creates (and attaches?) the CppHeap the same way it’s being done here, then use that here and in NodeMainInstance?

I don't think the exact value matters a whole lot. In Electron, Chromium will initialize the heap with 0x1. Node addons making use of cppgc don't need to specify the value, it's automatically set by cppgc::MakeGarbageCollected.

What does matter, is that it doesn't match the value used in the embedder fields for non-cppgc objects. That was why 8b0e5b19bd0 updated the value in the first place.

We could certainly expose a method that uses this fixed value that embedders can use.

Question is: where? It could potentially be something extra done in SetIsolateUpForNode (that is called by Electron, so I guess it would need to be optionally done based on something in IsolateSettings). Or it could be a new method.

that is called by Electron, so I guess it would need to be optionally done based on something in IsolateSettings

I think that would seem okay to me 👍 IsolateSettings was added specifically so that Electron can customize this a bit more iirc.

One issue with this is that the CppHeap needs to be owned by someone. With the changes I have thus far, it's either owned by CommonEnvironmentSetup::Impl, or NodeMainInstance.

SetIsolateUpForNode doesn't have any place to own the CppHeap.

I guess we could make it part of the MultiIsolatePlatform’s job to attach/detach the cpp heap in RegisterIsolate/UnregisterIsolate? And if we need this behavior to be toggle-able in the default implementation, we could add an extra argument to MultiIsolatePlatform::Create()…

addaleax · 2022-12-12T18:15:17Z

src/node_main_instance.cc

@@ -84,6 +88,17 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
  SetIsolateCreateParamsForNode(isolate_params_.get());
  Isolate::Initialize(isolate_, *isolate_params_);

+  cpp_heap_ = CppHeap::Create(


Here it doesn’t make a difference since for NodeMainInstance, we are in control of how the process is initialized. For the code in CommonEnvironmentSetup this is probably a valid question though – I assume this doesn’t work if Node.js was initialized with kNoInitializeCppgc? Does it crash or just do nothing?

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

joyeecheung · 2022-12-12T23:42:55Z

src/node_main_instance.cc

@@ -84,6 +88,17 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
  SetIsolateCreateParamsForNode(isolate_params_.get());
  Isolate::Initialize(isolate_, *isolate_params_);

+  cpp_heap_ = CppHeap::Create(


Can you add a test for kNoInitializeCppgc, similar to the one in embedtest.cc? I assume doing CppHeap::Create() and Isolate:: AttachCppHeap() right after CommonEnvironmentSetup::Create() would be timely enough? It would be good to know when we add something in CommonEnvironmentSetup::Create() that makes it no longer the case though.

I think for NodeMainInstance() we can probably just do this unconditionally for the add-ons. It does not seem to be necessary for embedders who use NodeMainInstance() and also enable cppgc for add-ons (do they exist?) to customize this part.

joyeecheung · 2022-12-12T23:42:55Z

tools/install.py

@@ -197,15 +197,56 @@ def files(action):
 def headers(action):
  def wanted_v8_headers(files_arg, dest):
    v8_headers = [
+      'deps/v8/include/cppgc/allocation.h',


I think the concern is that we don't want to accidentally expand the API surface. If we have a disallow list we might fail to notice when V8 adds new headers and then breaks the API by removing them. If the list is explicit it helps us notice about changes like this and we can make some fixups or adjust the timing of the V8 upgrade.

Initialize cppgc, create a CppHeap, and attach it to the Isolate. This allows C++ addons to start using cppgc to manage objects. Refs: nodejs#40786

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:

When cppgc is initialized without a page allocator, the platform held by HeapBase is not an instance of 'CppgcPlatformAdapter', so it's invalid to assume that it is. The assumption was made for setting the isolate, so this commit adds a method to the protocol for setting the isolate so it can be fowarded through to the underlying platform correctly.

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

Open

dharesign commented Dec 1, 2022

View changes

src/node.cc Outdated Show resolved Hide resolved

dharesign commented Dec 1, 2022

View changes

dharesign force-pushed the initialize-cppgc branch from 662d1aa to 3860b21 Compare Dec 9, 2022

dharesign marked this pull request as ready for review Dec 10, 2022

dharesign force-pushed the initialize-cppgc branch from 3860b21 to 97c248b Compare Dec 12, 2022

addaleax reviewed Dec 12, 2022

View changes

dharesign closed this Dec 12, 2022

dharesign reopened this Dec 12, 2022

dharesign force-pushed the initialize-cppgc branch from 97c248b to a4f84b2 Compare Dec 12, 2022

joyeecheung reviewed Dec 12, 2022

View changes

dharesign added 2 commits Dec 13, 2022

src,tools: initialize cppgc

6ee3402

Initialize cppgc, create a CppHeap, and attach it to the Isolate. This allows C++ addons to start using cppgc to manage objects. Refs: nodejs#40786

deps: Fix V8 Use-After-Move DCHECK

a00a060

dharesign force-pushed the initialize-cppgc branch from a4f84b2 to a00a060 Compare Dec 13, 2022

Nov	DEC	Jan
	21
2021	2022	2023

src,tools: initialize cppgc #45704

src,tools: initialize cppgc #45704

dharesign commented Dec 1, 2022

nodejs-github-bot commented Dec 1, 2022

dharesign Dec 1, 2022

dharesign Dec 1, 2022

joyeecheung Dec 12, 2022

targos Dec 13, 2022

dharesign Dec 1, 2022

addaleax Dec 12, 2022

dharesign Dec 12, 2022

joyeecheung Dec 12, 2022

dharesign Dec 13, 2022

addaleax Dec 13, 2022

joyeecheung Dec 13, 2022 •

edited

joyeecheung Dec 13, 2022 •

edited

dharesign commented Dec 12, 2022

addaleax Dec 12, 2022

dharesign Dec 12, 2022

dharesign Dec 12, 2022

addaleax Dec 13, 2022

dharesign Dec 13, 2022

addaleax Dec 14, 2022

addaleax Dec 12, 2022

nornagon commented Dec 12, 2022

dharesign commented Dec 12, 2022

dharesign commented Dec 12, 2022

dharesign commented Dec 12, 2022

joyeecheung Dec 12, 2022

joyeecheung Dec 12, 2022

dharesign commented Dec 13, 2022

nodejs-github-bot commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

src,tools: initialize cppgc #45704

Are you sure you want to change the base?

src,tools: initialize cppgc #45704

Conversation

dharesign commented Dec 1, 2022

nodejs-github-bot commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Dec 13, 2022 • edited

Choose a reason for hiding this comment

joyeecheung Dec 13, 2022 • edited

Choose a reason for hiding this comment

dharesign commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nornagon commented Dec 12, 2022

dharesign commented Dec 12, 2022

dharesign commented Dec 12, 2022

dharesign commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharesign commented Dec 13, 2022

nodejs-github-bot commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

dharesign commented Dec 13, 2022

joyeecheung Dec 13, 2022 •

edited

joyeecheung Dec 13, 2022 •

edited