feat: implement mark_sweep _branded based on approved API redesign#83
feat: implement mark_sweep _branded based on approved API redesign#83shruti2522 wants to merge 7 commits intoboa-dev:mainfrom
Conversation
shruti2522
force-pushed
the
prototype-impl
branch
from
5ed16ac to
a7869b7
Compare
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
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 😄
| /// Converts this root into a `Gc` pointer | ||
| pub fn get<'gc>( | ||
| &self, | ||
| _cx: &crate::collectors::mark_sweep_branded::MutationContext<'id, 'gc>, |
| /// 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); |
There was a problem hiding this comment.
Hmmmm, a couple questions here:
- Have you looked into using a Trace color vs. a
Tracer? I'd prefer that approach vs. keepingTraceraround. - This should still be
&vs.& mut.
There was a problem hiding this comment.
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
| let slot = self | ||
| .pool | ||
| .borrow_mut() | ||
| .try_alloc_bytes(layout) |
There was a problem hiding this comment.
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
| // 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>>, |
There was a problem hiding this comment.
We shouldn't need to maintain a Vec of pool entries here nor the allocation count. That should be managed by the allocator.
| use core::ptr::NonNull; | ||
|
|
||
| /// A weak reference to a GC managed value | ||
| pub struct WeakGc<'id, T: Trace + ?Sized> { |
There was a problem hiding this comment.
Hmmmm, I think we will still need an ephemeron implementation, but we could probably iterate on this
| 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 { |
There was a problem hiding this comment.
RootNode should ideally be allocated onto our heap. It would actually probably be really ideal to have a mempool specifically for RootNodes
| pub(crate) fn iter_from_sentinel( | ||
| sentinel: NonNull<Self>, | ||
| ) -> impl Iterator<Item = NonNull<Self>> { | ||
| struct Iter { |
There was a problem hiding this comment.
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
| // the `Collector` instance | ||
| pub(crate) pool: RefCell<PoolAllocator<'static>>, | ||
| pool_entries: RefCell<Vec<PoolEntry>>, | ||
| pub(crate) sentinel: Pin<Box<RootLink>>, |
There was a problem hiding this comment.
How about a new type RootSentinel that can be a transparent new type.
pub struct RootSentinel(Pin<Box<RootLink>>);| /// Reachability flag set by the mark phase. | ||
| pub(crate) marked: Cell<bool>, | ||
| /// Type-erased trace function. | ||
| pub(crate) trace_fn: Option<TraceFn>, |
There was a problem hiding this comment.
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.
shruti2522
force-pushed
the
prototype-impl
branch
from
d5ebf27 to
2fbadae
Compare
shruti2522
force-pushed
the
prototype-impl
branch
from
57590c5 to
85b9952
Compare
2 participants
Implement
mark_sweep_brandedGC based on the approved API redesign for oscars. This is a draft implementation, also includes changes from #82