Make a user visible and usable Rootable trait, use that to implement root mapping Arena<R> -> Arena<R2> methods and also dynamic root sets

[kyren]

This one API can be used more effectively than the entire gc-sequence crate.

The gc-sequence crate is bad and I should feel bad. It's an absolute nightmare to try to use, in the same way that the futures crate pre-async was a nightmare to try to use.

Why does it exist though? Well, when working with a big garbage collected language environment, we find that we need to dig down into the environment, pull a value out, do some operations on that value, then write it somewhere and continue, and if that operation involves, say, many potential allocations, it is sad if we can't allow continuous collection to happen while this entire sequence occurs. So, if we naively attack this problem, we get something like the gc-sequence crate, the Sequence trait allows us to suspend these long operations and do garbage collection during the suspend points.

But why do we really need to suspend the operation this way, what if we could turn everything upside down and have the caller handle the suspension state and run collect instead of having a Sequencer do it? We would need to be able to write a state somehow and then store it in such a way that it can be garbage collected, call the garbage collector, then resume. Well that's what this API allows us to do!

gc-sequence style...

#[derive(Collect)]
#[collect(no_drop)]
struct MyRoot<'gc> {
    some_complex_state: Vec<Gc<'gc, i32>>,
}

make_sequencable_arena!(sequencable, MyRoot);
use sequencable::Arena as MyArena;

fn example() {
    let arena = MyArena::new(ArenaParameters::default(), |mc| MyRoot {
        some_complex_state: vec![Gc::allocate(mc, 42)],
    });

    let mut sequence = arena.sequence(|root| {
        sequence::from_fn_with(root.some_complex_state[0], |_, test| {
            // A complex operation that does some allocations
            assert_eq!(*test, 42);
            test
        })
        .then(|_, test| {
            // Another operation that does some allocations
            *test
        })
        .boxed()
    });

    loop {
        match sequence.step() {
            Ok((_, output)) => {
                assert_eq!(output, 42);
                return;
            }
            Err(s) => sequence = s,
        }
    }
}

new style...

fn example2() {
    #[derive(Collect, Root)]
    #[collect(no_drop)]
    struct Root<'gc> {
        some_complex_state: Vec<Gc<'gc, i32>>,
    }

    let arena = Arena::<Root>::new(ArenaParameters::default(), |mc| Root {
        some_complex_state: vec![Gc::allocate(mc, 42), Gc::allocate(mc, 69)],
    });

    #[derive(Collect, Root)]
    #[collect(no_drop)]
    struct Intermediate<'gc> {
        root: Root<'gc>,
        state: Gc<'gc, i32>,
    }

    let arena = arena.map_root::<Intermediate>(|_, root| {
        let state = root.some_complex_state[0];
        Intermediate { root, state }
    });

    arena.mutate(|_, root| {
        // A complex operation that does some allocations
        assert_eq!(*root.state, 42);
    });

    let arena = arena.map_root::<Intermediate>(|_, intermediate| {
        let state = intermediate.root.some_complex_state[1];
        Intermediate {
            root: intermediate.root,
            state,
        }
    });

    arena.mutate(|_, root| {
        // Another complex operation that does some allocations
        assert_eq!(*root.state, 69);
    });
}

The idea is that since we can remap the root type to something else, we can put whatever kind of new state we want into it and change its type, so we can reify the state we'd normally store in the stack for some some complex operation, perform it in steps with potential garbage collection in-between, then return the state to its original type. This is also ofc a useful API besides this specific pattern.

I had to make a couple of changes in order to make this work well. First, I moved the root type back to being directly held by an Arena so that mapping between different states is easier and low cost, and this also makes the Arena::mutate_root method much more clearly sound (I believe it was sound before but only because the root Gc pointer is inaccessible in other places and this seemed fragile). I did this without introducing a Wake phase, and I think this change is not too controversial.

I almost had this working with just this and some other minimal changes, but I was stymied at the last minute by rust-lang/rust#49601 and the only solution I found was to change RootProvider to use a GAT. This cascaded into several other changes and though they all work, they seem sort of ugly and how exactly they should be designed is much more up in the air to me. So, this PR is currently a draft.

I truly hate the gc-sequence API and if I were to pick luster back up I would completely drop it and use this API instead basically everywhere, so I think having such an API is very very useful. I'd also like to just deprecate the gc-sequence crate going forward because I'm not sure anybody is really using it, and those who are would probably be better served by this vastly simpler approach.

