rework cppgc for prototype chain #1047

littledivy · 2025-01-16T04:30:09Z

Implement prototype inheritance into cppgc object wraps.

struct Foo {}

impl GarbageCollected for Foo {}

#[op2]
impl Foo {
    fn super_method(&self) {
        println!("super method called!");
    }
}

struct Bar {}

impl GarbageCollected for Bar {}

#[op2(inherit = Foo)]
impl Bar {
    #[constructor]
    #[cppgc]
    fn new() -> (Foo, Bar) {
        let foo = Foo {};
        let bar = Bar {};

        (foo, bar)
    }

    #[fast]
    fn use_proto(&self, #[proto] foo: &Foo) {
        // do something with foo
    }
}

const bar = new Bar();

assert(bar instanceof Foo);
bar.superMethod();
bar.useProto();

core/cppgc.rs

bartlomieju · 2025-01-16T14:50:21Z

core/cppgc.rs

@@ -58,6 +132,78 @@ pub fn make_cppgc_object<'a, T: GarbageCollected + 'static>(
  wrap_object(scope, obj, t)
 }

+pub fn make_cppgc_object2<


Maybe change a name to something more descriptive to suggest that it uses the second argument as a prototype

It's named because its easier for the macro to do: format!("make_cppgc_object{}", chain.len()). These aren't supposed to be publicly used. I've added #[doc(hidden)]

lucacasonato

I think cppgc_template_constructor should throw a TypeError.

lucacasonato · 2025-01-16T16:02:00Z

core/cppgc.rs

