src: attach CppHeap to an v8::Isolate with isolate params #53038
Conversation
nodejs-github-bot
added
c++
Create a `CppHeap` in the `NodeMainInstance` instead and attach the `CppHeap` with `Isolate::CreateParams`. Existing cppgc addon tests should continue to work wihtout change.
| @@ -44,6 +44,7 @@ class TransferData; | |||
| class BaseObject : public MemoryRetainer { | |||
| public: | |||
| enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount }; | |||
| constexpr static uint16_t kDefaultCppGCEmebdderTypeID = 0x90de; | |||
There was a problem hiding this comment.
| constexpr static uint16_t kDefaultCppGCEmebdderTypeID = 0x90de; | |
| constexpr static uint16_t kDefaultCppGCEmbedderTypeID = 0x90de; |
| {}, | ||
| WrapperDescriptor(BaseObject::kEmbedderType, | ||
| BaseObject::kSlot, | ||
| BaseObject::kDefaultCppGCEmebdderTypeID)})), |
There was a problem hiding this comment.
How are embedders expected to get the correct values such as kDefaultCppGCEmebdderTypeID? Shouldn't this be part of NewIsolate() or SetIsolateCreateParamsForNode()?
There was a problem hiding this comment.
Actually, wrapper descriptors are going to be deprecated so that embedders don't have to care about what the value is: nodejs/node-v8#283.
Read more at https://docs.google.com/document/d/1-sBltmlx4yUIzmNHrRO9lKjqiN-uJ81gCmj9-Gq5d5w/edit
There was a problem hiding this comment.
Okay, then this will 'just' attach a CppHeap with default options in the future anyway? That still feels like NewIsolate() or SetIsolateCreateParamsForNode() would be the right places for this, as the goal is to initialize a 'Node.js-flavored' Isolate instance, unless I'm missing something
There was a problem hiding this comment.
Yes, I turned this into a draft because I think the current form is not complete.
|
Closed in favor of nodejs/node-v8#284 |
Create a
CppHeapin theNodeMainInstanceinstead and attach theCppHeapwithIsolate::CreateParams. Existing cppgc addon testsshould continue to work wihtout change.
Fixes: #52718