[kyren]

I was really chuffed with 9fea270 because it basically completely eliminates the need for any complex macro trickery like in #26, but it ended up not mattering due to later having to switch to a GAT. There might be a much cleaner solution to this that doesn't rely on GATs, but I think I lack the deep rust knowledge to pull it off.

[kyren]

I had another idea for an API that allowed any N number of external root types that you could only access from within the arena, kind of like QCell / QCellOwner https://docs.rs/qcell/0.5.2/qcell/

I think that would be a more powerful API but I still think this simpler API is useful, it's kind of frustrating that mapping is not just trivial to provide, it almost was!

[kyren]

I also want to point out that I think, in retrospect, that this approach might have been really obvious to everyone but me, and it just took me this long to come to an obvious conclusion.

My original ORIGINAL plan was to find some way of expressing "coroutines as safe points", so I think from that perspective it's easy to see how I got from that to the compromise of gc-sequence, but I clearly needed to take a step back.

[moulins]

This map_root API seems very useful in general, but I'm not sure this is a best fit for the issue of 'sequencing' operations: having to define a new root type for every intermediate step still seems somewhat tedious.

What about also providing some kind of erased container to manage temporary roots at runtime? For example:

#[derive(Collect, Root)]
struct DynamicRootSet<'gc> {
    roots: slotmap::SlotMap<Gc<'gc, dyn Collect + 'gc>>,
}

struct RootHandle<R: RootProvider> {
    key: slotmap::Key,
    _marker: PhantomData<&'static Root<'static, R>>,
}

impl<'gc> DynamicRootSet<'gc> {
    pub fn new() -> Self;

    pub fn stash<R: RootProvider>(&mut self, root: Gc<'gc, Root<'gc>>) -> RootHandle<R>;

    // Panics if the handle is dangling
    pub fn unstash<R: RootProvider>(&mut self, handle: RootHandle<R>) -> Gc<'gc, Root<'gc>>;
}

Then, you can include a DynamicRootSet inside your main root type, and you don't need to use map_root at all!

#[derive(Collect, Root)]
struct MyRoot<'gc> {
    my_complex_state: Vec<Gc<'gc, i32>>,
    dyn_roots: DynamicRootSet<'gc>,
}

let mut arena = Arena::new(|mc| MyRoot {
    my_complex_state: ...,
    dyn_roots: DynamicRootSet::new(),
});

let (stashed_foo, stashed_bar) = arena.mutate_root(|mc, root| {
    let foo = root.my_complex_state.remove(0);
    let bar = root.my_complex_state.remove(42);
    (root.dyn_roots.stash(foo), root.dyn_roots.stash(bar))
});

arena.collect();

arena.mutate_root(move |mc, root| {
    root.my_complex_state.push(root.dyn_roots.unstash(stashed_foo));
    root.my_complex_state.push(root.dyn_roots.unstash(stashed_bar));
});

---

My original ORIGINAL plan was to find some way of expressing "coroutines as safe points"

Can't the Send trait be abused for this?

If Gc pointers are !Send, and if the Arena API requires Send futures, then the user can't hold a Gc pointer across the await point, and so we know that no unreachable Gcs can exist while garbage collection is running.

[kyren]

Then, you can include a DynamicRootSet inside your main root type, and you don't need to use map_root at all!

I didn't explain it fully because it was late, this is the QCell / QCellOwner idea I was talking about, the owner would not be 'gc but it would only be accessible by a proxy type inside the arena... actually QCell is not a good description of it because it doesn't match perfectly but this is the same idea, I just needed to figure out the simpler thing first. You have to be able to downcast the handle types which means they have to be 'static which... actually I think there's a way to make that work but it's just.. one step at a time, I'm pretty sure such an API requires the actually difficult part of this PR anyway which is the Rootable trait changes. Whenever you need to be able to produce a 'static type that the user holds on to from a gc'ed type.. it needs to implement Rootable.

[kyren]

Can't the Send trait be abused for this?

I thought of this, and I also thought of an even grosser one which is to assert at runtime that the async fn state size is less than the size of a pointer lol, but that is definitely less gross. I don't remember where I left off with this four years ago but I ran into something, originally I tried to do it with async functions that could take parameters for any lifetime but there are compiler limitations that make it impossible, I'd have to go dig up the specific issues. For !Send Gc handles.. you'd need the future to have a reference of a Sync type that allows it to access all of the Gc handles.. it might work and I just didn't try hard enough, I was very burnt out on the idea at the time, feel free to try and make it work.

I think you can look at this problem as divided into two kinds of solutions, one involves taking whatever safe point annotated code you have and pushing it behind a trait that implements Collect, like Sequence, Future, etc... and the other strategy involves making the top-level Arena type be able to be annotated with some state so that the user can exit the mutate callback themselves while storing this state. Anything in the second class of solutions requires a Root / Rootable whatever we call it trait that the user can actually interact with sanely which is MOSTLY what I meant for this PR to be about. The mapping API is just the simplest, very useful thing that this enables.

[moulins]

I didn't explain it fully because it was late, this is the QCell / QCellOwner idea I was talking about [...]

Ah, I thought you were talking about a much more static API (e.g. defining a tuple of root types that can each be mutated separately)...

Hm, you're right that my proposed API needs some kind of runtime downcast check to protect against using a handle in the wrong DynamicRootSet, but I think this is fixable by making sure that all handles are globally unique... I need to think about it a little more.

For !Send Gc handles.. you'd need the future to have a reference of a Sync type that allows it to access all of the Gc handles.. it might work and I just didn't try hard enough, I was very burnt out on the idea at the time, feel free to try and make it work.

This was just an idea in the air, and actually I think there are ways of circumventing the Send check, e.g. by temporarily sending Gc pointers to a scoped thread; fixing that would require even more trickery so it's probably best to abandon the idea before it becomes too complex :c

I definitely think the Rootable trait -- together with some kind of API to store temporary state inside the root set -- is the way to go, so I don't see anything wrong with merging this as-is (modulo the exact name of the trait).

One last remark: should Rootable and Collect even be different traits? Unless there's some limitation I'm not aware of (maybe downcasting stuff?), I don't think there's any reason to have a Collect type that isn't also Rootable.

[kyren]

One last remark: should Rootable and Collect even be different traits? Unless there's some limitation I'm not aware of (maybe downcasting stuff?), I don't think there's any reason to have a Collect type that isn't also Rootable.

I actually agree! If we can make Rootable convenient enough there's not a reason to even have them be separate. I think either they should be the same trait or if we can keep that nifty dyn trait trick with the macro from 9fea270 then they're morally the same trait you just have to wrap one of them in a macro.

[kyren]

Hm, you're right that my proposed API needs some kind of runtime downcast check to protect against using a handle in the wrong DynamicRootSet, but I think this is fixable by making sure that all handles are globally unique... I need to think about it a little more

This is not actually what I meant I just didn't read your API proposal closely enough. You already have the thing I meant where you have a typed handle that is 'static that holds the type of the extra root, my point is just that having such a thing requires an easy to implement RootProvider trait, I just didn't need to explain that to you because it's right there in RootHandle 😝 You're right that you ALSO need to check to make sure everything is from the right arena but there are lots of ways to do that, like a pointer compare or lots of things really.

In any case I think we're synced up now, we just need to design a RootProvider trait that humans can implement and interact with and that doesn't make the compiler upset when you try to use it, and it will allow for lots of different things.

Sorry for poor reading comprehension, I'm replying to all this on my phone.

[kyren]

so I don't see anything wrong with merging this as-is (modulo the exact name of the trait).

I was hoping Aaron would swoop in with his compiler knowledge and tell me how everything I'm trying to do is 10x easier with this One Weird Trick like he did before 😝

[kyren]

Okay.. I don't know what in the world I was thinking last night, maybe hung over from Thanksgiving leftovers or something, let me start over here.

I almost had this working with just this and some other minimal changes, but I was stymied at the last minute by rust-lang/rust#49601 and the only solution I found was to change RootProvider to use a GAT. This cascaded into several other changes and though they all work, they seem sort of ugly and how exactly they should be designed is much more up in the air to me. So, this PR is currently a draft.

