src: attach CppHeap to an v8::Isolate with isolate params#53038
src: attach CppHeap to an v8::Isolate with isolate params#53038legendecas wants to merge 1 commit intonodejs:mainfrom
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.
| 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 |
3 participants
Create a
CppHeapin theNodeMainInstanceinstead and attach theCppHeapwithIsolate::CreateParams. Existing cppgc addon testsshould continue to work wihtout change.
Fixes: #52718