kyren / gc-arena

Incremental garbage collection from safe Rust
Creative Commons Zero v1.0 Universal
438 stars 36 forks source link

Decouple and expose the `GcRefCell` type publicly #30

Closed redacted-moose closed 1 week ago

redacted-moose commented 1 year ago

This decouples the GcRefCell type from the Gc and GcCell types.

This simplifies things quite a bit: the GcCell and GcWeakCell types can now just be written as type aliases. This also paves the way for modeling types like Gc<'gc, Vec<GcRefCell<'gc, i32>>>, where the vector allocation is immutable, but individual entries in it are mutable (and can be borrowed individually). Users can't currently construct such a type, but it should now be possible to expose the functionality to do so with a little additional effort.

This comes at the cost of an additional pointer for each GcRefCell allocation, which seems reasonable.

This builds on the work done in #29.

moulins commented 1 year ago

Ah, it's funny, I was working on the exact same idea, but with a more complicated scheme that doesn't require an extra indirection for the RefCell ^^

I'll clean up my branch so we can compare notes.

moulins commented 1 year ago

Ok, so here's my code, and here's a quick explanation of the idea.


The core trick is to add a by-reference wrapper type, Mutable<T> (unsafe to construct), that represents the fact that Gc::write_barrier was called with the correct parent Gc pointer. Then, you can provide cell types (I have both a RefCell and a Cell analog) that require &Mutable<Self> instead of &Self for all mutating operations, and be sure that write barriers are properly emitted.

Usage then could look like this:

use gc_cell::{Cell, Mutable};

// A linked-link thingy for demonstration purposes.
type NodeLink<'gc> = Cell<Option<Gc<'gc, Node<'gc>>>>;
#[derive(Collect)]
struct Node<'gc> {
    next: NodeLink<'gc>,
    value: String,
}

// Maybe this could be generated by a macro?
fn project_next<'gc>(this: &Mutable<Node<'gc>>) -> &Mutable<NodeLink<'gc>> {
  // Safe because this doesn't deref through an intermediate GC pointer.
  unsafe { Mutable::assume(&this.next) }
}

// Example usage:
fn remove_next<'gc>(mc: MutationContext<'gc, '_>, node: Gc<'gc, Node<'gc>>) {
    // `gc_cell::Cell` behaves like a `std::cell::Cell` when reading.
    let new_next = node.next.get().and_then(|n| n.next().get());

    // But to write, we need to emit a write barrier...
    let node: &Mutable<Node<'gc>> = Gc::mutate(mc, node);
    // ...and project the inner field.
    let next: &Mutable<NodeLink<'gc>> = project_next(node);
    // Notice that mutating a `gc_cell::Cell` requires a `Mutable`.
    next.set(new_next);
}
redacted-moose commented 1 year ago

Nice! This is the discussion I was hoping to have by opening this PR :smile:

I think I like your approach better actually, since it's actually zero-cost and has the same benefits as my code (and then some, like not requiring cell types to be allocated behind a GC reference). Honestly, there's not much I'd change about your code (maybe a few names, like maybe calling the gc_cell module cell instead to better line up with standard terminology :shrug:). It's a bit more involved, but not really that difficult to understand IMO. I definitely think it's a good idea to provide a macro to generate safe projections, those will get pretty annoying to implement by hand. You should be able to implement the projection functions on the type itself for more convenient syntax since &Mutable<T> implements Deref<Target = T>:

impl<'gc> Node<'gc> {
    pub fn project_next(self: &Mutable<Self>) -> &Mutable<NodeLink<'gc>> {
        // ... 
   }
}

We could extend the Collect derive macro to support a new attribute on fields maybe?

#[derive(Collect)]
struct Node<'gc> {
    // Automatically generates a `.project_next()` method to project `&Mutable<Self>` to `&Mutable<NodeLink<'gc>>`
    #[project]
    next: NodeLink<'gc>,
    value: String,
}

Or maybe it belongs on a separate proc macro? We could also just make it a declarative macro for now.

What work do you have left to do on your branch besides this? I'll close this PR if you're code is almost ready - not sure there's much you'd want to crib from this anyways.

moulins commented 1 year ago

You should be able to implement the projection functions on the type itself for more convenient syntax since &Mutable<T> implements Deref<Target = T>

Unfortunately that's feature-gated (behind arbitrary_self_types IIRC), but this would be wonderful yeah ^^, and would make the method aliases on Mutable<**Cell<T>> unnecessary.


Regarding generating projections with macros, I was thinking along the same lines, with something like this:

#[derive(Collect)]
struct Foo<'gc> {
    #[collect(mutable)]
    a: Cell<Bar<'gc>>,
    b: String,
    #[collect(mutable)]
    c: RefCell<Baz<'gc>>,
}

// GENERATED

struct FooMutFields<'fields, 'gc> {
    a: &'fields Mutable<Cell<Bar<'gc>>>,
    c: &'fields Mutable<RefCell<Baz<'gc>>>,
}

impl<'gc> Foo<'gc> {
    fn project_mutable(this: &Mutable<Foo<'gc>>) -> FooMutFields<'_, 'gc> {
        // does the projection with unsafe
    }
}

Making it a declarative macro used on each projection site would probably be easier to implement, but I'm not sure on how to enforce the "no Deref'ing through a Gc" requirement.


Regarding the state of my branch, it's certainly usable right now, but I'd like to try implementing the proc-macro stuff, and also play with ergonomics a little; I guess I can still open a draft PR in the mean time though.

redacted-moose commented 1 year ago

You should be able to implement the projection functions on the type itself for more convenient syntax since &Mutable implements Deref

Unfortunately that's feature-gated (behind arbitrary_self_types IIRC), but this would be wonderful yeah ^^, and would make the method aliases on Mutable<**Cell> unnecessary.

Grrr, I forgot that that isn't stabilized yet...


... I guess I can still open a draft PR in the mean time though.

Cool, I'll close this once you open yours.

kyren commented 1 week ago

This has long since been superseded by moulins' Lock and RefLock, closing!