diff --git a/changelog.d/9020-map-tombstone-delete.md b/changelog.d/9020-map-tombstone-delete.md new file mode 100644 index 0000000000..fbcf3e4f7f --- /dev/null +++ b/changelog.d/9020-map-tombstone-delete.md @@ -0,0 +1,30 @@ +Ordered `Map` deletes are O(1) instead of O(N). + +Emptying an N-entry Map was O(N²) three ways at once: every delete memmoved the +trailing entries down, span-barriered every moved slot, and walked all three +side indexes decrementing every offset above the hole. Per-delete cost doubled +with N (0.96 µs at N=2k to 8.2 µs at N=16k) where node is flat at ~0.03 µs — +263× at N=16k. On the ECS archetype-migration row, `delete_entry_at_index` and +its memmove were ~14% of the frame. + +A delete now tombstones in place: the key slot takes a reserved hole marker, the +value slot is cleared through the barriered store so SATB marking still shades +the overwritten child, and the live `size` drops while the array extent (`used`, +a new `MapHeader` field) stays put. Raw entry indices are therefore stable — no +shifting, no span barrier over the tail, and the side indexes only forget the +deleted key, so the offset-repair walkers are deleted outright. + +The hole marker can never collide with a stored key: `normalize_zero` +canonicalizes it to `undefined` on the resolved-key path, the string-key path +writes a STRING_TAG-boxed pointer that cannot equal it, and compaction only +moves keys that were already normalized on insert. + +Insertion order is unchanged: iteration walks raw indices and skips holes, and +delete-then-re-add still appends. Compaction runs when tombstones outnumber live +entries, before growing, or when a raw-indexed accessor observes holes — after +which the typed `for…of` lane's `used == size` admission holds again, so the +codegen lane self-heals rather than misreading holes. + +The GC contract bounds the Map slot descriptor's range by `used`, with +`size ≤ used ≤ capacity` as the corruption guard; holes are non-pointer markers +the tag-filtered scan skips. diff --git a/crates/perry-codegen/src/expr/arrays_finds.rs b/crates/perry-codegen/src/expr/arrays_finds.rs index ab957c7d4a..47fc0eb4e5 100644 --- a/crates/perry-codegen/src/expr/arrays_finds.rs +++ b/crates/perry-codegen/src/expr/arrays_finds.rs @@ -91,6 +91,18 @@ use super::index_get::numeric_index_has_integer_array_index_proof; /// other shape — a subclass instance, a plain object, an out-of-range or /// negative index, an unpublished handle — takes the runtime helper exactly /// as before, so the two paths are equivalent by construction. +/// Byte offset of `MapHeader::used`, which the tombstoned-delete lane loads to +/// check `used == size` (no holes) before admitting a raw entry read. +/// +/// `perry-codegen` does not depend on `perry-runtime`, so nothing binds this +/// literal to the struct it describes. The runtime pins the offset with an +/// `offset_of!` assertion, which catches a field REORDER — but the natural fix +/// for that assertion is to update its expected value, which leaves this string +/// stale and still compiling, and generated code would then load the wrong word +/// of the header. `map_header_used_offset_is_what_codegen_assumes` ties the two +/// together by reading the runtime source, exactly as `hot_tls.rs` does. +const MAP_HEADER_USED_OFFSET: &str = "32"; + fn lower_map_entry_at_inline( ctx: &mut FnCtx<'_>, m_box: &str, @@ -146,9 +158,18 @@ fn lower_map_entry_at_inline( let live = blk.icmp_eq(I8, &forwarded, "0"); let size_ptr = blk.inttoptr(I64, &m_handle); let size = blk.load(I32, &size_ptr); + // Tombstoned deletes leave `used > size`; a raw entry read is only + // dense-correct with no holes present, so a holey map falls back to + // the runtime helper — which compacts, after which this admission + // holds again (the lane self-heals). + let used_addr = blk.add(I64, &m_handle, MAP_HEADER_USED_OFFSET); + let used_ptr = blk.inttoptr(I64, &used_addr); + let used = blk.load(I32, &used_ptr); + let dense = blk.icmp_eq(I32, &used, &size); let in_range = blk.icmp_ult(I32, &i_i32, &size); let a = blk.and(I1, &is_map, &live); - let admitted = blk.and(I1, &a, &in_range); + let b = blk.and(I1, &a, &dense); + let admitted = blk.and(I1, &b, &in_range); blk.cond_br(&admitted, &fast_label, &slow_label); } ctx.current_block = fast_idx; @@ -1422,3 +1443,28 @@ pub(crate) fn lower( _ => unreachable!("expr/mod.rs dispatched a variant not handled by this submodule"), } } + +#[cfg(test)] +mod map_header_layout_tests { + /// The offset the runtime pins with `offset_of!(MapHeader, used) == N`. + fn runtime_map_used_offset() -> usize { + let src = include_str!("../../../perry-runtime/src/map.rs"); + let needle = "offset_of!(MapHeader, used) == "; + let rest = src + .split_once(needle) + .expect("MapHeader::used offset assertion not found in map.rs - was it renamed?") + .1; + let digits: String = rest.chars().take_while(char::is_ascii_digit).collect(); + digits.parse().expect("offset is a decimal literal") + } + + #[test] + fn map_header_used_offset_is_what_codegen_assumes() { + assert_eq!( + super::MAP_HEADER_USED_OFFSET.parse::().unwrap(), + runtime_map_used_offset(), + "codegen emits a stale MapHeader::used offset; the tombstone lane \ + would read the wrong header word and mis-admit holey maps", + ); + } +} diff --git a/crates/perry-runtime/src/collection_iter_object.rs b/crates/perry-runtime/src/collection_iter_object.rs index cb04007f73..5cdedf6669 100644 --- a/crates/perry-runtime/src/collection_iter_object.rs +++ b/crates/perry-runtime/src/collection_iter_object.rs @@ -265,26 +265,34 @@ unsafe fn dispatch_map_iterator_method_emit( } let cursor = f64::from_bits(js_object_get_field(iter_obj(), 1).bits()) as u32; let last_key = js_object_get_field(iter_obj(), 4); - let size = crate::map::js_map_size(map()); + let used = crate::map::map_used_entries(map()); // Is the last-returned key still at cursor-1? (SameValueZero, so a // NaN key matches itself.) If so, no delete shifted an entry at/below // the cursor. let in_place = cursor > 0 && { - let prev = crate::map::js_map_entry_key_at(map(), cursor - 1); + let prev = crate::map::map_entry_key_raw(map(), cursor - 1); crate::value::js_jsvalue_same_value_zero(prev, f64::from_bits(last_key.bits())) != 0 }; - let idx = next_read_index(cursor, in_place, || { + let mut idx = next_read_index(cursor, in_place, || { crate::map::find_key_index(map(), f64::from_bits(last_key.bits())) }); - if idx >= size { - js_object_set_field(iter_obj(), 1, JSValue::number(size as f64)); + // Tombstoned deletes leave holes in the raw entry order; the + // cursor walks raw indices, so step over them here. + while idx < used + && crate::map::map_entry_key_raw(map(), idx).to_bits() + == crate::map::MAP_HOLE_KEY_BITS + { + idx += 1; + } + if idx >= used { + js_object_set_field(iter_obj(), 1, JSValue::number(used as f64)); // Once a collection iterator is exhausted it stays exhausted, // even if entries are appended later. js_object_set_field(iter_obj(), 0, JSValue::undefined()); return emit_iter_result(&scope, &iter_h, emit_cached, JSValue::undefined(), true); } - let entry_key = crate::map::js_map_entry_key_at(map(), idx); + let entry_key = crate::map::map_entry_key_raw(map(), idx); // Record state for the next re-derive BEFORE any allocation below. js_object_set_field(iter_obj(), 1, JSValue::number((idx + 1) as f64)); js_object_set_field(iter_obj(), 4, JSValue::from_bits(entry_key.to_bits())); @@ -292,10 +300,10 @@ unsafe fn dispatch_map_iterator_method_emit( let value = match kind { KIND_KEYS => JSValue::from_bits(entry_key.to_bits()), KIND_VALUES => { - JSValue::from_bits(crate::map::js_map_entry_value_at(map(), idx).to_bits()) + JSValue::from_bits(crate::map::map_entry_value_raw(map(), idx).to_bits()) } _ => { - let val = crate::map::js_map_entry_value_at(map(), idx); + let val = crate::map::map_entry_value_raw(map(), idx); JSValue::from_bits(make_pair_array(entry_key, val).to_bits()) } }; diff --git a/crates/perry-runtime/src/gc/layout_slot_visit.rs b/crates/perry-runtime/src/gc/layout_slot_visit.rs index abe78b396d..b47c7444a1 100644 --- a/crates/perry-runtime/src/gc/layout_slot_visit.rs +++ b/crates/perry-runtime/src/gc/layout_slot_visit.rs @@ -215,6 +215,7 @@ pub(super) unsafe fn visit_gc_rewrite_slot_descriptors( GcRewriteDescriptorKind::Map => { let map = user_ptr as *mut crate::map::MapHeader; let size = (*map).size; + let used = (*map).used; let capacity = (*map).capacity; // Corruption guard only: mirror Set's 16M bound (set.rs // gc_element_slot_range). Every GC walk (mark, copy, rewrite, @@ -222,7 +223,7 @@ pub(super) unsafe fn visit_gc_rewrite_slot_descriptors( // lower cap makes larger maps invisible to the collector — // entries reachable only through a >cap map would be swept // while live and never rewritten after a move. - if size > capacity || size > 16_000_000 || (*map).entries.is_null() { + if size > used || used > capacity || used > 16_000_000 || (*map).entries.is_null() { return; } // Defensive tripwire (# fabricated-Map): if a fabricated Map @@ -248,7 +249,7 @@ pub(super) unsafe fn visit_gc_rewrite_slot_descriptors( return; } visit(GcMutableSlotDescriptor::Range { - range: HeapSlotRange::new((*map).entries as *mut u64, size as usize * 2), + range: HeapSlotRange::new((*map).entries as *mut u64, used as usize * 2), layout_kind: None, }); // #6759 phase 1: the metadata edge. This arm is the MARK path as diff --git a/crates/perry-runtime/src/gc/tests/barrier.rs b/crates/perry-runtime/src/gc/tests/barrier.rs index 7b2bfcb424..937f8a1ed2 100644 --- a/crates/perry-runtime/src/gc/tests/barrier.rs +++ b/crates/perry-runtime/src/gc/tests/barrier.rs @@ -15,6 +15,7 @@ unsafe fn alloc_old_test_map( let entries = std::alloc::alloc_zeroed(layout) as *mut u64; assert!(!entries.is_null()); (*map).size = 0; + (*map).used = 0; (*map).capacity = capacity; (*map).entries = entries as *mut f64; (map, entries, layout) @@ -26,6 +27,7 @@ unsafe fn retire_old_test_map( layout: std::alloc::Layout, ) { (*map).size = 0; + (*map).used = 0; (*map).capacity = 0; (*map).entries = std::ptr::null_mut(); std::alloc::dealloc(entries as *mut u8, layout); @@ -625,6 +627,7 @@ fn test_old_young_edge_verifier_accepts_map_external_slot() { let map_header = unsafe { header_from_user_ptr(map as *const u8) }; unsafe { (*map).size = 1; + (*map).used = 1; *entries = ptr_bits(young); (*map_header).gc_flags |= GC_FLAG_MARKED; } @@ -932,6 +935,7 @@ fn test_dirty_page_map_entry_scan_is_external_range_bounded() { let (map, entries, layout) = unsafe { alloc_old_test_map(2048) }; unsafe { (*map).size = 2048; + (*map).used = 2048; } let (dirty_idx, clean_idx) = unsafe { field_indices_on_distinct_pages(entries, 4096) }; let dirty_slot = unsafe { entries.add(dirty_idx) }; @@ -1045,6 +1049,7 @@ fn test_dirty_page_map_external_dedupes_and_clears() { let (map, entries, layout) = unsafe { alloc_old_test_map(16) }; unsafe { (*map).size = 16; + (*map).used = 16; *entries.add(1) = POINTER_TAG | young as u64; } let slot = unsafe { entries.add(1) }; @@ -1077,6 +1082,7 @@ fn test_dirty_page_map_realloc_span_marks_new_entries_pages() { let (map, entries, layout) = unsafe { alloc_old_test_map(1024) }; unsafe { (*map).size = 1024; + (*map).used = 1024; *entries.add(1023) = POINTER_TAG | young as u64; } let new_layout = std::alloc::Layout::from_size_align(2048 * 16, 8).unwrap(); @@ -1462,6 +1468,7 @@ fn test_incremental_barrier_marks_external_map_and_set_slots() { let (set, elements, set_layout) = unsafe { alloc_old_test_set(1) }; unsafe { (*map).size = 1; + (*map).used = 1; (*set).size = 1; } mark_user_ptr(map as usize); @@ -1687,6 +1694,7 @@ fn test_rewrite_remembered_dirty_range_updates_map_external_entry_span() { let (map, entries, layout) = unsafe { alloc_old_test_map(2048) }; unsafe { (*map).size = 2048; + (*map).used = 2048; } let (dirty_idx, clean_idx) = unsafe { field_indices_on_distinct_pages(entries, 4096) }; let dirty_slot = unsafe { entries.add(dirty_idx) }; diff --git a/crates/perry-runtime/src/gc/tests/helper_stores.rs b/crates/perry-runtime/src/gc/tests/helper_stores.rs index fcefac78cf..a553e8a7de 100644 --- a/crates/perry-runtime/src/gc/tests/helper_stores.rs +++ b/crates/perry-runtime/src/gc/tests/helper_stores.rs @@ -22,6 +22,7 @@ unsafe fn alloc_old_test_map( let entries = std::alloc::alloc_zeroed(layout) as *mut u64; assert!(!entries.is_null()); (*map).size = 0; + (*map).used = 0; (*map).capacity = capacity; (*map).entries = entries as *mut f64; (map, entries, layout) @@ -33,6 +34,7 @@ unsafe fn retire_old_test_map( layout: std::alloc::Layout, ) { (*map).size = 0; + (*map).used = 0; (*map).capacity = 0; (*map).entries = std::ptr::null_mut(); std::alloc::dealloc(entries as *mut u8, layout); @@ -102,6 +104,7 @@ fn map_and_set_external_helper_stores_preserve_young_children() { let (map, entries, layout) = unsafe { alloc_old_test_map(1) }; unsafe { (*map).size = 1; + (*map).used = 1; crate::gc::runtime_store_external_jsvalue_slot( map as usize, entries as usize, diff --git a/crates/perry-runtime/src/map.rs b/crates/perry-runtime/src/map.rs index 11d1231429..b060ce40ce 100644 --- a/crates/perry-runtime/src/map.rs +++ b/crates/perry-runtime/src/map.rs @@ -444,22 +444,6 @@ impl NumericIndex { } self.dense_key_count = 0; } - - fn repair_entry_indices_after_delete(&mut self, deleted_index: u32) { - for entry_index in self.hashed.values_mut() { - if *entry_index > deleted_index { - *entry_index -= 1; - } - } - if let Some(dense) = self.dense.as_mut() { - for entry_index in &mut dense.slots { - if *entry_index != DENSE_NUMERIC_EMPTY && *entry_index > deleted_index { - *entry_index -= 1; - } - } - } - } - fn allowed_dense_span(&self) -> usize { self.dense_key_count .saturating_mul(DENSE_NUMERIC_SPAN_FACTOR) @@ -1105,8 +1089,25 @@ pub struct MapHeader { /// marked as well as rewritten (#6812: an edge visited only on the rewrite /// path is invisible to marking). pub meta: *mut crate::object::ObjectMeta, + /// Extent of the entries array actually written: raw entry indices run + /// `0..used`. `size` stays the LIVE count, so `used - size` is the number + /// of tombstoned entries awaiting compaction. Appended last; codegen + /// reads it at offset 32 (pinned below). + pub used: u32, } +const _: () = { + assert!(std::mem::offset_of!(MapHeader, size) == 0); + assert!(std::mem::offset_of!(MapHeader, capacity) == 4); + assert!(std::mem::offset_of!(MapHeader, entries) == 8); + assert!(std::mem::offset_of!(MapHeader, used) == 32); +}; + +/// The tombstone a deleted entry's KEY slot takes. Never a legal stored key: +/// `normalize_zero` canonicalizes a leaked array hole to `undefined` before +/// any key reaches the entries buffer. +pub(crate) const MAP_HOLE_KEY_BITS: u64 = crate::value::TAG_HOLE; + /// Each map entry is 16 bytes (key + value, both as f64/JSValue) const ENTRY_SIZE: usize = 16; @@ -1133,6 +1134,12 @@ unsafe fn entries_ptr_mut(map: *mut MapHeader) -> *mut f64 { /// so `v == 0.0` stays false for them (NaN-tagged f64 is never equal to 0.0). #[inline(always)] fn normalize_zero(key: f64) -> f64 { + if key.to_bits() == MAP_HOLE_KEY_BITS { + // An array hole leaking through an untyped path reads as `undefined` + // at every other boundary; canonicalize here too, so the tombstone + // marker can never collide with a stored key. + return f64::from_bits(TAG_UNDEFINED); + } if key == 0.0 { 0.0 } else if key.is_nan() && crate::value::JSValue::from_bits(key.to_bits()).is_number() { @@ -1341,6 +1348,7 @@ pub extern "C" fn js_map_alloc(capacity: u32) -> *mut MapHeader { // zeroing, so this MUST be initialised explicitly — an uninitialised // meta edge is a garbage pointer the collector would follow. (*ptr).meta = std::ptr::null_mut(); + (*ptr).used = 0; // Register in map registry for runtime type detection register_map(ptr, entries, cap as usize); @@ -1406,6 +1414,99 @@ pub extern "C" fn js_map_find_key_index(map_boxed: f64, key: f64) -> f64 { #[used] static KEEP_MAP_FIND_KEY_INDEX: extern "C" fn(f64, f64) -> f64 = js_map_find_key_index; +/// Live-extent accessor for iteration (`0..used` are the raw entry indices). +#[inline(always)] +pub(crate) fn map_used_entries(map: *const MapHeader) -> u32 { + unsafe { (*map).used } +} + +/// Raw-indexed entry reads for the iterator objects: bound by `used`, no +/// compaction — the advance loop skips tombstones itself, so iterating a map +/// that is being emptied stays O(live + holes), not O(n) per element. +#[inline(always)] +pub(crate) unsafe fn map_entry_key_raw(map: *const MapHeader, idx: u32) -> f64 { + if idx >= (*map).used { + return f64::from_bits(TAG_UNDEFINED); + } + ptr::read(entries_ptr(map).add(idx as usize * 2)) +} + +#[inline(always)] +pub(crate) unsafe fn map_entry_value_raw(map: *const MapHeader, idx: u32) -> f64 { + if idx >= (*map).used { + return f64::from_bits(TAG_UNDEFINED); + } + ptr::read(entries_ptr(map).add(idx as usize * 2 + 1)) +} + +/// Squeeze the tombstones out: shift live pairs down (insertion order is +/// preserved — only holes are removed), then rebuild the three side indexes +/// from the dense buffer. One overlap-safe pass plus one dirty-span barrier, +/// exactly the cost ONE ordered delete used to pay — but amortized over the +/// deletes that created the holes. +unsafe fn compact_map_entries(map: *mut MapHeader) { + let used = (*map).used as usize; + let entries = entries_ptr_mut(map); + let mut out = 0usize; + for i in 0..used { + let key = ptr::read(entries.add(i * 2)); + if key.to_bits() == MAP_HOLE_KEY_BITS { + continue; + } + if out != i { + // GC_STORE_AUDIT(EXTERNAL_BARRIERED): the dirty-span barrier below + // covers every surviving slot this pass writes. Overlap-safe by + // construction -- `out <= i` always, so a live pair only ever moves + // DOWN within the one buffer, never onto an unread source. + ptr::write(entries.add(out * 2), key); + ptr::write(entries.add(out * 2 + 1), ptr::read(entries.add(i * 2 + 1))); + } + out += 1; + } + debug_assert_eq!(out as u32, (*map).size); + (*map).used = out as u32; + if out > 0 { + // GC_STORE_AUDIT(EXTERNAL_BARRIERED): compaction is followed by a dirty-span barrier for every surviving slot. + crate::gc::runtime_write_barrier_external_slot_span( + map as usize, + entries as usize, + out * 2, + ); + } + // Raw entry indices changed; rebuild the side indexes from the dense + // buffer (the same rebuilds every GC rewrite already performs). + if let Some(index) = (*map).numeric_index.as_mut() { + index.clear(); + for i in 0..out { + let bits = ptr::read(entries.add(i * 2)).to_bits(); + if is_safe_numeric_key(bits) { + index.insert(NumericKey(bits), i as u32); + } + } + } + MAP_STRING_INDEX.with(|idx| { + let mut idx = idx.borrow_mut(); + if let Some(slot) = idx.get_mut(&(map as usize)) { + slot.clear(); + for i in 0..out { + let kb = ptr::read(entries.add(i * 2)).to_bits(); + if is_string_like(kb) { + if let Some(h) = string_content_hash(kb) { + slot.entry(h).or_insert_with(Vec::new).push(i as u32); + } + } + } + } + }); + rebuild_map_ptr_index(map); +} + +pub(crate) unsafe fn compact_if_holey(map: *mut MapHeader) { + if (*map).used != (*map).size { + compact_map_entries(map); + } +} + /// The two lookups every hot `Map` does, with nothing else in the frame. /// /// `find_key_index` grew the string-hash, pointer-index and generic-compare @@ -1421,14 +1522,14 @@ static KEEP_MAP_FIND_KEY_INDEX: extern "C" fn(f64, f64) -> f64 = js_map_find_key /// path; a key outside the span goes to the hashed index there. #[inline(always)] unsafe fn find_key_index_hot(map: *const MapHeader, key: f64) -> Option { - let size = (*map).size; + let used = (*map).used; let key_bits = key.to_bits(); if !is_plain_nonzero_number_bits(key_bits) { return None; } - if size <= SIDE_TABLE_THRESHOLD { + if used <= SIDE_TABLE_THRESHOLD { let entries = entries_ptr(map); - for i in 0..size { + for i in 0..used { if ptr::read(entries.add((i as usize) * 2)).to_bits() == key_bits { return Some(i as i32); } @@ -1443,7 +1544,7 @@ unsafe fn find_key_index_hot(map: *const MapHeader, key: f64) -> Option { return None; } let entry = *dense.slots.get_unchecked(offset); - if entry == DENSE_NUMERIC_EMPTY || entry >= size { + if entry == DENSE_NUMERIC_EMPTY || entry >= used { return Some(-1); } Some(entry as i32) @@ -1463,18 +1564,18 @@ pub(crate) unsafe fn find_key_index(map: *const MapHeader, key: f64) -> i32 { /// see the hot lane. #[inline(never)] unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { - let size = (*map).size; + let used = (*map).used; let key_bits = key.to_bits(); // Small maps: linear scan beats side-table dispatch. - if size <= SIDE_TABLE_THRESHOLD { + if used <= SIDE_TABLE_THRESHOLD { let entries = entries_ptr(map); // A plain (untagged, non-NaN), non-zero number is SameValueZero-equal // to an entry key exactly when the bits match: no tagged value can // equal a number, and only `±0` / NaN break bit identity, so those // (and every non-number) keep the general comparison below. if is_plain_nonzero_number_bits(key_bits) { - for i in 0..size { + for i in 0..used { let entry_bits = ptr::read(entries.add((i as usize) * 2)).to_bits(); if entry_bits == key_bits { return i as i32; @@ -1482,8 +1583,11 @@ unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { } return -1; } - for i in 0..size { + for i in 0..used { let entry_key = ptr::read(entries.add((i as usize) * 2)); + if entry_key.to_bits() == MAP_HOLE_KEY_BITS { + continue; + } if jsvalue_eq(entry_key, key) { return i as i32; } @@ -1496,7 +1600,7 @@ unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { if is_safe_numeric_key(key_bits) { if let Some(index) = (*map).numeric_index.as_ref() { if let Some(i) = index.get(&NumericKey(key_bits)) { - if i < size { + if i < used { return i as i32; } } @@ -1518,7 +1622,7 @@ unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { // FNV-1a collisions are rare but possible; validate // each candidate via `jsvalue_eq` (memcmp on bytes). for &cand_idx in bucket { - if cand_idx >= size { + if cand_idx >= used { continue; } let cand_key = ptr::read(entries.add((cand_idx as usize) * 2)); @@ -1547,7 +1651,7 @@ unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { let idx = idx.borrow(); if let Some(slot) = idx.get(&(map as usize)) { if let Some(&i) = slot.get(&MapPtrKey(key)) { - if i < size { + if i < used { return Some(i as i32); } } @@ -1562,8 +1666,11 @@ unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { // Linear scan for maps with no side-table entry. let entries = entries_ptr(map); - for i in 0..size { + for i in 0..used { let entry_key = ptr::read(entries.add((i as usize) * 2)); + if entry_key.to_bits() == MAP_HOLE_KEY_BITS { + continue; + } if jsvalue_eq(entry_key, key) { return i as i32; } @@ -1573,14 +1680,17 @@ unsafe fn find_key_index_cold(map: *const MapHeader, key: f64) -> i32 { } unsafe fn find_string_key_index(map: *const MapHeader, key: *const StringHeader) -> i32 { - let size = (*map).size; + let used = (*map).used; let key_value = boxed_heap_string_key(key); let key_bits = key_value.to_bits(); - if size <= SIDE_TABLE_THRESHOLD { + if used <= SIDE_TABLE_THRESHOLD { let entries = entries_ptr(map); - for i in 0..size { + for i in 0..used { let entry_key = ptr::read(entries.add((i as usize) * 2)); + if entry_key.to_bits() == MAP_HOLE_KEY_BITS { + continue; + } if jsvalue_eq(entry_key, key_value) { return i as i32; } @@ -1595,7 +1705,7 @@ unsafe fn find_string_key_index(map: *const MapHeader, key: *const StringHeader) if let Some(slot) = idx.get(&(map as usize)) { if let Some(bucket) = slot.get(&h) { for &cand_idx in bucket { - if cand_idx >= size { + if cand_idx >= used { continue; } let cand_key = ptr::read(entries.add((cand_idx as usize) * 2)); @@ -1614,8 +1724,11 @@ unsafe fn find_string_key_index(map: *const MapHeader, key: *const StringHeader) } let entries = entries_ptr(map); - for i in 0..size { + for i in 0..used { let entry_key = ptr::read(entries.add((i as usize) * 2)); + if entry_key.to_bits() == MAP_HOLE_KEY_BITS { + continue; + } if jsvalue_eq(entry_key, key_value) { return i as i32; } @@ -1626,12 +1739,19 @@ unsafe fn find_string_key_index(map: *const MapHeader, key: *const StringHeader) /// Grow the entries array if needed (header stays at same address) unsafe fn ensure_capacity(map: *mut MapHeader) -> bool { - let size = (*map).size; - let capacity = (*map).capacity; - - if size < capacity { + if (*map).used < (*map).capacity { return false; } + // 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 { + compact_map_entries(map); + if (*map).used < (*map).capacity { + return false; + } + } + let capacity = (*map).capacity; // Double the capacity let new_capacity = capacity * 2; @@ -1697,18 +1817,19 @@ unsafe fn map_set_string_key_value( let key = key_handle.get_raw_const_ptr::(); let value = value_handle.get_nanbox_f64(); let size = (*map).size; + let used = (*map).used; let entries = entries_ptr_mut(map); - if grew && size > 0 { + if grew && used > 0 { crate::gc::runtime_write_barrier_external_slot_span( map as usize, entries as usize, - size as usize * 2, + used as usize * 2, ); } let key_value = boxed_heap_string_key(key); - let key_slot = entries.add((size as usize) * 2); - let value_slot = entries.add((size as usize) * 2 + 1); + let key_slot = entries.add((used as usize) * 2); + let value_slot = entries.add((used as usize) * 2 + 1); // GC_STORE_AUDIT(EXTERNAL_BARRIERED): map append key/value slots use the shared external-slot helper. crate::gc::runtime_store_external_jsvalue_slot( map as usize, @@ -1722,6 +1843,7 @@ unsafe fn map_set_string_key_value( ); (*map).size = size + 1; + (*map).used = used + 1; if let Some(h) = string_content_hash(key_value.to_bits()) { MAP_STRING_INDEX.with(|idx| { @@ -1729,7 +1851,7 @@ unsafe fn map_set_string_key_value( let slot = idx .entry(map as usize) .or_insert_with(std::collections::HashMap::new); - slot.entry(h).or_insert_with(Vec::new).push(size); + slot.entry(h).or_insert_with(Vec::new).push(used); }); } @@ -1806,17 +1928,18 @@ fn map_set_resolved(map: *mut MapHeader, key: f64, value: f64) { let key = key_handle.get_nanbox_f64(); let value = value_handle.get_nanbox_f64(); let size = (*map).size; + let used = (*map).used; let entries = entries_ptr_mut(map); - if grew && size > 0 { + if grew && used > 0 { crate::gc::runtime_write_barrier_external_slot_span( map as usize, entries as usize, - size as usize * 2, + used as usize * 2, ); } - let key_slot = entries.add((size as usize) * 2); - let value_slot = entries.add((size as usize) * 2 + 1); + let key_slot = entries.add((used as usize) * 2); + let value_slot = entries.add((used as usize) * 2 + 1); // GC_STORE_AUDIT(EXTERNAL_BARRIERED): map append key/value slots use the shared external-slot helper. crate::gc::runtime_store_external_jsvalue_slot( map as usize, @@ -1830,6 +1953,7 @@ fn map_set_resolved(map: *mut MapHeader, key: f64, value: f64) { ); (*map).size = size + 1; + (*map).used = used + 1; // Update the O(1) side-tables: numeric keys by bits, string keys by // content hash, pointer keys (objects/symbols/bigints) in the @@ -1837,7 +1961,7 @@ fn map_set_resolved(map: *mut MapHeader, key: f64, value: f64) { let key_bits = key.to_bits(); if is_safe_numeric_key(key_bits) { if let Some(index) = (*map).numeric_index.as_mut() { - index.insert(NumericKey(key_bits), size); + index.insert(NumericKey(key_bits), used); } } else if is_string_like(key_bits) { // String key: content-hashed index bypasses the gen-GC stale-bits @@ -1849,7 +1973,7 @@ fn map_set_resolved(map: *mut MapHeader, key: f64, value: f64) { let slot = idx .entry(map as usize) .or_insert_with(std::collections::HashMap::new); - slot.entry(h).or_insert_with(Vec::new).push(size); + slot.entry(h).or_insert_with(Vec::new).push(used); }); } } else { @@ -1858,7 +1982,7 @@ fn map_set_resolved(map: *mut MapHeader, key: f64, value: f64) { let slot = idx .entry(map as usize) .or_insert_with(crate::fast_hash::new_ptr_hash_map); - slot.insert(MapPtrKey(key), size); + slot.insert(MapPtrKey(key), used); }); } } @@ -2269,89 +2393,81 @@ unsafe fn delete_entry_at_index(map: *mut MapHeader, idx: i32) -> i32 { } let size = (*map).size; let idx = idx as usize; - if idx >= size as usize { + if idx >= (*map).used as usize { return 0; } let entries = entries_ptr_mut(map); let deleted_key = ptr::read(entries.add(idx * 2)); - // #2831: preserve insertion order. JS Map iteration must keep the - // relative order of surviving entries after a delete (and a - // delete-then-re-add appends at the end). The previous swap-and-pop - // moved the last entry into the hole, reordering iteration. Compact the - // already-owned key/value pairs with one overlap-safe move. This does not - // create a new parent -> child edge: every copied value was already in - // this Map. The span mark preserves the old -> young remembered-set - // contract for the slots' new addresses without paying two full runtime - // stores per entry. - let moved_entries = size as usize - idx - 1; - if moved_entries > 0 { - // GC_STORE_AUDIT(EXTERNAL_BARRIERED): ordered compaction is followed by a dirty-span barrier for every moved slot. - ptr::copy( - entries.add((idx + 1) * 2), - entries.add(idx * 2), - moved_entries * 2, - ); - crate::gc::runtime_write_barrier_external_slot_span( - map as usize, - entries.add(idx * 2) as usize, - moved_entries * 2, - ); - } + // O(1) ordered delete (#2831 preserved): survivors keep their RAW entry + // indices, so nothing shifts, nothing is memmoved, no span barrier over + // the tail, and no side-index offsets need repairing — the three O(n) + // costs that made emptying an N-entry map O(N²) (18.7x node on the ECS + // archetype-migration row). The entry is TOMBSTONED: its key slot takes + // the reserved hole marker (never a legal stored key — `normalize_zero` + // canonicalizes a leaked hole to `undefined`), and its value slot is + // cleared through the barriered store so SATB marking still shades the + // overwritten child. Iteration walks raw indices and skips holes; + // delete-then-re-add still appends at the end. Tombstones are squeezed + // out when they outnumber the live entries, or on growth. + crate::gc::runtime_store_external_jsvalue_slot( + map as usize, + entries.add(idx * 2) as usize, + MAP_HOLE_KEY_BITS, + ); + crate::gc::runtime_store_external_jsvalue_slot( + map as usize, + entries.add(idx * 2 + 1) as usize, + crate::value::TAG_UNDEFINED, + ); (*map).size = size - 1; + forget_map_index_entry(map, deleted_key, idx as u32); - // The old implementation rebuilt all three indexes from the entries - // buffer after every ordered delete. Repair their existing u32 offsets - // in place instead: removing one key and decrementing later offsets is a - // cache-linear pass over index values and does not re-hash surviving keys. - repair_map_indices_after_ordered_delete(map, deleted_key, idx as u32); + let used = (*map).used; + if used >= 16 && (*map).size < used / 2 { + compact_map_entries(map); + } 1 } -unsafe fn repair_map_indices_after_ordered_delete( - map: *mut MapHeader, - deleted_key: f64, - deleted_idx: u32, -) { +/// Forget ONE deleted key from whichever side index holds it. Raw entry +/// indices are stable under tombstoned deletes, so — unlike the pre-tombstone +/// repair — no surviving offset is touched. +unsafe fn forget_map_index_entry(map: *mut MapHeader, deleted_key: f64, deleted_idx: u32) { let map_addr = map as usize; let deleted_bits = deleted_key.to_bits(); - if let Some(index) = (*map).numeric_index.as_mut() { - if is_safe_numeric_key(deleted_bits) { + if is_safe_numeric_key(deleted_bits) { + if let Some(index) = (*map).numeric_index.as_mut() { index.remove(&NumericKey(deleted_bits)); } - index.repair_entry_indices_after_delete(deleted_idx); + return; } - - MAP_STRING_INDEX.with(|indexes| { - let mut indexes = indexes.borrow_mut(); - if let Some(index) = indexes.get_mut(&map_addr) { - for bucket in index.values_mut() { - bucket.retain(|entry_idx| *entry_idx != deleted_idx); - for entry_idx in bucket { - if *entry_idx > deleted_idx { - *entry_idx -= 1; + if is_string_like(deleted_bits) { + if let Some(h) = string_content_hash(deleted_bits) { + MAP_STRING_INDEX.with(|indexes| { + let mut indexes = indexes.borrow_mut(); + if let Some(index) = indexes.get_mut(&map_addr) { + if let Some(bucket) = index.get_mut(&h) { + bucket.retain(|entry_idx| *entry_idx != deleted_idx); + if bucket.is_empty() { + index.remove(&h); + } } } - } - index.retain(|_, bucket| !bucket.is_empty()); + }); } - }); - - MAP_PTR_INDEX.with(|indexes| { - let mut indexes = indexes.borrow_mut(); - if let Some(index) = indexes.get_mut(&map_addr) { - if is_ptr_index_key(deleted_bits) { + return; + } + if is_ptr_index_key(deleted_bits) { + MAP_PTR_INDEX.with(|indexes| { + let mut indexes = indexes.borrow_mut(); + if let Some(index) = indexes.get_mut(&map_addr) { index.remove(&MapPtrKey(deleted_key)); } - for entry_idx in index.values_mut() { - if *entry_idx > deleted_idx { - *entry_idx -= 1; - } - } - } - }); + }); + } } /// Rebuild ONLY the pointer-key index for `map` from its current entries @@ -2362,9 +2478,9 @@ unsafe fn rebuild_map_ptr_index(map: *mut MapHeader) { if map.is_null() { return; } - let size = (*map).size as usize; + let used = (*map).used as usize; let capacity = (*map).capacity as usize; - if size > capacity || size > 16_000_000 || (*map).entries.is_null() { + if used > capacity || used > 16_000_000 || (*map).entries.is_null() { return; } let entries = entries_ptr(map); @@ -2374,7 +2490,7 @@ unsafe fn rebuild_map_ptr_index(map: *mut MapHeader) { .entry(map as usize) .or_insert_with(crate::fast_hash::new_ptr_hash_map); slot.clear(); - for i in 0..size { + for i in 0..used { let entry_key = ptr::read(entries.add(i * 2)); if is_ptr_index_key(entry_key.to_bits()) { slot.insert(MapPtrKey(entry_key), i as u32); @@ -2406,6 +2522,7 @@ pub extern "C" fn js_map_clear(map: *mut MapHeader) { // map has nothing to reset: the per-entity `adds.clear(); removes.clear()` // of a change set is this case half the time. let size = unsafe { (*map).size }; + let used = unsafe { (*map).used }; if size == 0 { return; } @@ -2415,16 +2532,17 @@ pub extern "C" fn js_map_clear(map: *mut MapHeader) { // cheaper than the two thread-local resolutions plus two hash probes // that find two empty tables — the per-frame grouping maps of an ECS // are this shape, ten thousand clears a frame. - let side_tables_may_hold_this_map = size > SIDE_TABLE_CLEAR_SCAN_MAX + let side_tables_may_hold_this_map = used > SIDE_TABLE_CLEAR_SCAN_MAX || unsafe { let entries = entries_ptr(map); - (0..size as usize).any(|i| { + (0..used as usize).any(|i| { let key_bits = ptr::read(entries.add(i * 2)).to_bits(); !is_safe_numeric_key(key_bits) }) }; unsafe { (*map).size = 0; + (*map).used = 0; } unsafe { if let Some(index) = (*map).numeric_index.as_mut() { @@ -2460,6 +2578,13 @@ pub extern "C" fn js_map_entry_key_at(map: *const MapHeader, idx: u32) -> f64 { return f64::from_bits(TAG_UNDEFINED); } unsafe { + if (*map).used != (*map).size { + // Tombstones present under a raw-indexed read: the typed for-of + // lane and this fallback iterate raw indices against the live + // size, so squeeze the holes out — after which the codegen lane's + // `used == size` admission holds again and the lane self-heals. + compact_map_entries(map as *mut MapHeader); + } let size = (*map).size; if idx >= size { return f64::from_bits(TAG_UNDEFINED); @@ -2477,6 +2602,13 @@ pub extern "C" fn js_map_entry_value_at(map: *const MapHeader, idx: u32) -> f64 return f64::from_bits(TAG_UNDEFINED); } unsafe { + if (*map).used != (*map).size { + // Tombstones present under a raw-indexed read: the typed for-of + // lane and this fallback iterate raw indices against the live + // size, so squeeze the holes out — after which the codegen lane's + // `used == size` admission holds again and the lane self-heals. + compact_map_entries(map as *mut MapHeader); + } let size = (*map).size; if idx >= size { return f64::from_bits(TAG_UNDEFINED); @@ -2494,6 +2626,7 @@ pub extern "C" fn js_map_entries(map: *const MapHeader) -> *mut crate::array::Ar if map.is_null() { return crate::array::js_array_alloc(0); } + unsafe { compact_if_holey(map as *mut MapHeader) }; let scope = crate::gc::RuntimeHandleScope::new(); let map_handle = scope.root_raw_const_ptr(map); unsafe { @@ -2547,6 +2680,7 @@ pub extern "C" fn js_map_keys(map: *const MapHeader) -> *mut crate::array::Array if map.is_null() { return crate::array::js_array_alloc(0); } + unsafe { compact_if_holey(map as *mut MapHeader) }; let scope = crate::gc::RuntimeHandleScope::new(); let map_handle = scope.root_raw_const_ptr(map); unsafe { @@ -2579,6 +2713,7 @@ pub extern "C" fn js_map_values(map: *const MapHeader) -> *mut crate::array::Arr if map.is_null() { return crate::array::js_array_alloc(0); } + unsafe { compact_if_holey(map as *mut MapHeader) }; let scope = crate::gc::RuntimeHandleScope::new(); let map_handle = scope.root_raw_const_ptr(map); unsafe { @@ -2617,6 +2752,7 @@ fn copy_map_into_new(src: *const MapHeader) -> *mut MapHeader { if src.is_null() { return js_map_alloc(4); } + unsafe { compact_if_holey(src as *const MapHeader as *mut MapHeader) }; let src_handle = scope.root_raw_const_ptr(src); let size = unsafe { let s = src_handle.get_raw_const_ptr::(); @@ -2903,6 +3039,7 @@ fn js_map_foreach_impl( if map.is_null() { return; } + 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); @@ -3488,3 +3625,7 @@ mod tests { } } } + +#[cfg(test)] +#[path = "map_tombstone_tests.rs"] +mod map_tombstone_tests; diff --git a/crates/perry-runtime/src/map_tombstone_tests.rs b/crates/perry-runtime/src/map_tombstone_tests.rs new file mode 100644 index 0000000000..9c8de86d3d --- /dev/null +++ b/crates/perry-runtime/src/map_tombstone_tests.rs @@ -0,0 +1,163 @@ +//! Tombstoned ordered deletes (#2831 semantics, O(1) cost). +//! +//! A delete no longer shifts survivors: the entry is holed in place, raw +//! entry indices stay stable, and compaction runs only when tombstones +//! outnumber live entries or the array must grow. These tests pin the +//! observable contract — insertion order, delete-then-re-add, lookup +//! correctness across holes, iterator hole-skips, and the self-healing +//! compaction under raw-indexed access. + +use super::*; + +#[test] +fn ordered_delete_preserves_order_and_lookup_across_holes() { + let map = js_map_alloc(8); + for k in [10.0f64, 20.0, 30.0, 40.0, 50.0] { + js_map_set(map, k, k * 10.0); + } + + assert_eq!(js_map_delete(map, 30.0), 1, "middle"); + assert_eq!(js_map_delete(map, 10.0), 1, "front"); + assert_eq!(js_map_delete(map, 50.0), 1, "back"); + unsafe { + assert_eq!((*map).size, 2); + assert!((*map).used >= 2, "holes may remain before compaction"); + } + + // Survivors resolve, deleted keys do not — through every lookup lane. + assert_eq!(js_map_get(map, 20.0), 200.0); + assert_eq!(js_map_get(map, 40.0), 400.0); + for gone in [10.0f64, 30.0, 50.0] { + assert_eq!(js_map_has(map, gone), 0, "{gone} was deleted"); + } + + // Delete-then-re-add appends at the end (#2831): iteration order is + // 20, 40, 30 after re-adding 30. + js_map_set(map, 30.0, 999.0); + unsafe { compact_if_holey(map) }; + unsafe { + let entries = entries_ptr(map); + assert_eq!(ptr::read(entries), 20.0); + assert_eq!(ptr::read(entries.add(2)), 40.0); + assert_eq!(ptr::read(entries.add(4)), 30.0); + } + assert_eq!(js_map_get(map, 30.0), 999.0); +} + +#[test] +fn emptying_a_map_stays_consistent_and_compacts() { + let map = js_map_alloc(16); + for i in 0..64 { + js_map_set(map, i as f64, (i * 2) as f64); + } + for i in 0..64 { + assert_eq!(js_map_delete(map, i as f64), 1, "key {i} deletes once"); + assert_eq!(js_map_delete(map, i as f64), 0, "and only once"); + } + unsafe { + assert_eq!((*map).size, 0); + assert!( + (*map).used < 64, + "the tombstone threshold must have compacted at least once (used = {})", + (*map).used + ); + } + for i in 0..64 { + assert_eq!(js_map_has(map, i as f64), 0); + } + js_map_set(map, 7.0, 70.0); + assert_eq!( + js_map_get(map, 7.0), + 70.0, + "the emptied map still accepts inserts" + ); +} + +#[test] +fn raw_indexed_access_self_heals_by_compacting() { + let map = js_map_alloc(8); + for k in [1.0f64, 2.0, 3.0] { + js_map_set(map, k, k); + } + assert_eq!(js_map_delete(map, 2.0), 1); + unsafe { + assert_ne!((*map).used, (*map).size, "a hole is present"); + } + // The raw-indexed extern compacts first, so entry 1 is the THIRD key — + // exactly what the typed for-of lane's fallback needs for raw == live. + assert_eq!(js_map_entry_key_at(map, 1), 3.0); + unsafe { + assert_eq!((*map).used, (*map).size, "access healed the layout"); + } +} + +#[test] +fn iterator_skips_holes_and_survives_deleting_the_last_returned_key() { + unsafe { + let iter = crate::value::js_nanbox_pointer( + crate::collection_iter_object::js_map_keys_iter_obj(map_with(&[1.0, 2.0, 3.0, 4.0])), + ); + let key = |r: f64| { + f64::from_bits( + crate::object::js_object_get_field( + crate::value::js_nanbox_get_pointer(r) as *mut crate::object::ObjectHeader, + 0, + ) + .bits(), + ) + }; + let done = |r: f64| { + crate::value::JSValue::from_bits( + crate::object::js_object_get_field( + crate::value::js_nanbox_get_pointer(r) as *mut crate::object::ObjectHeader, + 1, + ) + .bits(), + ) + .as_bool() + }; + let next = |iter: f64| crate::collection_iter_object::js_for_of_next(iter); + + let backing = iter_backing(iter); + let r = next(iter); + assert_eq!(key(r), 1.0); + // Delete the key we just returned, and one ahead of the cursor. + js_map_delete(backing, 1.0); + js_map_delete(backing, 3.0); + let r = next(iter); + assert_eq!(key(r), 2.0, "hole at the cursor's resume point is skipped"); + let r = next(iter); + assert_eq!(key(r), 4.0, "hole ahead of the cursor is skipped"); + assert!(done(next(iter)), "then exhausted"); + } +} + +#[test] +fn clear_resets_the_extent() { + let map = js_map_alloc(4); + js_map_set(map, 1.0, 1.0); + js_map_set(map, 2.0, 2.0); + js_map_delete(map, 1.0); + js_map_clear(map); + unsafe { + assert_eq!((*map).size, 0); + assert_eq!((*map).used, 0); + } + js_map_set(map, 9.0, 90.0); + assert_eq!(js_map_get(map, 9.0), 90.0); +} + +fn map_with(keys: &[f64]) -> *mut MapHeader { + let map = js_map_alloc(8); + for &k in keys { + js_map_set(map, k, k * 100.0); + } + map +} + +unsafe fn iter_backing(iter: f64) -> *mut MapHeader { + let obj = crate::value::js_nanbox_get_pointer(iter) as *mut crate::object::ObjectHeader; + crate::value::js_nanbox_get_pointer(f64::from_bits( + crate::object::js_object_get_field(obj, 0).bits(), + )) as *mut MapHeader +}