RFC: api redesign prototype and notes based on gc-arena#68
RFC: api redesign prototype and notes based on gc-arena#68nekevss merged 6 commits intoboa-dev:mainfrom
Conversation
c211cae to
fed14b3
Compare
fed14b3 to
8a0ef34
Compare
8a0ef34 to
cf29ff4
Compare
… feasibilty of context bound pointer lifetimes
eb19a58 to
f661622
Compare
| } | ||
|
|
||
| pub(crate) fn alloc<'gc, T: Trace + Finalize + 'gc>(&'gc self, value: T) -> Gc<'gc, T> { | ||
| let boxed = Box::new(GcBox { |
There was a problem hiding this comment.
I'm highly skeptical of a box usage here. We're just going to end up with the same fragmentation issue we have with the current implementation.
There was a problem hiding this comment.
removed box usage here, used PoolAllocator instead
| | **Pointer** | `Gc<T>` | `Gc<'gc, T>` | | ||
| | **Lifetime** | `'static` + `extend_lifetime()` | `'gc` branded | | ||
| | **Rooting** | Implicit (inc/dec on clone/drop) | Explicit (`Root<T>`) | | ||
| | **Copy cost** | Cell write | Zero | |
There was a problem hiding this comment.
Also the drop cost is a required access to a TLS which uses a futex lock in most implementations. It's very expensive for the current GC. It would be free for the proposed one.
There was a problem hiding this comment.
Thanks, completely missed that. Added TLS futex drop cost to the table
61bed6b to
142d613
Compare
|
Replaced Vec root tracking with intrusive linked list for O(1) root removal, switched from Box allocation to PoolAllocator, and added compile fail tests to check that the I had kept the initial implementation simple since I wanted to focus the discussion on the API design first, but you're right that it should reflect the actual target implementation more closely. Hope this looks better |
| collector_roots: Rc<RefCell<RootList>>, | ||
| node: RootListNode, // Intrusive list node |
There was a problem hiding this comment.
Slight tweak here. You don't need a collector_roots field, you just need to unlink the previous and next elements of RootListNode from the current node, then link them together.
To collect the roots the GC can just take the base root, the go through the linked list to get all the inner Gcs
There was a problem hiding this comment.
Maybe the intrusive_collections crate could be a good inspiration for this
There was a problem hiding this comment.
Used intrusive_collections as the inspiration.RootLink holds only Cell<Option<NonNull<RootLink>>> for prev/next, with gc_ptr recovered via offset_of!
eb0b461 to
8310612
Compare
| // `T: Sized` ensures `gc_ptr` is always a thin (one-word) pointer, so | ||
| // `link` sits at the same byte offset in `Root<T>` for every sized `T`. |
There was a problem hiding this comment.
You could also put the link at the start. right? Such that it doesn't matter if T is sized
There was a problem hiding this comment.
Done, though T: Sized is still required to keep gc_ptr as a thin pointer for sound type erased *mut u8 collector reads. Removing it would need a second NonNull<GcBox<()>> field for the GC to read, with Root::get() using the fat pointer. should I do that?
There was a problem hiding this comment.
Well I guess it kinda doesn't matter, because in our current GC we do some vtable hacks to erase pointers in a slightly safer way, so we also require T:Sized
There was a problem hiding this comment.
Makes sense, I'll leave it as is then
There was a problem hiding this comment.
Actually that's false. What we did in our current GC was to introduce the concept of a GcErasedPtr, which is just a NonNull<GcBox<NonTraceable>>, and because the GcBox has a VTable of every T, you don't need to ensure T: Sized
There was a problem hiding this comment.
We don't need to introduce that here though, since it's just a prototype. We can address these limitations after iterating on this
jedel1043
left a comment
There was a problem hiding this comment.
The new API looks great! Nice job!
We might have some ways to statically ensure Root is not used with a different realm, like relating them to the GcContext itself by some invariant lifetime, but that's a future improvement that we can do while iterating on this.
|
Thanks! Sounds good, I'll keep those improvements in mind for future iterations. |
nekevss
left a comment
There was a problem hiding this comment.
Looks good overall and is super interesting
functional prototype and notes for a new GC API based on the requiremnts fo an ideal Gc mentioned in boa#2631. It explores moving from the current implicit rooting behavior to zero overhead, lifetime branded model inspired by
gc-arena.Gc,Root,MutationContext) with examples