From 41d3e0d0a51849349981625a2d59c76d255879f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 18:54:24 +0200 Subject: [PATCH 1/2] fix(runtime): a live-index read inside forEach defers the squeeze instead of shifting the walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #9504 made the array-like `map[i]` / `set[i]` read a live-index accessor: it squeezes tombstones so raw index == live index and never hands out a hole. `forEach` walks the raw entries with a counter that the delete-path squeeze defers around (the walk registers itself) — but the accessor's squeeze had no such guard. A callback that deleted already-visited entries and then read `map[j]` compacted the buffer under the walk's counter, shifting the survivors below it: with two earlier entries deleted, two later ones were never visited (18 of 20, Map and Set). While a walk is active the accessor now defers the squeeze exactly as the delete path does and resolves the live index by stepping over the tombstones (O(idx), on a path that is rare by construction); the outermost walk's completion performs the deferred squeeze as before. Outside a walk the accessor squeezes as #9504 specified, and #9504's `a_tombstoned_collection_never_hands_a_hole_to_an_indexed_read` stays green. The for…of fast path is unaffected: its cursor rebases through the compaction log (#9513), so a squeeze under it was already exact. Found by the automated review on #9513, reproduced on merged main. Tests: `a_live_index_read_inside_foreach_defers_the_squeeze_and_skips_nothing` for Map and Set (every entry visited once; live values read mid-walk; layout left alone during the walk, squeezed on completion; the accessor still squeezes outside a walk) and `test_gap_foreach_live_index_read_no_skip.ts` (single and nested walks), node-differential. perry-runtime 3018 passed / 0 failed (release, single-threaded); #9504's and #9513's fixtures still match node; cargo fmt --all --check clean; no clippy warning in the touched files. --- changelog.d/0000-foreach-live-index-read.md | 24 +++++++ crates/perry-runtime/src/map.rs | 50 +++++++++++--- .../perry-runtime/src/map_tombstone_tests.rs | 69 +++++++++++++++++++ crates/perry-runtime/src/set.rs | 38 ++++++++-- .../perry-runtime/src/set_tombstone_tests.rs | 60 ++++++++++++++++ ...est_gap_foreach_live_index_read_no_skip.ts | 43 ++++++++++++ 6 files changed, 272 insertions(+), 12 deletions(-) create mode 100644 changelog.d/0000-foreach-live-index-read.md create mode 100644 test-files/test_gap_foreach_live_index_read_no_skip.ts diff --git a/changelog.d/0000-foreach-live-index-read.md b/changelog.d/0000-foreach-live-index-read.md new file mode 100644 index 0000000000..57b5801857 --- /dev/null +++ b/changelog.d/0000-foreach-live-index-read.md @@ -0,0 +1,24 @@ +**Map/Set: an array-like `map[i]` / `set[i]` read inside a `forEach` callback no longer makes the walk skip entries.** + +`#9504` made the array-like indexed read on a collection a *live-index* +accessor: it squeezes tombstones first so raw index == live index, and never +hands out a hole. `forEach` walks the raw entries with a counter that is +protected against the *delete-path* squeeze (the walk registers itself and +that squeeze defers) — but not against the accessor's. A callback that deleted +already-visited entries and then read `map[j]` compacted the buffer under the +walk's counter, shifting the survivors below it: with two earlier entries +deleted, two later ones were never visited (18 of 20). + +While a `forEach` walk is active the accessor now defers the squeeze exactly +as the delete path does, and resolves the live index by stepping over the +tombstones (O(idx), on a path that is rare by construction); the outermost +walk's completion performs the deferred squeeze as before. Outside a walk the +accessor squeezes as `#9504` specified. The `for…of` fast path is unaffected — +its cursor rebases through the compaction log (`#9513`), so a squeeze under it +was already exact. + +Found by the automated review on #9513. Regressions: runtime unit tests for +Map and Set (visit set, live values read mid-walk, layout left alone during +the walk, squeezed on completion) and +`test_gap_foreach_live_index_read_no_skip.ts` (single and nested walks), +node-differential. diff --git a/crates/perry-runtime/src/map.rs b/crates/perry-runtime/src/map.rs index 316e0c53a6..7fb8e0c0cb 100644 --- a/crates/perry-runtime/src/map.rs +++ b/crates/perry-runtime/src/map.rs @@ -2899,13 +2899,48 @@ pub extern "C" fn js_map_entry_key_at(map: *const MapHeader, idx: u32) -> f64 { // exactly. The walkers themselves never come here: they read through // `js_map_entry_key_raw_at`, which never compacts (a compaction per // observed hole is what made delete-during-`for…of` O(n) per delete). - compact_if_holey(map as *mut MapHeader); - if idx >= (*map).size { + // + // While a `forEach` walk is active the squeeze is DEFERRED instead — + // that walk's raw counter is not epoch-rebased, so compacting under it + // (a callback that deletes an earlier entry and then reads `map[j]`) + // shifted the survivors below its counter and it skipped entries. The + // live index is translated by stepping over holes, and the outermost + // walk's completion performs the deferred squeeze as it already does + // for the delete path. + let Some(raw) = live_index_to_raw(map, idx) else { return f64::from_bits(TAG_UNDEFINED); - } + }; let entries = entries_ptr(map); - ptr::read(entries.add(idx as usize * 2)) + ptr::read(entries.add(raw as usize * 2)) + } +} + +/// Resolve a LIVE entry index onto the raw entries buffer for the live-index +/// accessors: squeeze first when no `forEach` walk is active (so raw == live +/// and the read is O(1)), otherwise leave the layout alone and step over the +/// tombstones (O(idx)). `None` past the live size. +unsafe fn live_index_to_raw(map: *const MapHeader, idx: u32) -> Option { + if idx >= (*map).size { + return None; + } + if !map_foreach_is_active(map) { + compact_if_holey(map as *mut MapHeader); + return Some(idx); + } + let used = (*map).used; + let entries = entries_ptr(map); + let mut live = 0u32; + let mut raw = 0u32; + while raw < used { + if ptr::read(entries.add(raw as usize * 2)).to_bits() != MAP_HOLE_KEY_BITS { + if live == idx { + return Some(raw); + } + live += 1; + } + raw += 1; } + None } /// RAW twin of `js_map_entry_key_at` for the raw-index walkers (the `for…of` @@ -2940,12 +2975,11 @@ pub extern "C" fn js_map_entry_value_at(map: *const MapHeader, idx: u32) -> f64 } unsafe { // Live-index accessor — see `js_map_entry_key_at`. - compact_if_holey(map as *mut MapHeader); - if idx >= (*map).size { + let Some(raw) = live_index_to_raw(map, idx) else { return f64::from_bits(TAG_UNDEFINED); - } + }; let entries = entries_ptr(map); - ptr::read(entries.add(idx as usize * 2 + 1)) + ptr::read(entries.add(raw as usize * 2 + 1)) } } diff --git a/crates/perry-runtime/src/map_tombstone_tests.rs b/crates/perry-runtime/src/map_tombstone_tests.rs index 3f95b3d546..eb06abc483 100644 --- a/crates/perry-runtime/src/map_tombstone_tests.rs +++ b/crates/perry-runtime/src/map_tombstone_tests.rs @@ -485,3 +485,72 @@ fn clear_truncates_the_squeeze_history() { ); assert_eq!(js_map_entry_key_raw_at(map, 0), 7.0); } + +extern "C" fn delete_earlier_then_live_read( + _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 == 5.0 { + let map = crate::value::js_nanbox_get_pointer(collection) as *mut MapHeader; + js_map_delete(map, 1.0); + js_map_delete(map, 2.0); + // The array-like `map[0]` read: a LIVE-index accessor. It must answer + // the live element without squeezing the layout under the walk. + assert_eq!(js_map_entry_key_at(map, 0), 0.0, "live index 0 is key 0"); + assert_eq!( + js_map_entry_key_at(map, 1), + 3.0, + "live index 1 skips the two holes" + ); + assert_eq!(js_map_entry_value_at(map, 1), 30.0); + unsafe { + assert_ne!( + (*map).used, + (*map).size, + "the walk's raw layout was left alone" + ); + } + } + f64::from_bits(crate::value::TAG_UNDEFINED) +} + +#[test] +fn a_live_index_read_inside_foreach_defers_the_squeeze_and_skips_nothing() { + // A callback that deletes already-visited entries and then reads `map[j]` + // used to compact the entries under forEach's raw counter, shifting the + // survivors below it: keys 6 and 7 were never visited. + let map = js_map_alloc(32); + let expected = (0..20) + .map(|key| (key as f64, (key * 10) as f64)) + .collect::>(); + for &(key, value) in &expected { + js_map_set(map, key, value); + } + js_map_foreach( + map, + foreach_callback(delete_earlier_then_live_read as *const u8), + f64::from_bits(crate::value::TAG_UNDEFINED), + ); + assert_eq!( + take_foreach_delete_visits(), + expected, + "every entry visited exactly once" + ); + assert_eq!(js_map_size(map), 18); + unsafe { + assert_eq!( + (*map).used, + (*map).size, + "the outermost walk's completion squeezed" + ); + } + // Outside a walk the accessor squeezes as before (#9504 contract). + js_map_delete(map, 3.0); + assert_eq!(js_map_entry_key_at(map, 1), 4.0); + unsafe { + assert_eq!((*map).used, (*map).size); + } +} diff --git a/crates/perry-runtime/src/set.rs b/crates/perry-runtime/src/set.rs index 141a26213d..5bb25eade3 100644 --- a/crates/perry-runtime/src/set.rs +++ b/crates/perry-runtime/src/set.rs @@ -1879,13 +1879,43 @@ pub extern "C" fn js_set_value_at(set: *const SetHeader, i: u32) -> f64 { // those paths only, RECORDED in the compaction log so an open // `for…of` cursor rebases exactly. The walkers read through // `js_set_value_raw_at`, which never compacts. - compact_if_holey_set(set as *mut SetHeader); - if i >= (*set).size { + // + // While a `forEach` walk is active the squeeze is DEFERRED (its raw + // counter is not epoch-rebased; compacting under it skipped entries) + // and the live index is translated by stepping over holes; the + // outermost walk's completion performs the deferred squeeze. + let Some(raw) = live_index_to_raw_set(set, i) else { return f64::from_bits(UNDEF); - } + }; let elements = (*set).elements as *const f64; - ptr::read(elements.add(i as usize)) + ptr::read(elements.add(raw as usize)) + } +} + +/// Resolve a LIVE element index onto the raw elements buffer — see +/// `map::live_index_to_raw`. +unsafe fn live_index_to_raw_set(set: *const SetHeader, idx: u32) -> Option { + if idx >= (*set).size { + return None; + } + if !set_foreach_is_active(set) { + compact_if_holey_set(set as *mut SetHeader); + return Some(idx); + } + let used = (*set).used; + let elements = elements_ptr(set); + let mut live = 0u32; + let mut raw = 0u32; + while raw < used { + if ptr::read(elements.add(raw as usize)).to_bits() != SET_HOLE_VALUE_BITS { + if live == idx { + return Some(raw); + } + live += 1; + } + raw += 1; } + None } /// RAW twin of `js_set_value_at` for the raw-index walkers (the `for…of` diff --git a/crates/perry-runtime/src/set_tombstone_tests.rs b/crates/perry-runtime/src/set_tombstone_tests.rs index f0ebd355f8..7f3adfbcd4 100644 --- a/crates/perry-runtime/src/set_tombstone_tests.rs +++ b/crates/perry-runtime/src/set_tombstone_tests.rs @@ -394,3 +394,63 @@ fn clear_truncates_the_squeeze_history() { ); assert_eq!(js_set_value_raw_at(set, 0), 7.0); } + +extern "C" fn delete_earlier_then_live_read( + _closure: *const crate::closure::ClosureHeader, + value: f64, + _value_again: f64, + collection: f64, +) -> f64 { + FOREACH_DELETE_VISITS.with(|visits| visits.borrow_mut().push(value.to_bits())); + if value == 5.0 { + let set = crate::value::js_nanbox_get_pointer(collection) as *mut SetHeader; + js_set_delete(set, 1.0); + js_set_delete(set, 2.0); + assert_eq!(js_set_value_at(set, 0), 0.0, "live index 0 is value 0"); + assert_eq!( + js_set_value_at(set, 1), + 3.0, + "live index 1 skips the two holes" + ); + unsafe { + assert_ne!( + (*set).used, + (*set).size, + "the walk's raw layout was left alone" + ); + } + } + f64::from_bits(crate::value::TAG_UNDEFINED) +} + +#[test] +fn a_live_index_read_inside_foreach_defers_the_squeeze_and_skips_nothing() { + let expected = (0..20).map(|value| value as f64).collect::>(); + let set = js_set_alloc(32); + for &value in &expected { + js_set_add(set, value); + } + js_set_foreach( + set, + foreach_callback(delete_earlier_then_live_read as *const u8), + f64::from_bits(crate::value::TAG_UNDEFINED), + ); + assert_eq!( + take_foreach_delete_visits(), + expected, + "every element visited exactly once" + ); + assert_eq!(js_set_size(set), 18); + unsafe { + assert_eq!( + (*set).used, + (*set).size, + "the outermost walk's completion squeezed" + ); + } + js_set_delete(set, 3.0); + assert_eq!(js_set_value_at(set, 1), 4.0); + unsafe { + assert_eq!((*set).used, (*set).size); + } +} diff --git a/test-files/test_gap_foreach_live_index_read_no_skip.ts b/test-files/test_gap_foreach_live_index_read_no_skip.ts new file mode 100644 index 0000000000..29e5f040a9 --- /dev/null +++ b/test-files/test_gap_foreach_live_index_read_no_skip.ts @@ -0,0 +1,43 @@ +// A forEach callback that deletes already-visited entries and then performs an +// array-like indexed read on the collection (`set[0]` / `map[0]` — Perry's +// live-index accessor, which squeezes tombstones so raw index == live index) +// must not shift the raw layout under the walk's own counter: every entry not +// deleted before its visit is visited exactly once. Node has no array-like +// read on collections (it yields undefined), so only visit ORDER is printed. +function walkSet(): string { + const s = new Set(); + for (let i = 0; i < 20; i++) s.add(i); + const seen: number[] = []; + s.forEach((v) => { + seen.push(v); + if (v === 5) { s.delete(1); s.delete(2); void (s as any)[0]; void (s as any)[1]; } + }); + return `set ${seen.length} ${seen.join(",")} size=${s.size}`; +} +function walkMap(): string { + const m = new Map(); + for (let i = 0; i < 20; i++) m.set(i, i * 10); + const seen: number[] = []; + m.forEach((_v, k) => { + seen.push(k); + if (k === 5) { m.delete(1); m.delete(2); void (m as any)[0]; void (m as any)[1]; } + }); + return `map ${seen.length} ${seen.join(",")} size=${m.size}`; +} +// Nested walks: the inner walk's read must not squeeze under the outer walk. +function nested(): string { + const m = new Map(); + for (let i = 0; i < 12; i++) m.set(i, i); + const outer: number[] = []; + m.forEach((_v, k) => { + outer.push(k); + if (k === 4) { + m.delete(0); + m.forEach((_iv, ik) => { if (ik === 6) void (m as any)[0]; }); + } + }); + return `nested ${outer.length} ${outer.join(",")}`; +} +console.log(walkSet()); +console.log(walkMap()); +console.log(nested()); From 06e443d53d7551bd5e547ca4b098f211f7f96aab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 21:06:40 +0200 Subject: [PATCH 2/2] changelog: key the fragment on #9561 --- ...foreach-live-index-read.md => 9561-foreach-live-index-read.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{0000-foreach-live-index-read.md => 9561-foreach-live-index-read.md} (100%) diff --git a/changelog.d/0000-foreach-live-index-read.md b/changelog.d/9561-foreach-live-index-read.md similarity index 100% rename from changelog.d/0000-foreach-live-index-read.md rename to changelog.d/9561-foreach-live-index-read.md