+/// ErasedPtr is a wrapper around a `v8::cppgc::Ptr` that allows downcasting
+/// to a specific type. Safety is guaranteed by the `tag` field in the
+/// `CppGcObject` struct.
+struct ErasedPtr {


Actually this abstraction is much nicer than what we had previously. With this it is impossible to accidentially use T without checking ptr.tag anymore. Great!

lucacasonato · 2025-01-16T16:03:45Z

core/cppgc.rs

+  fn trace(&self, visitor: &v8::cppgc::Visitor) {
+    // Trace all the objects top-down the prototype chain.
+    for ptr in self.0.iter().flatten() {
+      ptr.ptr.trace(visitor);


Does this dispatch correctly? We don't want to call EreasedPtr's T here - I assume this all works out, but can you add some comments?

lucacasonato

@devsnek PTAL also

core/cppgc.rs

devsnek · 2025-01-16T16:19:58Z

core/cppgc.rs

-        tag: TypeId::of::<T>(),
-        member: t,
-      },
+      PrototypeChainStore([


do normal objects need this? its unfortunate to have double the allocations. could we instead have two tags, one for plain objects and one for prototype backed objects? plain objects can continue to use wrap/unwrap, and prototype backed objects can use wrap2/unwrap2.

@littledivy ^^

That could work with a custom DST. A safe enum will require heap indirection or just have the same size.

I'll give the DSTs a shot, that might also improve the 3 protochain limitation we have since we could just size it at runtime.

size: u8 data: [CppGcObject]

if you can solve it with one system that's cool, but i just meant have two different wrap mechanisms. the default one would be the existing one, and you can opt into a prototype mode if you're using that.

Ok, I made the prototype chain opt-in with #[op2(base)] on the base class otherwise it is treated as a basic object with a single CppGcObject allocation.

oh but that would not work with try_unwrap_cppgc_object. we'll basically have to have 2 functions and the macro would somehow need to know which one to use to "unwrap" self.

#[op2] impl A { fn method(&self) { // we don't know statically if `self` is with proto or without proto. // // the macro produces `try_unwrap_cppgc_object(args.this())` // that uses the `PrototypeChain` trait to figure that out. } } #[op2] fn op_a(#[cppgc] a: &A) { // same here }

It is doable with extra attributes if you feel strongly about it. I think the PrototypeChain trait is not that big of a change and most of it is auto generated by the macro anyways.

self would always be with proto. plain fn would be without proto unless you do like #[cppgc(proto)] or something

self would always be with proto.

don't we want it to without proto for single objects tho?

// want this to be without proto: #[op2] impl A {} // want this to be with proto: #[op2(base)] impl Base {} #[op2(inherit = Base)] impl B {}

So maybe we do this: by default self and #[cppgc] are no-proto. And we do proto only when base and inherit attribute or#[cppgc(proto)] is used.

Downside is that devs will have to keep track of what has proto and what doesn't - which was already done for then with the default PrototypeChain trait.

ultimately I'm just aiming to keep the complexity down for cppgc backed resources. prototype stuff has a lot of its own codegen already so it should be free to use new apis I think.

PTAL 4dc27fb

didn't implement #[cppgc(proto)] cuz we don't need it right now.

devsnek · 2025-01-16T16:24:55Z

core/cppgc.rs

+  }
+}
+
+struct PrototypeChainStore([Option<ErasedPtr>; MAX_PROTO_CHAIN]);


maybe crazy but could this be an interior linked list of ErasedPtr stored on each object? not sure if saving the memory is worth the slight overhead of traversing when unwrapping, perhaps worth testing?

Tried it out:

#[repr(C)] struct CppGcObject<T: GarbageCollected> { tag: TypeId, next: Option<ErasedPtr>, member: T, }

./dcore_proto_linked_list test.mjs # linked list 🛑 deno_core binary is meant for development and testing purposes. Run test.mjs 1e8x new DOMPoint(): 10290ms 1e8x point.x: 2985ms

./dcore_proto_array test.mjs # array 🛑 deno_core binary is meant for development and testing purposes. Run test.mjs 1e8x new DOMPoint(): 10544ms 1e8x point.x: 2785ms

Maybe it makes sense to use linked list when we increase depth further than 3?

littledivy added 8 commits January 15, 2025 09:03

feat: object wrap inheritance

590c1e7

Protochain poc

e37a158

work

c5e869e

lint

6131736

trace

a9b73f5

docs

b5ed169

Flatten

0484aa1

attr

2d67870

littledivy force-pushed the objwrap_inherit branch from f319d33 to 2d67870 Compare January 16, 2025 14:20

littledivy marked this pull request as ready for review January 16, 2025 14:41

bartlomieju reviewed Jan 16, 2025

View reviewed changes

lucacasonato reviewed Jan 16, 2025

View reviewed changes

core/cppgc.rs Outdated Show resolved Hide resolved

devsnek reviewed Jan 16, 2025

View reviewed changes

crowlKats and others added 3 commits January 16, 2025 18:39

Merge branch 'main' into objwrap_inherit

2fa9093

fix merge

6de41c6

Review

356aa7b

petamoriken mentioned this pull request Jan 25, 2025

feat(ext/geometry): Geometry Interfaces Module Level 1 denoland/deno#27527

Open

2 tasks

littledivy added 7 commits March 25, 2025 17:56

x

dd3e1ed

merge

4e2f318

changes

79bfb75

x

1a8c4d9

Lint

099f8bd

Seperate out proto and non-proto methods

4dc27fb

Merge branch 'main' into objwrap_inherit

f8c4e68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework cppgc for prototype chain #1047

rework cppgc for prototype chain #1047

littledivy commented Jan 16, 2025 •

edited

Loading

bartlomieju Jan 16, 2025

littledivy Jan 17, 2025

lucacasonato left a comment

lucacasonato Jan 16, 2025

lucacasonato Jan 16, 2025

littledivy Jan 17, 2025

lucacasonato left a comment

devsnek Jan 16, 2025 •

edited

Loading

devsnek Mar 25, 2025

littledivy Mar 25, 2025

devsnek Mar 26, 2025

littledivy Mar 27, 2025

littledivy Mar 28, 2025

devsnek Mar 28, 2025

littledivy Mar 28, 2025

devsnek Mar 28, 2025

littledivy Mar 28, 2025

devsnek Jan 16, 2025

littledivy Jan 17, 2025 •

edited

Loading

rework cppgc for prototype chain #1047

Are you sure you want to change the base?

rework cppgc for prototype chain #1047

Conversation

littledivy commented Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucacasonato left a 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

lucacasonato left a comment

Choose a reason for hiding this comment

devsnek Jan 16, 2025 • edited Loading

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

Choose a reason for hiding this comment

littledivy Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

littledivy commented Jan 16, 2025 •

edited

Loading

devsnek Jan 16, 2025 •

edited

Loading

littledivy Jan 17, 2025 •

edited

Loading