fix(runtime): keep Set/Map forEach stable across compaction

changelog.d/9082-map-set-foreach-compaction.md

@@ -0,0 +1,2 @@
+`Map.prototype.forEach` and `Set.prototype.forEach` now continue visiting live
+entries when callback-side deletes cross the backing-store compaction threshold.

crates/perry-runtime/src/exception.rs

@@ -114,6 +114,10 @@ struct ExceptionState {
     /// `call_method_depth_*`). Indexed by try-depth, in lockstep with
     /// `jump_buffers`.
     call_method_depths: Box<[u32]>,
+    /// Active Set/Map `forEach` walks. Their normal epilogues re-enable
+    /// backing-store compaction, but a caught throw skips those epilogues.
+    set_foreach_depths: Box<[usize]>,
+    map_foreach_depths: Box<[usize]>,
     /// Recorded-prototype lookup stack depth. A getter can throw while
     /// `resolve_inherited_field` is recursively walking; longjmp skips its
     /// guard drops, so restore the stack to this try-entry savepoint.
@@ -157,6 +161,8 @@ impl ExceptionState {
             shadow_savepoints: vec![ShadowSavepoint::EMPTY; MAX_TRY_DEPTH].into_boxed_slice(),
             runtime_handle_savepoints: vec![0usize; MAX_TRY_DEPTH].into_boxed_slice(),
             call_method_depths: vec![0u32; MAX_TRY_DEPTH].into_boxed_slice(),
+            set_foreach_depths: vec![0usize; MAX_TRY_DEPTH].into_boxed_slice(),
+            map_foreach_depths: vec![0usize; MAX_TRY_DEPTH].into_boxed_slice(),
             prototype_resolution_depths: vec![0usize; MAX_TRY_DEPTH].into_boxed_slice(),
             static_private_owner_depths: vec![0usize; MAX_TRY_DEPTH].into_boxed_slice(),
             private_lexical_brand_depths: vec![0usize; MAX_TRY_DEPTH].into_boxed_slice(),
@@ -219,6 +225,8 @@ fn try_push_with_kind(kind: HandlerKind) -> *mut i32 {
         // this `try` can restore it — `longjmp` skips the `CallMethodDepthGuard`
         // `Drop`s of the method frames it unwinds (#5591).
         (*s).call_method_depths[depth] = crate::object::call_method_depth_savepoint();
+        (*s).set_foreach_depths[depth] = crate::set::set_foreach_stack_savepoint();
+        (*s).map_foreach_depths[depth] = crate::map::map_foreach_stack_savepoint();
         (*s).prototype_resolution_depths[depth] =
             crate::object::prototype_chain::resolution_stack_savepoint();
         (*s).static_private_owner_depths[depth] =
@@ -346,6 +354,8 @@ pub extern "C-unwind" fn js_throw(value: f64) -> ! {
         // otherwise caught throws wrap the counter below zero and wedge every
         // later method call into the depth-guard fallback (#5591).
         crate::object::call_method_depth_restore((*s).call_method_depths[depth]);
+        crate::set::set_foreach_stack_restore((*s).set_foreach_depths[depth]);
+        crate::map::map_foreach_stack_restore((*s).map_foreach_depths[depth]);
         crate::object::prototype_chain::resolution_stack_restore(
             (*s).prototype_resolution_depths[depth],
         );
@@ -681,6 +691,8 @@ pub(crate) fn test_unwind_innermost_shadow_restore() {
         let depth = (*s).try_depth - 1;
         shadow_stack_restore((*s).shadow_savepoints[depth]);
         runtime_handle_stack_restore((*s).runtime_handle_savepoints[depth]);
+        crate::set::set_foreach_stack_restore((*s).set_foreach_depths[depth]);
+        crate::map::map_foreach_stack_restore((*s).map_foreach_depths[depth]);
         crate::object::prototype_chain::resolution_stack_restore(
             (*s).prototype_resolution_depths[depth],
         );

crates/perry-runtime/src/map.rs

@@ -16,6 +16,39 @@ const TAG_UNDEFINED: u64 = 0x7FFC_0000_0000_0001;
 
 crate::perry_thread_local! {
     static MAP_ITERATOR_ARRAYS: RefCell<PtrHashSet<usize>> = RefCell::new(new_ptr_hash_set());
+    /// Backing Maps whose `forEach` raw-entry walk is currently active. A
+    /// compaction would move entries behind those cursors (#9082).
+    static MAP_FOREACH_STACK: RefCell<Vec<usize>> = const { RefCell::new(Vec::new()) };
+}
+
+#[inline]
+fn map_foreach_is_active(map: *const MapHeader) -> bool {
+    let addr = map as usize;
+    MAP_FOREACH_STACK.with(|stack| stack.borrow().contains(&addr))
+}
+
+fn map_foreach_enter(map: *const MapHeader) {
+    MAP_FOREACH_STACK.with(|stack| stack.borrow_mut().push(map as usize));
+}
+
+/// Pop one completed walk. Returns true when this was the outermost walk of
+/// this Map, at which point deferred compaction is safe again.
+fn map_foreach_leave(map: *const MapHeader) -> bool {
+    let addr = map as usize;
+    MAP_FOREACH_STACK.with(|stack| {
+        let mut stack = stack.borrow_mut();
+        let popped = stack.pop();
+        debug_assert_eq!(popped, Some(addr));
+        !stack.contains(&addr)
+    })
+}
+
+pub(crate) fn map_foreach_stack_savepoint() -> usize {
+    MAP_FOREACH_STACK.with(|stack| stack.borrow().len())
+}
+
+pub(crate) fn map_foreach_stack_restore(depth: usize) {
+    MAP_FOREACH_STACK.with(|stack| stack.borrow_mut().truncate(depth));
 }
 
 fn mark_map_iterator_array(arr: *mut crate::array::ArrayHeader) {
@@ -789,6 +822,13 @@ pub(crate) fn map_header_moved_for_gc(old_addr: usize, new_addr: usize) {
             idx.insert(new_addr, slot);
         }
     });
+    MAP_FOREACH_STACK.with(|stack| {
+        for addr in stack.borrow_mut().iter_mut() {
+            if *addr == old_addr {
+                *addr = new_addr;
+            }
+        }
+    });
 }
 
 pub(crate) unsafe fn finalize_map_side_allocation_for_gc(map: *mut MapHeader) {
@@ -1745,7 +1785,7 @@ unsafe fn ensure_capacity(map: *mut MapHeader) -> bool {
     // Full by EXTENT. Squeeze tombstones out first — reclaiming holes is
     // cheaper than doubling, and it keeps a delete-heavy map from growing on
     // dead weight.
-    if (*map).size < (*map).used {
+    if (*map).size < (*map).used && !map_foreach_is_active(map) {
         compact_map_entries(map);
         if (*map).used < (*map).capacity {
             return false;
@@ -2425,7 +2465,7 @@ unsafe fn delete_entry_at_index(map: *mut MapHeader, idx: i32) -> i32 {
     forget_map_index_entry(map, deleted_key, idx as u32);
 
     let used = (*map).used;
-    if used >= 16 && (*map).size < used / 2 {
+    if used >= 16 && (*map).size < used / 2 && !map_foreach_is_active(map) {
         compact_map_entries(map);
     }
     1
@@ -2524,6 +2564,9 @@ pub extern "C" fn js_map_clear(map: *mut MapHeader) {
     let size = unsafe { (*map).size };
     let used = unsafe { (*map).used };
     if size == 0 {
+        if !map_foreach_is_active(map) {
+            unsafe { (*map).used = 0 };
+        }
         return;
     }
     // The string and pointer side-tables hold an entry only for a string or
@@ -2542,7 +2585,29 @@ pub extern "C" fn js_map_clear(map: *mut MapHeader) {
         };
     unsafe {
         (*map).size = 0;
-        (*map).used = 0;
+        if map_foreach_is_active(map) {
+            // Preserve the raw [[MapData]] extent while a forEach cursor names
+            // it. Clearing turns each live pair into a tombstone; entries
+            // appended by the callback remain after the old extent and are
+            // visited by the continuing walk.
+            let entries = entries_ptr_mut(map);
+            for i in 0..used as usize {
+                if ptr::read(entries.add(i * 2)).to_bits() != MAP_HOLE_KEY_BITS {
+                    crate::gc::runtime_store_external_jsvalue_slot(
+                        map as usize,
+                        entries.add(i * 2) as usize,
+                        MAP_HOLE_KEY_BITS,
+                    );
+                    crate::gc::runtime_store_external_jsvalue_slot(
+                        map as usize,
+                        entries.add(i * 2 + 1) as usize,
+                        crate::value::TAG_UNDEFINED,
+                    );
+                }
+            }
+        } else {
+            (*map).used = 0;
+        }
     }
     unsafe {
         if let Some(index) = (*map).numeric_index.as_mut() {
@@ -3039,7 +3104,9 @@ fn js_map_foreach_impl(
     if map.is_null() {
         return;
     }
-    unsafe { compact_if_holey(map as *mut MapHeader) };
+    if !map_foreach_is_active(map) {
+        unsafe { compact_if_holey(map as *mut MapHeader) };
+    }
     let scope = crate::gc::RuntimeHandleScope::new();
     let map_handle = scope.root_raw_const_ptr(map);
     let callback_handle = scope.root_nanbox_f64(callback);
@@ -3048,6 +3115,7 @@ fn js_map_foreach_impl(
     // survives a GC triggered inside the callback.
     let has_override = collection_override.to_bits() != crate::value::TAG_UNDEFINED;
     let collection_handle = scope.root_nanbox_f64(collection_override);
+    map_foreach_enter(map);
     unsafe {
         // The collection itself is the third callback argument and the
         // identity user code compares `self === m` against.
@@ -3089,6 +3157,10 @@ fn js_map_foreach_impl(
             crate::object::js_implicit_this_set(prev_this);
         }
     }
+    let map = map_handle.get_raw_const_ptr::<MapHeader>();
+    if map_foreach_leave(map) {
+        unsafe { compact_if_holey(map as *mut MapHeader) };
+    }
 }
 
 #[cfg(test)]

crates/perry-runtime/src/map_tombstone_tests.rs

@@ -26,30 +26,84 @@ extern "C" fn delete_current_map_entry(
     f64::from_bits(crate::value::TAG_UNDEFINED)
 }
 
+extern "C" fn delete_earlier_map_entry(
+    _closure: *const crate::closure::ClosureHeader,
+    value: f64,
+    key: f64,
+    collection: f64,
+) -> f64 {
+    FOREACH_DELETE_VISITS.with(|visits| visits.borrow_mut().push((key.to_bits(), value.to_bits())));
+    if key > 0.0 {
+        let map = crate::value::js_nanbox_get_pointer(collection) as *mut MapHeader;
+        js_map_delete(map, key - 1.0);
+    }
+    f64::from_bits(crate::value::TAG_UNDEFINED)
+}
+
+fn take_foreach_delete_visits() -> Vec<(f64, f64)> {
+    FOREACH_DELETE_VISITS.with(|visits| {
+        visits
+            .borrow_mut()
+            .drain(..)
+            .map(|(key, value)| (f64::from_bits(key), f64::from_bits(value)))
+            .collect()
+    })
+}
+
+fn foreach_callback(func: *const u8) -> f64 {
+    FOREACH_DELETE_VISITS.with(|visits| visits.borrow_mut().clear());
+    let callback = crate::closure::js_closure_alloc(func, 0);
+    crate::value::js_nanbox_pointer(callback as i64)
+}
+
 #[test]
-fn foreach_skips_tombstones_created_by_callback_deletes() {
+fn foreach_survives_delete_compaction_threshold() {
     let map = js_map_alloc(4);
-    for (key, value) in [(1.0, 10.0), (2.0, 20.0), (3.0, 30.0)] {
+    let expected = (0..20)
+        .map(|key| (key as f64, (key * 10) as f64))
+        .collect::<Vec<_>>();
+    for &(key, value) in &expected {
         js_map_set(map, key, value);
     }
-    FOREACH_DELETE_VISITS.with(|visits| visits.borrow_mut().clear());
-    let callback = crate::closure::js_closure_alloc(delete_current_map_entry as *const u8, 0);
+    js_map_foreach(
+        map,
+        foreach_callback(delete_current_map_entry as *const u8),
+        f64::from_bits(crate::value::TAG_UNDEFINED),
+    );
+    assert_eq!(take_foreach_delete_visits(), expected);
+    assert_eq!(js_map_size(map), 0);
+    unsafe {
+        assert_eq!(
+            (*map).used,
+            0,
+            "the completed walk runs deferred compaction"
+        )
+    };
 
+    let map = js_map_alloc(4);
+    for &(key, value) in &expected {
+        js_map_set(map, key, value);
+    }
     js_map_foreach(
         map,
-        crate::value::js_nanbox_pointer(callback as i64),
+        foreach_callback(delete_earlier_map_entry as *const u8),
         f64::from_bits(crate::value::TAG_UNDEFINED),
     );
+    assert_eq!(take_foreach_delete_visits(), expected);
+    assert_eq!(js_map_size(map), 1);
+}
 
-    let visits = FOREACH_DELETE_VISITS.with(|visits| {
-        visits
-            .borrow_mut()
-            .drain(..)
-            .map(|(key, value)| (f64::from_bits(key), f64::from_bits(value)))
-            .collect::<Vec<_>>()
-    });
-    assert_eq!(visits, vec![(1.0, 10.0), (2.0, 20.0), (3.0, 30.0)]);
-    assert_eq!(js_map_size(map), 0);
+#[test]
+fn caught_throw_restores_map_foreach_compaction_state() {
+    let map = js_map_alloc(4);
+    let base = map_foreach_stack_savepoint();
+    let _ = crate::exception::js_try_push();
+    map_foreach_enter(map);
+    assert!(map_foreach_is_active(map));
+    crate::exception::test_unwind_innermost_shadow_restore();
+    assert_eq!(map_foreach_stack_savepoint(), base);
+    assert!(!map_foreach_is_active(map));
+    crate::exception::js_try_end();
 }
 
 #[test]