feat: implement mark_sweep _branded based on approved API redesign by shruti2522 · Pull Request #83 · boa-dev/oscars

shruti2522 · 2026-04-20T13:34:22Z

Implement mark_sweep_branded GC based on the approved API redesign for oscars. This is a draft implementation, also includes changes from #82

nekevss

Some early feedback. I may try a second pass at this within a couple days.

In general, a lot of this is super interesting. Super excited to see it progress 😄

nekevss · 2026-04-24T01:52:29Z

+/// Use `Tracer::mark` for every reachable `Gc` pointer.
+pub trait Trace {
+    /// Marks all `Gc` pointers reachable from `self`.
+    fn trace(&mut self, tracer: &mut Tracer);


Hmmmm, a couple questions here:

Have you looked into using a Trace color vs. a Tracer? I'd prefer that approach vs. keeping Tracer around.

This should still be & vs. & mut.

Yes, the TraceColor refactor slipped my mind 😅 on it now. Also switching &mut self to &self since mark bits already use interior mutability via Cell<bool> so it's overkill

nekevss · 2026-04-24T02:09:38Z

+        let slot = self
+            .pool
+            .borrow_mut()
+            .try_alloc_bytes(layout)


I think this is incorrect. It should be try_alloc not try_alloc_bytes

Is there a reason for allocating the the BumpPage here rather than the slots?

If we are using one of the bump pages here, then we should probably use one of the arena allocators

nekevss · 2026-04-24T02:13:01Z

+    // and we ensure that `Gc` objects and pool allocations do not outlive
+    // the `Collector` instance
+    pub(crate) pool: RefCell<PoolAllocator<'static>>,
+    pool_entries: RefCell<Vec<PoolEntry>>,


We shouldn't need to maintain a Vec of pool entries here nor the allocation count. That should be managed by the allocator.

nekevss · 2026-04-24T02:18:34Z

+use core::ptr::NonNull;
+
+/// A weak reference to a GC managed value
+pub struct WeakGc<'id, T: Trace + ?Sized> {


Hmmmm, I think we will still need an ephemeron implementation, but we could probably iterate on this

nekevss · 2026-04-24T02:21:26Z

+    pub fn root<T: Trace + Finalize + 'gc>(&self, gc: Gc<'gc, T>) -> Root<'id, T> {
+        let gc_ptr = gc.ptr;
+
+        let node = Box::new(RootNode {


RootNode should ideally be allocated onto our heap. It would actually probably be really ideal to have a mempool specifically for RootNodes

nekevss · 2026-04-24T02:29:22Z

+    pub(crate) fn iter_from_sentinel(
+        sentinel: NonNull<Self>,
+    ) -> impl Iterator<Item = NonNull<Self>> {
+        struct Iter {


thought: this should probably just be a separate iterator RootLinkIter, and then we call iter that returns RootLinkIter rather than iter_from_sentinel call which is an anonymous iter

nekevss · 2026-04-24T02:31:28Z

+    // the `Collector` instance
+    pub(crate) pool: RefCell<PoolAllocator<'static>>,
+    pool_entries: RefCell<Vec<PoolEntry>>,
+    pub(crate) sentinel: Pin<Box<RootLink>>,


How about a new type RootSentinel that can be a transparent new type.

pub struct RootSentinel(Pin<Box<RootLink>>);

nekevss · 2026-04-24T02:38:19Z

+    /// Reachability flag set by the mark phase.
+    pub(crate) marked: Cell<bool>,
+    /// Type-erased trace function.
+    pub(crate) trace_fn: Option<TraceFn>,


Is there ever an instance where trace_fn would be None. This should be fine to just be TraceFn as it's never not set.

nekevss

Have another small batch of comments / feedback

nekevss · 2026-04-28T05:51:02Z

+        let size = mem::size_of::<PoolItem<gc::RootNode<'id, T>>>();
+        let mut pool = self.root_pool.borrow_mut();
+        let slot_ptr = pool
+            .alloc_slot_raw(size)


Why are we creating new methods on pool rather than using the pre-existing try_alloc?

nekevss · 2026-04-28T05:55:51Z

+
+        let mut pool = self.pool.borrow_mut();
+        let slot_ptr = pool
+            .alloc_slot_raw(size)


same question here: why would we be using a new alloc_slot_raw vs. the pre-existing try_alloc

nekevss · 2026-04-28T05:57:05Z

+            // Initialize the PoolItem<GcBox<T>> in place
+            core::ptr::write(
+                pool_item_ptr.as_ptr(),
+                PoolItem(GcBox {


This should be ideally be defined as a ctor on GcBox rather than created here.

nekevss · 2026-04-28T05:57:36Z

+
+        self.generic_alloc_id.set(alloc_id.wrapping_add(1));
+
+        unsafe fn trace_value<T: trace::Trace>(


This definition can be moved to the gc_box module

nekevss · 2026-04-28T05:59:32Z

+///
+/// The `'a` lifetime ties the color to the collection cycle,
+/// preventing it from being stored or escaping the collector.
+pub struct TraceColor<'a> {


This change just kept the tracer implementation and named it to TraceColor. Is there a reason for the name change vs. switching to the full tricolor approach?

Is the worklist somehow necessary for this current implementation?

nekevss

Hopefully last round of general review. 😅 There's a several things that I think I'd prefer to address on this first pass at this API

nekevss · 2026-04-30T00:42:57Z

+
+/// A weak reference to a GC managed value
+pub struct WeakGc<'id, T: Trace + ?Sized> {
+    pub(crate) ptr: NonNull<PoolItem<GcBox<T>>>,


use PoolPointer

nekevss · 2026-04-30T01:15:58Z

+use core::ptr::NonNull;
+
+pub struct Ephemeron<'id, K: Trace, V: Trace> {
+    pub(crate) key_ptr: NonNull<PoolItem<GcBox<K>>>,


issue: key_ptr here should be an Option<pointer>

Ephemeron keys are removed and set to None when they underlying ptr is invalid (i.e. null).

nekevss · 2026-04-30T01:19:25Z

+/// A transient pointer to a GC-managed value.
+#[derive(Debug)]
+pub struct Gc<'gc, T: Trace + ?Sized + 'gc> {
+    pub(crate) ptr: NonNull<PoolItem<GcBox<T>>>,


Here and elsewhere: is there a reason for using NonNull<PoolItem<GcBox<T>>>? If there's not a particular reason, then we should be using PoolPointer

nekevss · 2026-04-30T01:20:41Z

+    // the `Collector` instance
+    pub(crate) pool: RefCell<PoolAllocator<'static>>,
+    /// Dedicated pool for RootNode allocations
+    pub(crate) root_pool: RefCell<PoolAllocator<'static>>,


thought: hmmmm, intriguing. I'm not entirely convinced immediately that this is needed, but we can leave it as is currently.

nekevss · 2026-04-30T02:48:16Z

+        for link_ptr in self.sentinel.iter() {
+            unsafe {
+                // Read the `gc_ptr` field using the stable offset
+                let gc_ptr_ptr = link_ptr


Instead of doing pointer offsets here, we should be able to cast to an ErasedRootNode type and use that specific type to begin the marking correct? My initial reaction would is that may be a better approach as it allows us to be more explicit and not deal as much in NonNull<u8> land.

Is there any benefit to manually calculating the pointer and dealing with NonNull<u8>s over explicit types

nekevss · 2026-04-30T02:54:00Z

+                drop_and_free::<T>,
+                alloc_id,
+            ))
+            .expect("branded GC: pool allocation failed");


We should be throwing this error and not panicking

nekevss · 2026-04-30T03:07:18Z

+            for entry in self.ephemerons.borrow().iter() {
+                use crate::alloc::mempool3::PoolItem;
+                unsafe {
+                    let key_box = entry.key_ptr.cast::<PoolItem<GcBox<()>>>();


thought: If we are always casting to this value, then we should just probably just store the pointer as that value.

nekevss · 2026-04-30T03:08:34Z

+                unsafe {
+                    let key_box = entry.key_ptr.cast::<PoolItem<GcBox<()>>>();
+                    // Skip entries invalidated by a previous collection cycle.
+                    if (*key_box.as_ptr()).0.alloc_id != entry.key_alloc_id {


This should be probably handled by the ephemeron key being an option

feat: implement mark_sweep_branded based on approved API redesign

a7869b7

shruti2522 force-pushed the prototype-impl branch from 5ed16ac to a7869b7 Compare April 23, 2026 17:56

shruti2522 marked this pull request as ready for review April 23, 2026 17:57

shruti2522 added 2 commits April 23, 2026 17:58

Merge remote-tracking branch 'upstream/main' into prototype-impl

a62f19e

fix

222a786

nekevss requested changes Apr 24, 2026

View reviewed changes

shruti2522 and others added 3 commits April 24, 2026 18:24

add ephemeron support

974c72b

replace Tracer with TraceColor

4928a18

remove pool_entries Vec and allocation_count

2fbadae

shruti2522 force-pushed the prototype-impl branch 2 times, most recently from 57590c5 to 85b9952 Compare April 24, 2026 21:20

nekevss reviewed Apr 28, 2026

View reviewed changes

shruti2522 force-pushed the prototype-impl branch 3 times, most recently from e270d95 to 08de4ff Compare April 29, 2026 14:24

implement tri-color marking

f5d3154

shruti2522 force-pushed the prototype-impl branch from 08de4ff to f5d3154 Compare April 29, 2026 14:25

nekevss reviewed Apr 30, 2026

View reviewed changes


		self.generic_alloc_id.set(alloc_id.wrapping_add(1));

		unsafe fn trace_value<T: trace::Trace>(

Conversation

shruti2522 commented Apr 20, 2026

Uh oh!

nekevss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nekevss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nekevss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants