fix(runtime,hir,codegen): Map/Set mutation during for…of is O(1) per step and never skips an entry

changelog.d/9513-map-set-iteration-compaction.md

@@ -0,0 +1,62 @@
+**Map/Set: mutation during `for…of` is O(1) per step again, and never skips an entry.**
+
+`#9020` made ordered `delete` O(1) by tombstoning entries in place, and moved
+the squeeze that used to happen per delete into the raw-index *readers*
+(`js_map_entry_key_at` / `js_set_value_at`) as a "self-heal": the first raw
+read that saw a hole compacted the whole collection. The `for…of` fast path
+reads raw slots on every step, so a loop that deletes while iterating paid one
+full compaction per delete — 50,000 entries with 12,500 deletes inside the walk
+took 13–32 s against Node's 0.07 s (190–460×), quadratic in the collection
+size; the identical deletes with no iterator open cost 0.5 s.
+
+Worse, every raw-index cursor (the fast path, the iterator objects) recovered
+from a squeeze by re-finding the last returned key and, when that key was
+itself deleted, reading `cursor-1` — which assumed exactly ONE hole had been
+squeezed. Deleting several already-visited entries plus the current one in a
+single loop body skipped entries, and enough holes ended the loop early
+(40 entries, 21 deleted at the 21st: Perry visited 21, Node 40).
+
+Fixed the way V8 transitions its ordered-hash-table iterators: every squeeze
+(`compact_*`, and `clear` while a walk may be open) records which raw indices
+it removed, in a per-collection log, and bumps a `compaction_epoch` in the
+header. A cursor carries the epoch it last synchronised with; one runtime call
+per step (`js_map_cursor_next` / `js_set_cursor_next`) rebases it — down by
+exactly the removed count below it, in order, through every record since —
+then steps over tombstones and returns the next live raw index. This is exact
+by the walk's own invariant (the yielded entries are precisely the live ones
+below the cursor), needs no key lookup, no "iteration active" registration
+that a `break`, `return` or abandoned generator could leak, and the walkers'
+reads no longer compact at all. The codegen inline entry read is bounded by
+the raw extent instead of requiring a dense buffer.
+
+Two reader contracts, kept apart. `js_map_entry_key_at` / `js_map_entry_value_at`
+/ `js_set_value_at` remain the LIVE-index accessors #9504 made the array-like
+`map[i]` / `set[i]` read, `console.table` and collection equality go through:
+they squeeze tombstones first so raw index == live index, never hand out a
+hole, and now record that squeeze — so a `for…of` cursor open on the same
+collection rebases exactly instead of skipping. The walkers use new RAW twins
+(`js_map_entry_key_raw_at`, `js_map_entry_value_raw_at`, `js_set_value_raw_at`),
+bounded by the raw extent, which never compact; only the cursor ever reads
+them, and it only yields live raw indices. The iterator objects use the same
+rebase (their field 3 now holds the epoch). History is retained under a budget
+of removed raw indices — `max(4096, capacity)` per collection — never a record
+count, because a delete+re-add pair on a collection at full capacity squeezes
+one hole per pair on the grow path and a single loop body can force dozens of
+those. A `clear()` record supersedes everything before it and truncates it.
+Exceeding the budget therefore takes one loop body deleting more than the
+collection's whole capacity between two of its own reads; a cursor trimmed out
+of its history is stepped over holes without the lost rebase, and the bound is
+pinned by a forty-squeezes-in-one-body test for Map and Set.
+
+Also correct now: `clear()` inside a walk restarts the cursor at 0, so entries
+added afterwards are visited, as the spec's in-place emptying of `[[MapData]]`
+requires.
+
+Tests: runtime unit tests for both reader contracts (raw twins never compact;
+live-index accessors squeeze, record, and a cursor past the hole rebases), the
+exact multi-hole rebase, successive squeezes + `clear`, forty squeezes in one
+body at full capacity, `clear` truncating the history, and address reuse (Map
+and Set); #9504's `a_tombstoned_collection_never_hands_a_hole_to_an_indexed_read`
+stays green; `test_gap_map_set_multi_delete_during_iteration.ts` covers the fast path,
+the iterator objects, re-add, `clear`, and a 50k-entry churn, all
+Node-differential.

crates/perry-codegen/src/expr/arrays_finds.rs

@@ -91,8 +91,8 @@ 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.
+/// Byte offset of `MapHeader::used`, the raw extent the inline entry read is
+/// bounded by (the for-of cursor only ever yields a live raw index below it).
 ///
 /// `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
@@ -156,20 +156,19 @@ fn lower_map_entry_at_inline(
         let gc_flags = blk.load(I8, &flags_ptr);
         let forwarded = blk.and(I8, &gc_flags, GC_FLAG_FORWARDED);
         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).
+        // The index is a LIVE raw index by construction: it comes from
+        // `js_map_cursor_next`, and the for-of desugars in perry-hir are the
+        // only producers of these reads. So the read is bounded by the raw
+        // extent `used`, not the live `size`, and holes need no admission
+        // check — the cursor already stepped over them. (This lane used to
+        // require `used == size` and fall back to a runtime helper that
+        // compacted the whole map on every holey read.)
         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 in_range = blk.icmp_ult(I32, &i_i32, &used);
         let a = blk.and(I1, &is_map, &live);
-        let b = blk.and(I1, &a, &dense);
-        let admitted = blk.and(I1, &b, &in_range);
+        let admitted = blk.and(I1, &a, &in_range);
         blk.cond_br(&admitted, &fast_label, &slow_label);
     }
     ctx.current_block = fast_idx;
