CppGC and CreateParams

[Ronald Fenner]

I was implementing the cppgc into my project and found that the CppHeap::Create returns a unique ptr where as the CreateParams.cpp_heap is just a pointer leading to having to get the pointer from the unique and then releasing when setting the param. Considering it's now the preferred way to set an embedder cppgc heap maybe the create params should be made a unique ptr as well.

[Michael Lippautz]

On Sun, Jun 9, 2024 at 9:25 PM 'Ronald Fenner' via v8-dev <v8-...@googlegroups.com> wrote:

I was implementing the cppgc into my project and found that the CppHeap::Create returns a unique ptr where as the CreateParams.cpp_heap is just a pointer leading to having to get the pointer from the unique and then releasing when setting the param. Considering it's now the preferred way to set an embedder cppgc heap maybe the create params should be made a unique ptr as well.

That doesn't work the way `CppHeap::Create()` is set up as it takes a `const CreateParams&`, so we cannot move away and take ownership. This is not really great API design but it's consistent with `Isolate::CreateParams` which is much harder to change as every embedder uses it at this point.

We may change this at some point but really it is introducing some churn for embedders for code esthetics (the API is not used in many places and scenarios), so we want to strike a balance when we change it.

-Michael

 

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/fa2f1dbf-b07c-4f10-8198-1929bb74824fn%40googlegroups.com.

[Ronald Fenner]

You do make changes to the params from rime to time, and to avoid the churn for embedders, you could depreciate the pointer version and add the unique_ptr one as preferred like you did for array buffer allocator.

Currently the code looks like this for me

std::unique_ptr<v8::CppHeap> heap = v8::CppHeap::Create(

V8Platform::Get().get(),

v8::CppHeapCreateParams({}, v8::WrapperDescriptor((int)V8NativeObjInternalFields::ObjInfo, (int)V8NativeObjInternalFields::ObjInstance, m_CppHeapID)));

params.cpp_heap = heap.get();

// the isolate will own the heap so release it

heap.release();

where as if it wasa nique_ptr could do

params.cpp_heap = v8::CppHeap::Create(

V8Platform::Get().get(),

v8::CppHeapCreateParams({}, v8::WrapperDescriptor((int)V8NativeObjInternalFields::ObjInfo, (int)V8NativeObjInternalFields::ObjInstance, m_CppHeapID)));

leveraging move semantics. The isolate is already assuming ownership over the heap passed in the pointer.

This was more of a observation of the disconnect between the CppHeap::Create returning a unique_ptr where as the Isolate::CreateParams expects just a pointer.

[Michael Lippautz]

On Mon, Jun 10, 2024 at 9:38 PM 'Ronald Fenner' via v8-dev <v8-...@googlegroups.com> wrote:

You do make changes to the params from rime to time, and to avoid the churn for embedders, you could depreciate the pointer version and add the unique_ptr one as preferred like you did for array buffer allocator.

Currently the code looks like this for me

std::unique_ptr<v8::CppHeap> heap = v8::CppHeap::Create(

V8Platform::Get().get(),

v8::CppHeapCreateParams({}, v8::WrapperDescriptor((int)V8NativeObjInternalFields::ObjInfo, (int)V8NativeObjInternalFields::ObjInstance, m_CppHeapID)));

params.cpp_heap = heap.get();

// the isolate will own the heap so release it

heap.release();

where as if it wasa nique_ptr could do

params.cpp_heap = v8::CppHeap::Create(

V8Platform::Get().get(),

v8::CppHeapCreateParams({}, v8::WrapperDescriptor((int)V8NativeObjInternalFields::ObjInfo, (int)V8NativeObjInternalFields::ObjInstance, m_CppHeapID)));

leveraging move semantics. The isolate is already assuming ownership over the heap passed in the pointer.

You cannot move away from a `const CreateParams&` reference. In essence, move-only types (e.g. unique_ptr) are incompatible in such a design.

So, to make this work we'd need to change `Create()` to take by regular ref or value, which is another API change. This would mean that `Isolate` creation and `CppHeap` creation are different, or we'd change both APIs which would introduce quite some churn.

The reason it works for the AB allocator is that it's passed via `shared_ptr` which supports copying.

We looked into this when we added the API and decided to stick to the style for consistency with the drawback that it's not as declarative and nice to write. We are currently reworking how `CppHeap` ownership works though, so maybe we'll start this after getting the rest of the setup in place.

 

This was more of a observation of the disconnect between the CppHeap::Create returning a unique_ptr where as the Isolate::CreateParams expects just a pointer.

On Monday, June 10, 2024 at 6:06:02 AM UTC-5 mlip...@chromium.org wrote:

On Sun, Jun 9, 2024 at 9:25 PM 'Ronald Fenner' via v8-dev <v8-...@googlegroups.com> wrote:

I was implementing the cppgc into my project and found that the CppHeap::Create returns a unique ptr where as the CreateParams.cpp_heap is just a pointer leading to having to get the pointer from the unique and then releasing when setting the param. Considering it's now the preferred way to set an embedder cppgc heap maybe the create params should be made a unique ptr as well.

That doesn't work the way `CppHeap::Create()` is set up as it takes a `const CreateParams&`, so we cannot move away and take ownership. This is not really great API design but it's consistent with `Isolate::CreateParams` which is much harder to change as every embedder uses it at this point.

We may change this at some point but really it is introducing some churn for embedders for code esthetics (the API is not used in many places and scenarios), so we want to strike a balance when we change it.

-Michael

 

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/fa2f1dbf-b07c-4f10-8198-1929bb74824fn%40googlegroups.com.

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/0a1b0753-3115-4ebd-bc7c-80570246ce7en%40googlegroups.com.

[Ronald Fenner]

I see thanks for the explanation.