Merge train: #9513 (Map/Set O(1) iteration compaction) - #9547
Merged
Conversation
added 4 commits
September 2, 2026 17:14
…step and never skips an entry #9020 made ordered Map/Set deletes O(1) by tombstoning entries in place and moved the squeeze that used to run 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 observed 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 (quadratic in n), while the identical deletes with no iterator open cost 0.5 s. It was also a correctness bug. Every raw-index cursor (the fast path and 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 assumes exactly ONE hole was squeezed. Deleting several already visited entries plus the current one in a single body skipped entries, and enough holes ended the loop early: 40 entries, k0..k20 deleted at k20, perry visited 21, node 40. Map and Set, both walkers. Fixed the way V8 transitions ordered-hash-table iterators. Every squeeze (`compact_*`, and `clear` while a walk may be open) records the raw indices it removed in a per-collection log and bumps a `compaction_epoch` in the header (offset 36 for MapHeader, 28 for SetHeader — both in existing padding, so no other offset or the codegen-pinned `used` moves). A cursor carries the epoch it last synchronised with; one 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 entries below the cursor), needs no key lookup and no "iteration active" registration that a break, return or abandoned generator could leak, and the readers no longer compact at all. The codegen inline entry read is bounded by the raw extent instead of requiring a dense buffer; the iterator objects keep the epoch in their former size-sentinel field 3. `clear()` inside a walk is recorded as a prefix squeeze, so the cursor restarts at 0 and sees later appends, as the spec's in-place emptying requires. Log depth is bounded at 32 records per collection. Quiet-host numbers (node 26.5.1), warm: delete+re-add during for…of over 50k entries 13.38 s -> 0.02 s (mixed keys), 32.36 s -> 0.02 s (string keys); Set 8.14 s -> 0.02 s; the bench_map_set_tombstone_churn row from 152x node to ~5x faster than node. Every during-iteration variant now costs the same as its no-iterator control. Tests: runtime unit tests for the no-compaction read contract, the exact multi-hole rebase, successive squeezes + clear, and address reuse (Map and Set); the one test that pinned the old self-heal is rewritten to pin the new contract; `test_gap_map_set_multi_delete_during_iteration.ts` covers the fast path, the iterator objects, re-add, clear and a 50k churn, node-differential. Validation: perry-runtime 2979 passed / 0 failed (single-threaded, release); perry-hir and perry-codegen suites green; clippy --workspace 0 warnings; scripts/run_lint_gates.sh 60/60 (the two new perry_thread_local! holders are classified in gc_runtime_root_holders.json); force-evacuate / verify-evacuation / seeded schedule / from-space protect / VERIFY_MARK arms over the new fixture all match node.
…budget the squeeze history Rebased onto main. #9504 (merged after this branch's base) made the array-like `set[i]` / `map[i]` read go through `js_set_value_at` / `js_map_entry_key_at` and rely on their compaction to make raw index == live index — the exact compaction the first revision removed for the walkers, so `a_tombstoned_collection_never_hands_a_hole_to_an_indexed_read` failed on the merge. Two contracts wanted one function; they are now two: * `js_map_entry_key_at` / `js_map_entry_value_at` / `js_set_value_at` stay the LIVE-index accessors #9504 named them: squeeze first, never hand out a hole, and the squeeze is now recorded in the compaction log, so a for…of cursor open on the same collection rebases exactly instead of skipping. * the walkers read through 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, never compacting; the codegen MapEntryKeyAt / MapEntryValueAt / SetValueAt fallbacks name them. Review also caught that the 32-record history cap was the wrong shape: the grow-path squeeze (`ensure_capacity` at used == capacity with a hole) fires once per delete+re-add pair on a full collection, so one loop body reaches 33 records trivially. History is now budgeted by retained removed-index count, `max(4096, capacity)` per collection, a `clear()` record truncates everything before it, and a forty-squeezes-in-one-body test (Map and Set, at full capacity) pins exactness across the window. The version bump is dropped per the contributor rule. Validation on the rebased tree: perry-runtime 3012 passed / 0 failed (release, single-threaded) including #9504's test; Node-differential match on the new fixture, the existing Map/Set iteration fixture, and #9504's hole-leak-family fixtures; cargo clippy --workspace 0 warnings; cargo fmt --all --check clean.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughMap and Set iteration now uses compaction epochs and removal logs to rebase cursors across tombstone squeezes and ChangesMap and Set iteration repair
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands #9513 (Map/Set mutation during for…of is O(1) per step and never skips an entry — the compaction-epoch cursor protocol) rebased onto current main, plus one train fix: the
map_entry_atwalker IR proof now expects the #9513 raw twins (js_map_entry_key_raw_at/js_map_entry_value_raw_at) — the walker lowering moved to them while the live-index readers stay for direct user indexing.Validation: release build green; RUST_TEST_THREADS=1 perry-runtime 3014/0 — including
a_tombstoned_collection_never_hands_a_hole_to_an_indexed_read, the #9504 invariant this PR's earlier head broke (the updated head keeps the live-index readers, verified explicitly); perry-codegen + perry-hir 0 failed suites; gap fixture byte-identical to node; lint gates green.Rebase-merge preserving @proggeramlug's authorship on the three PR commits.
Summary by CodeRabbit
Bug Fixes
MapandSetiteration when entries are deleted, re-added, or cleared during a loop.clear().Tests