@@ -595,8 +594,14 @@ pub(crate) fn lower(
         // calling `js_map_entries` (which materializes N+1 small Arrays).
         Expr::MapEntryKeyAt { map, idx } | Expr::MapEntryValueAt { map, idx } => {
             let (runtime_fn, value_slot) = match expr {
-                Expr::MapEntryKeyAt { .. } => ("js_map_entry_key_at", false),
-                Expr::MapEntryValueAt { .. } => ("js_map_entry_value_at", true),
+                // The RAW twins: these nodes are produced only by the for-of
+                // desugars, whose cursor yields live raw indices, so the
+                // fallback must read raw slots bounded by `used` and never
+                // compact. (`js_map_entry_key_at` without `_raw` is the
+                // live-index accessor the array-like `map[i]` read uses,
+                // which compacts — #9504.)
+                Expr::MapEntryKeyAt { .. } => ("js_map_entry_key_raw_at", false),
+                Expr::MapEntryValueAt { .. } => ("js_map_entry_value_raw_at", true),
                 _ => unreachable!(),
             };
             rooting::with_operands_rooted(ctx, &[map, idx], |ctx, vals| {
@@ -619,7 +624,7 @@ pub(crate) fn lower(
                 let i_i32 = blk.fptosi(DOUBLE, &i_dbl, I32);
                 Ok(blk.call(
                     DOUBLE,
-                    "js_set_value_at",
+                    "js_set_value_raw_at",
                     &[(I64, &s_handle), (I32, &i_i32)],
                 ))
             })

crates/perry-codegen/src/runtime_decls/strings.rs

@@ -1235,10 +1235,22 @@ pub fn declare_phase_b_strings(module: &mut LlModule) {
     // would do. (Map ptr, entry idx) → key / value.
     module.declare_function("js_map_entry_key_at", DOUBLE, &[I64, I32]);
     module.declare_function("js_map_entry_value_at", DOUBLE, &[I64, I32]);
+    // RAW twins for the for-of walkers: bounded by the raw extent, never
+    // compact (the un-suffixed accessors above are live-index and compact).
+    module.declare_function("js_map_entry_key_raw_at", DOUBLE, &[I64, I32]);
+    module.declare_function("js_map_entry_value_raw_at", DOUBLE, &[I64, I32]);
+    module.declare_function("js_set_value_raw_at", DOUBLE, &[I64, I32]);
     // #6075: current index of a key/value (or -1) for the delete-safe for-of
     // fast path. Takes the NaN-boxed collection + key; strips internally.
     module.declare_function("js_map_find_key_index", DOUBLE, &[DOUBLE, DOUBLE]);
     module.declare_function("js_set_find_value_index", DOUBLE, &[DOUBLE, DOUBLE]);
+    // The delete-safe for-of cursor: (NaN-boxed collection, cursor, epoch) →
+    // next live raw index or -1, and the collection's compaction epoch the
+    // loop stores after each step. See perry-hir `map_set_delete_safe_for_of`.
+    module.declare_function("js_map_cursor_next", DOUBLE, &[DOUBLE, DOUBLE, DOUBLE]);
+    module.declare_function("js_map_compaction_epoch", DOUBLE, &[DOUBLE]);
+    module.declare_function("js_set_cursor_next", DOUBLE, &[DOUBLE, DOUBLE, DOUBLE]);
+    module.declare_function("js_set_compaction_epoch", DOUBLE, &[DOUBLE]);
     // Map/Set forEach: (collection_ptr, callback_nanboxed_f64, thisArg_f64) -> void (#2830)
     module.declare_function("js_map_foreach", VOID, &[I64, DOUBLE, DOUBLE]);
     module.declare_function("js_set_foreach", VOID, &[I64, DOUBLE, DOUBLE]);

crates/perry-hir/src/lower/for_head.rs

@@ -518,67 +518,50 @@ pub(crate) fn rewrite_collection_view_for_of(
     })
 }
 
-/// Build the delete-safe control for the Map/Set `for-of` fast path (#6075).
+/// Build the cursor control for the Map/Set `for-of` fast path.
 ///
-/// The fast path iterates the backing entries array by index (`arr_id` holds the
-/// collection, `idx_id` the cursor). But `delete` compacts that array (entries
-/// after the hole shift down), so deleting an entry at index ≤ the cursor moves
-/// an unvisited entry below the cursor and skips it. This re-derives the read
-/// index each iteration from the last-returned key, so a shift can't skip.
+/// The fast path walks the backing entries buffer by RAW index (`arr_id`
+/// holds the collection, `idx_id` the cursor). Two things can happen to the
+/// raw layout while the body runs:
+///
+/// * a `delete` tombstones an entry in place (#9020) — raw indices are
+///   stable, the cursor only has to step over the hole;
+/// * a squeeze (`delete` past the tombstone threshold, `set` at capacity,
+///   `clear`) moves survivors to lower raw indices — the cursor must move
+///   down by exactly the number of removed slots below it.
+///
+/// Both are answered by ONE runtime call per step, `js_map_cursor_next` /
+/// `js_set_cursor_next`: it rebases the cursor through the collection's
+/// compaction log (every squeeze since the epoch the loop last recorded) and
+/// returns the next live raw index, or `-1` when the extent is exhausted.
+/// The loop keeps the epoch it last synchronised with in a state temp. Cost
+/// per step is O(holes stepped over); nothing here is O(n), and nothing here
+/// compacts — the reader-side "self-heal" compaction that preceded this made
+/// a delete-during-`for…of` loop O(n) per delete and, when more than one
+/// hole was squeezed at once, its `cursor-1` recovery skipped entries.
 ///
 /// Returns `(init_lets, condition, body_prefix)`:
 /// - `init_lets` — declare the state temps; push BEFORE the `for`.
-/// - `condition` — replaces `idx < size`. Overwrites `idx_id` with the corrected
-///   read index (plain cursor while the previously-read key is still at
-///   `cursor-1`; else locate the last key — `+1` after it, or into its vacated
-///   slot if it was itself deleted) and yields whether that index is in range.
-/// - `body_prefix` — prepend to the loop body (runs after the in-range check):
-///   records the visited key for the next iteration's in-place check.
-///
-/// `find` is O(1) for numeric/string keys and only called when a shift is
-/// detected, so normal / append-only iteration keeps the plain cursor path.
+/// - `condition` — replaces `idx < size`: rebases + advances `idx_id` and
+///   yields whether an entry is available.
+/// - `body_prefix` — empty; kept so both desugars keep their shape.
 pub(crate) fn map_set_delete_safe_for_of(
     ctx: &mut LoweringContext,
     arr_id: LocalId,
     idx_id: LocalId,
     is_set: bool,
 ) -> (Vec<Stmt>, Expr, Vec<Stmt>) {
-    let lk_id = ctx.fresh_local(); // last-returned key
-    let sz_id = ctx.fresh_local(); // current size (spilled once per iteration)
-    let fk_id = ctx.fresh_local(); // find() result
-
-    // `.size` on a Map/Set-typed receiver is codegen-recognized and lowered to
-    // js_map_size / js_set_size (the raw `MapSize`/`SetSize` nodes are not
-    // codegen expressions), matching the original loop bound.
-    let size_of = |a: Expr| -> Expr {
-        Expr::PropertyGet {
-            byte_offset: 0,
-            object: Box::new(a),
-            property: "size".to_string(),
-        }
-    };
-    let key_at = |a: Expr, i: Expr| -> Expr {
-        if is_set {
-            Expr::SetValueAt {
-                set: Box::new(a),
-                idx: Box::new(i),
-            }
-        } else {
-            Expr::MapEntryKeyAt {
-                map: Box::new(a),
-                idx: Box::new(i),
-            }
-        }
-    };
-    let find_fn = if is_set {
-        "js_set_find_value_index"
+    let ep_id = ctx.fresh_local(); // compaction epoch last synchronised with
+    let nx_id = ctx.fresh_local(); // js_*_cursor_next result
+    let (next_fn, epoch_fn) = if is_set {
+        ("js_set_cursor_next", "js_set_compaction_epoch")
     } else {
-        "js_map_find_key_index"
+        ("js_map_cursor_next", "js_map_compaction_epoch")
     };
-    let find_call = |args: Vec<Expr>| -> Expr {
+    let extern_call = |name: &str, args: Vec<Expr>| -> Expr {
         Expr::Call {
             callee: Box::new(Expr::ExternFuncRef {
-                name: find_fn.to_string(),
+                name: name.to_string(),
                 param_types: Vec::new(),
                 return_type: Type::Number,
             }),
@@ -587,110 +570,40 @@ pub(crate) fn map_set_delete_safe_for_of(
             byte_offset: 0,
         }
     };
-    let cmp = |op: CompareOp, l: Expr, r: Expr| -> Expr {
-        Expr::Compare {
-            op,
-            left: Box::new(l),
-            right: Box::new(r),
-        }
-    };
-
-    // cursor-1 (index of the entry read on the previous iteration).
-    let prev_idx = |i: Expr| Expr::Binary {
-        op: BinaryOp::Sub,
-        left: Box::new(i),
-        right: Box::new(Expr::Number(1.0)),
-    };
-
-    // read_idx = cursor == 0                              // not started
-    //            ? cursor
-    //            : key_at(coll, cursor-1) === last_key    // last entry still in place
-    //              ? cursor                               // no shift at/below cursor
-    //              : (j = find(coll, last_key)) >= 0 ? j + 1 : cursor - 1
-    //
-    // Comparing the entry now at cursor-1 to the last-returned key detects ANY
-    // delete that compacted an entry at/below the cursor — including a delete
-    // balanced by an add in the same turn (which leaves `size` unchanged), which
-    // a size-only gate would miss. Map/Set keys are unique under SameValueZero,
-    // so `===` here is exact except for a NaN key (which just forces the `find`
-    // path — still correct).
-    let rederive = Expr::Conditional {
-        condition: Box::new(cmp(
-            CompareOp::Eq,
-            Expr::LocalGet(idx_id),
-            Expr::Number(0.0),
-        )),
-        then_expr: Box::new(Expr::LocalGet(idx_id)),
-        else_expr: Box::new(Expr::Conditional {
-            condition: Box::new(cmp(
-                CompareOp::Eq,
-                key_at(Expr::LocalGet(arr_id), prev_idx(Expr::LocalGet(idx_id))),
-                Expr::LocalGet(lk_id),
-            )),
-            then_expr: Box::new(Expr::LocalGet(idx_id)),
-            else_expr: Box::new(Expr::Sequence(vec![
-                Expr::LocalSet(
-                    fk_id,
-                    Box::new(find_call(vec![
-                        Expr::LocalGet(arr_id),
-                        Expr::LocalGet(lk_id),
-                    ])),
-                ),
-                Expr::Conditional {
-                    // A delete only shifts entries down: a merely-shifted last key
-                    // is now below the cursor (`0 <= j < cursor`) → resume after
-                    // it. Deleted (`j < 0`) or deleted-then-re-added at the end
-                    // (`j >= cursor`) → read the entry now in its old slot
-                    // (`cursor - 1`).
-                    condition: Box::new(cmp(
-                        CompareOp::Ge,
-                        Expr::LocalGet(fk_id),
-                        Expr::Number(0.0),
-                    )),
-                    then_expr: Box::new(Expr::Conditional {
-                        condition: Box::new(cmp(
-                            CompareOp::Lt,
-                            Expr::LocalGet(fk_id),
-                            Expr::LocalGet(idx_id),
-                        )),
-                        then_expr: Box::new(Expr::Binary {
-                            op: BinaryOp::Add,
-                            left: Box::new(Expr::LocalGet(fk_id)),
-                            right: Box::new(Expr::Number(1.0)),
-                        }),
-                        else_expr: Box::new(prev_idx(Expr::LocalGet(idx_id))),
-                    }),
-                    else_expr: Box::new(prev_idx(Expr::LocalGet(idx_id))),
-                },
-            ])),
-        }),
-    };
-
+    // nx = cursor_next(coll, idx, ep); ep = epoch(coll); idx = nx; nx >= 0
     let condition = Expr::Sequence(vec![
-        Expr::LocalSet(sz_id, Box::new(size_of(Expr::LocalGet(arr_id)))),
-        Expr::LocalSet(idx_id, Box::new(rederive)),
-        cmp(CompareOp::Lt, Expr::LocalGet(idx_id), Expr::LocalGet(sz_id)),
+        Expr::LocalSet(
+            nx_id,
+            Box::new(extern_call(
+                next_fn,
+                vec![
+                    Expr::LocalGet(arr_id),
+                    Expr::LocalGet(idx_id),
+                    Expr::LocalGet(ep_id),
+                ],
+            )),
+        ),
+        Expr::LocalSet(
+            ep_id,
+            Box::new(extern_call(epoch_fn, vec![Expr::LocalGet(arr_id)])),
+        ),
+        Expr::LocalSet(idx_id, Box::new(Expr::LocalGet(nx_id))),
+        Expr::Compare {
+            op: CompareOp::Ge,
+            left: Box::new(Expr::LocalGet(nx_id)),
+            right: Box::new(Expr::Number(0.0)),
+        },
     ]);
-
-    // Record the key just read so the next iteration can check it is still in
-    // place at cursor-1.
-    let body_prefix = vec![Stmt::Expr(Expr::LocalSet(
-        lk_id,
-        Box::new(key_at(Expr::LocalGet(arr_id), Expr::LocalGet(idx_id))),
-    ))];
-
-    let mk_let = |id: LocalId, tag: &str, ty: Type, init: Expr| Stmt::Let {
+    let mk_let = |id: LocalId, tag: &str, init: Expr| Stmt::Let {
         id,
         name: format!("__miter_{}_{}", tag, id),
-        ty,
+        ty: Type::Number,
         mutable: true,
         init: Some(init),
     };
     let init_lets = vec![
-        mk_let(lk_id, "lk", Type::Any, Expr::Undefined),
-        mk_let(sz_id, "sz", Type::Number, Expr::Number(0.0)),
-        mk_let(fk_id, "fk", Type::Number, Expr::Number(0.0)),
+        mk_let(ep_id, "ep", Expr::Number(0.0)),
+        mk_let(nx_id, "nx", Expr::Number(0.0)),
     ];
-
-    (init_lets, condition, body_prefix)
+    (init_lets, condition, Vec::new())
 }