feat: support weak map by shruti2522 · Pull Request #13 · boa-dev/oscars

shruti2522 · 2026-02-21T09:03:45Z

weak map implementation

added 8 new tests to test the changes
I have craeted a doc summarizing all the changes

verified all tests pass with miri

nekevss

Hi! Thanks for taking a crack at this. You beat me to it 😅 A lot of this is looking pretty good.

I left a couple of early comments for a first round of review, and I'll probably have a few more to follow up hopefully within 24-48 hours.

My biggest comment would be, please make sure to safety comment unsafe usage. I think I also saw a separate column in the collector for to mark and sweep weak maps specifically, and I'd be curious if we were able to still safely do that from just the ephemoron queue. But I haven't been able to dig into that completely just yet, so I'm probably going to focus more on that part for the second part of the review.

nekevss · 2026-02-23T04:39:44Z

oscars/src/alloc/arena2/alloc.rs

+    ///
+    /// - Caller must ensure that no other references to the value exist
+    /// - Caller must ensure that `T` is the correct type for the pointer
+    pub unsafe fn as_inner_mut(&mut self) -> &'arena mut T {


question: what is the purpose of this API?

I'm not seeing it used anywhere, and this is currently fairly stripped down and minimally functional as is. If there is no apparent use for it, then I'd prefer it not be added

nekevss · 2026-02-23T04:45:02Z

oscars/src/collectors/mark_sweep/internals/ephemeron.rs

-    pub fn new_in(key: K, value: V, collector: &mut MarkSweepGarbageCollector) -> Self
-    where
-        K: Sized,
+    pub fn new_in(key: WeakGcBox<K>, value: V, collector: &mut MarkSweepGarbageCollector) -> Self


issue: this should take a Gc<T>

I think this is probably the correct approach and the previous approach was probably trying to be too fancy, but the work here is being completed in the wrong place. Ephemeron should manage the creation of the WeakGcBox not some other API.

changed the signature from WeakGcBox<K> to &Gc<K>. now ephemeron internally creates the WeakGcBox from Gc pointer

nekevss · 2026-02-23T04:45:29Z

oscars/src/collectors/mark_sweep/internals/ephemeron.rs