This is.. totally wrong. I changed everything back to how it was in 16201e5 and it works fine and I just pushed a test that shows it works fine. I swear when I tried this before I ran into rust-lang/rust#49601 but I tried it today and just... didn't? It's possible yesterday I was using a different compiler version, I use nix to produce development environments and I did recently change where 'nixpkgs' points so it's possible I was just testing with an older compiler, and I definitely get errors with 1.60 that I don't get with 1.64, but honestly I'm not sure, I could have also just been tired and made a mistake trying to use the API. The lesson is don't try to do things when tired from too much turkey I guess.

So forget everything I said about the GAT RootProvider and proc-deriving RootProvider, we're back to a simpler, much less controversial change.

What about also providing some kind of erased container to manage temporary roots at runtime? For example:

Also.. after thinking about it some more, I think it might be nice to provide a version of Arena has a DynamicRootSet like you suggested, but go further and ONLY provide that, with no other default root type. We could make a version of Arena that is built on top of the current one that provides this API I think pretty easily now, with these changes?

The only question that I have left now (assuming rust-lang/rust#49601 is not just waiting somewhere for me to run into again) is the root_provider! macro is used a lot and is too long of a name, should it be named root! and then also name the trait Root? Anybody have any preference here?

[moulins]

So forget everything I said about the GAT RootProvider and proc-deriving RootProvider, we're back to a simpler, much less controversial change.

Oh that's very nice, and it's much cleaner yeah :)

The only question that I have left now [...], should it be named root! and then also name the trait Root? Anybody have any preference here?

What about this?

trait Rootable {
    type Root<'root>: 'root;
}

macro_rules! Rooted { ... }

type Root<'root, R> = <R as Rootable>::Root<'root>;

---

Also.. after thinking about it some more, I think it might be nice to provide a version of Arena has a DynamicRootSet like you suggested, but go further and ONLY provide that, with no other default root type. We could make a version of Arena that is built on top of the current one that provides this API I think pretty easily now, with these changes?

Hmm, but the "real" root still has a special power: it's the only object that can be borrowed mutably without an extra check. (If you want a mutable root inside the DynamicRootSet, you need to put it in a GcCell and do the read()/write() danse) Maybe a good compromise would be to keep Arena generic, but put DynamicRootSet<'static> as the default root type?

[kyren]

Hmm, but the "real" root still has a special power: it's the only object that can be borrowed mutably without an extra check. (If you want a mutable root inside the DynamicRootSet, you need to put it in a GcCell and do the read()/write() danse) Maybe a good compromise would be to keep Arena generic, but put DynamicRootSet<'static> as the default root type?

Yeah but.. since it's always going to be an allocated type it's exactly the same as putting it in a GcCell. But just to be clear, I'm not suggesting getting rid of the Arena type at all, just having an extra DynamicArena that you can use.. actually your suggestion of just having a DynamicRootSet type that can be put into an Arena is way better. I'm actually writing it right now so you'll see, but I think we're synced again.

trait Rootable {
type Root<'root>: 'root;
}

macro_rules! Rooted { ... }

type Root<'root, R> = ::Root<'root>;

Yeah.. or rootable! would work, I'll pick something, macros that start with upper case letters weird me out for some reason? I guess in type position that DOES make more sense though..

[moulins]

Yeah.. or rootable! would work, I'll pick something, macros that start with upper case letters weird me out for some reason? I guess in type position that DOES make more sense though..

The first time I saw this was with syn's Token![*] macro, and I find it really neat ^^ Maybe we should follow and spell the type with brackets (Rooted![Foo] instead of Rooted!(Foo)) to be more distinctive?

[kyren]

OKAY, now I feel like we're getting somewhere! The DynamicRootSet turned out to not be bad at all really, and seems like it will work really well? Hot dang this might actually make me start working on luster again.

[kyren]

As part of implementing DynamicRootSet I ofc ran into the limitation that you can't have a Gc<T: ?Sized> type, and I tried to lift this limitation (probably again) but was reminded of why CoerceUnsized exists and that it is unstable, and I was sad. Instead, I just keep the GcBox pointer directly.

[moulins]

This makes me wonder if Collect::trace shouldn't take &mut self; this would allow to get rid of the RefCell completely, and more generally be useful for data structures that want to exploit the O(n) traversal to do some internal clean-up (e.g. weak maps).

The Collect trait is already unsafe, so giving out a &mut self should be fine as long as the requirements are spelled out correctly.

This should probably be in a separate PR though, it's quite an important change to the (low-level) public API.

[kyren]

I think making the method &mut self might be a bad idea due to recursive objects. It's pretty easy to make an object with a pointer to itself in a garbage collected context (I mean, this is sort of the whole idea after all), and that would mean that you would have both a &mut self and also potentially &self at the same time.

To go even further, I think making a &mut to the target of a Gc pointer is almost always wrong, with the only exception I can think of being how the root object used to be handled, since we could guarantee it was unique. This was part of the reason that the mutate_root method made me uncomfortable before moving away from storing the root object in a pointer again.

This is also why the GcCell needs an actual RefCell and not just a write barrier.

(It's still been a while since I've deeply thought about this, if I'm missing something lemme know, I'm still honestly getting back up to speed with how all this works)

[moulins]

I think making the method &mut self might be a bad idea due to recursive objects. [...]

Hmm, you're right. Forbidding access to other Gc pointers in the trace impl would work (like what already happens in Drop), but there's no reason to impose this requirement on all impls, as you can still use some sort of cell to regain interior mutability if you need to.

[moulins]

As part of implementing DynamicRootSet I ofc ran into the limitation that you can't have a Gc<T: ?Sized> type, and I tried to lift this limitation (probably again) but was reminded of why CoerceUnsized exists and that it is unstable, and I was sad. Instead, I just keep the GcBox pointer directly.

I've beaten my head on this too, and I think they are solutions, but:

• they'll be very low level (as in, manual vtables, pointer tagging, etc);
• extremely tricky (again, manual vtables & pointer tagging);
• and I'm not sure if MIRI will like it.
  So I've kinda left this on the side for now.

Anyways, I'm going to bed, I'll take a second look at everything tomorrow :)

[kyren]

Anyways, I'm going to bed, I'll take a second look at everything tomorrow :)

Thank you for all this work, I know reviewing all of this is probably exhausting ❤️

I think I've addressed everything now, and I fixed the thing that miri caught (it made me panic to see miri errors, but it was very silly and shallow so it's okay).

Even though this PR has grown in scope a lot, I'm really happy with it now and I'm excited to merge it, I feel like with this the crate could actually be almost pleasant to use haha.

[kyren]

Please feel free to veto the Rootable! macro that matches the trait name, just makes there be one less thing to remember?

[moulins]

Please feel free to veto the Rootable! macro that matches the trait name, just makes there be one less thing to remember?

Looks fine to me :)

---

However, I think I found a subtle UB in the DynamicRootSet implementation: the id pointer isn't actually globally unique! If the DynamicRootSet becomes unreachable, its GcCell will get dropped, but dangling DynamicRoots will still point to it, which will go wrong if a new DynamicRootSet later gets allocated in the same place.

I can see three possible fixes:

• Allocate and leak a byte each time a DynamicRootSet is created (this is ugly);
• Go back to the previous id: Rc<()> design, but also store the id as an Rc in the DynamicRoot, so that the unique address stays valid until the last handle is dropped;
• Have a global atomic counter that's incremented on each DynamicRootSet instantiation, and panic if it overflows (probably fine on 64 bits, because 2⁶⁴ is a whole lot of numbers, but maybe problematic on 32 bits?)

[kyren]

However, I think I found a subtle UB in the DynamicRootSet implementation: the id pointer isn't actually globally unique! If the DynamicRootSet becomes unreachable, its GcCell will get dropped, but dangling DynamicRoots will still point to it, which will go wrong if a new DynamicRootSet later gets allocated in the same place.

AH of course, I've used this trick in other places but always via an Rc, I should have just done what I was used to and not tried to be clever, you're absolutely right that this way is not gonna work 😔

I can see three possible fixes:

I took the second strategy with extra spice. I don't know if the way I did it is that valuable or not, but I tried to keep the Copy nature of DynamicRootSet to prevent people from feeling the need to put it in a Gc, and then also minimize the size and clone cost of a DynamicRoot.. I honestly don't really know what's best here. I'm open for better suggestions.

[moulins]

LGTM :)

I don't know either what would be the very best implementation, and there's probably things to do to reduce the number of allocations per DynamicRoot (a separate Rc for each handle seems a little wasteful), but your implementation looks "efficient enough", at least as long as nobody notices anything in performance profiles :D

[kyren]

Yay merged 🎉 Thank you for all the hard work reviewing this, it incredibly helpful ❤️