+            this: ErasedEphemeron,
+            color: TraceColor,
+        ) -> bool {
+            let ephemeron = unsafe {


nit: Safety comment

nekevss · 2026-02-23T04:45:40Z

oscars/src/collectors/mark_sweep/internals/ephemeron.rs

+
+        // SAFETY: Cast back to concrete types to run finalizers
+        unsafe fn finalize_fn<K: Trace + 'static, V: Trace + 'static>(this: ErasedEphemeron) {
+            let ephemeron = unsafe {


nit: safety comment

nekevss · 2026-02-23T04:47:29Z

oscars/src/collectors/mark_sweep/internals/gc_box.rs

+    }
+
+    pub(crate) fn inner_ref(&self) -> &GcBox<NonTraceable> {
+        unsafe { self.as_sized_inner_ptr().as_ref() }


nit: safety comment

nekevss · 2026-02-23T04:47:38Z

oscars/src/collectors/mark_sweep/internals/gc_box.rs

-    pub fn value(&self) -> &T {
-        self.0.value()
+    pub(crate) fn as_sized_inner_ptr(&self) -> NonNull<GcBox<NonTraceable>> {
+        let heap_item = unsafe { self.as_heap_ptr().as_mut() };


nit: safety comment

nekevss · 2026-02-23T04:50:25Z

oscars/src/collectors/mark_sweep/internals/gc_box.rs


-    pub fn value(&self) -> &T {
-        self.0.value()
+    pub(crate) fn as_sized_inner_ptr(&self) -> NonNull<GcBox<NonTraceable>> {


comment: I'm not sure I'm in love with the name

I'm also not convinced there's a better name / wouldn't be surprised if this was based off naming elsewhere in this repository ... it may be worth being a bit better on naming in the future. For right now though, this is totally fine.

renamed it to erased_inner_ptr 😅

oscars/src/collectors/mark_sweep/internals/gc_box.rs

shruti2522 · 2026-02-23T13:36:58Z

Thanks for the review @nekevss 😄 I will address these comments and follow up with the changes

shruti2522 · 2026-02-25T16:07:39Z

this is ready for a second round of review @nekevss, whenever you have time 🙌🏻

nekevss

Alrighty, sorry for the delayed review. It's been a busy week 😅

This overall is seriously looking pretty good. Thanks for putting in the effort on this. I think I have one major issue that I'd appreciate if you could refactor on this, and that's primarily the weak map registry. I think it's the right idea to approach this if you want the weak map to be totally separate, but I'm ultimately not sure that that's the case.

Instead, I think it's going to be best if we look to allocate WeakMap onto the collector's heap, and manage it from there. While the HashMap's memory is separate from the collector heap, the weak map itself is tied to the life of the collector.

Again, great work on this so far! I'm really looking forward to being able to merge it.

nekevss · 2026-02-26T04:34:09Z

oscars/src/collectors/mark_sweep/internals/gc_box.rs

-    pub fn new_in(value: T, collection_state: &CollectionState) -> Self {
-        Self(GcBox::new_typed_in::<true>(value, collection_state))
+impl<T: Trace + Finalize + ?Sized> WeakGcBox<T> {
+    pub fn new_in(inner_ptr: ErasedArenaPointer<'static>) -> Self {


issue: remove _in suffix

The new_in is something I'm borrowing from allocator API methods with the hope of making the secondary parameter generic over a Collector trait of some sort. Whether we can do that, TBD. But since we are no longer allocating or needing to notify the collector here, we don't need the _in suffix.

nekevss · 2026-02-26T04:36:08Z

oscars/src/collectors/mark_sweep/internals/gc_box.rs

-    pub(crate) fn new_in(value: T, collection_state: &CollectionState) -> Self {
-        Self::new_typed_in::<false>(value, collection_state)
+    #[inline]
+    pub fn new_in(value: T, collection_state: &CollectionState) -> Self {


Huh, at some point, I had also stopped following my own scheme here. I think remove the _in suffix here as well the be consistent.

We can come back to this later on

nekevss · 2026-02-26T04:36:28Z

oscars/src/collectors/mark_sweep/internals/gc_box.rs

    // TODO (nekevss): What is the best function signature here?
-    pub(crate) fn new_typed_in<const IS_WEAK: bool>(
+    #[inline]
+    pub(crate) fn new_typed_in(


re: _in suffix

nekevss · 2026-02-26T04:37:36Z

oscars/src/collectors/mark_sweep/pointers/WEAK_MAP.md

@@ -0,0 +1,66 @@
+# WeakMap Support


nit: move this doc into notes, and maybe date it if possible 😄

nekevss · 2026-02-26T04:40:37Z

oscars/src/collectors/mark_sweep/pointers/weak_map.rs

@@ -0,0 +1,118 @@
+use hashbrown::HashMap;


note: it may be worthwhile to look at the HashTable impl in future iterations

nekevss · 2026-02-26T04:46:13Z

oscars/src/collectors/mark_sweep/pointers/weak_map.rs

+    #[cfg(not(debug_assertions))]
+    pub(crate) fn mark_registered(&mut self) {}
+
+    pub fn insert(


hmmmm, I'm not sure how I feel about this method taking a (key/value pair vs. an ephemeron). Again, I don't think I'd block this PR specifically on this. I'm mostly just trying to make note of: how can we do this better?

nekevss · 2026-02-26T05:00:43Z

oscars/src/collectors/mark_sweep/tests.rs

+        .with_heap_threshold(512);
+
+    let mut map = WeakMap::new();
+    unsafe { collector.register_weak_map(core::ptr::addr_of_mut!(map)) };


this is probably the most concerning point of this design to me. I'm not entirely sure that keeping these bounds is realistically possible ...

nekevss · 2026-02-26T05:11:36Z

oscars/src/collectors/mark_sweep/pointers/weak_map.rs

+use super::Gc;
+
+// must be registered via `register_weak_map` or reads after GC will panic
+pub struct WeakMap<K: Trace + 'static, V: Trace + 'static> {


I think, ultimately, if we want to remove the guard (i.e. register weak map), then we may need to allocate this hashmap onto the collectors heap.

I think that's the best path forward that I can think of where I feel alright about it ... if we are going to actively prune the WeakMap with each collection, instead of lazily check during get (which mind you, I'm not even sure is possible).

shruti2522 · 2026-02-26T22:32:13Z

Thanks for the review, I've updated the PR to resolve all the concerns you mentioned, I also agree with your notes about using a HashTable and thinking about the key/value pair vs. an ephemeron approach for insert, I'll definitely look into those for a future iteration, for now I have added TODO notes for these

Let me know if anything else is needed 😄

shruti2522 force-pushed the feat/weak_map branch 6 times, most recently from 7cbe85e to 3655e0b Compare February 22, 2026 16:59

shruti2522 marked this pull request as ready for review February 22, 2026 17:08

nekevss self-requested a review February 23, 2026 04:36

nekevss added the enhancement New feature or request label Feb 23, 2026

nekevss requested changes Feb 23, 2026

View reviewed changes

shruti2522 force-pushed the feat/weak_map branch from 3655e0b to 6a774fb Compare February 23, 2026 16:15

nekevss requested changes Feb 26, 2026

View reviewed changes

shruti2522 force-pushed the feat/weak_map branch from 4e6740e to bd8a16f Compare February 26, 2026 22:08

feat: support weak map

0de0f8e

shruti2522 force-pushed the feat/weak_map branch from bd8a16f to 0de0f8e Compare February 26, 2026 22:12

Conversation

shruti2522 commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shruti2522 commented Feb 23, 2026

Uh oh!

shruti2522 commented Feb 25, 2026

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!

nekevss Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shruti2522 commented Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shruti2522 commented Feb 21, 2026 •

edited

Loading

nekevss Feb 26, 2026 •

edited

Loading