perf(runtime): index object/bigint Map keys, single-pass timer drain (#6084) - #6285
Merged
Conversation
added 4 commits
July 11, 2026 10:13
js_timer_tick and js_callback_timer_tick removed expired entries with Vec::remove(i) inside an index scan, shifting the whole tail once per expired timer -- O(n^2) on bursts of same-deadline timers. Replace both with one shared drain_expired_timers helper: a single-pass stable partition (mem::take + repartition) that preserves creation order for the expired batch (same-deadline timers fire in scheduling order, Node semantics) and queue order for survivors. Cleared callback timers are still discarded in the same pass. Deliberately minimal: the BinaryHeap redesign from #6084 is a separate follow-up; the TIMER_REF_STATES bounded-eviction logic (#6257) is untouched. Part of #6084.
Map's O(1) side-tables covered only bits-stable numeric keys and content-hashed strings; every object, symbol, closure and bigint key fell through to a full linear scan of the entries buffer, so get/set/has/delete on an object-keyed Map were O(n) each and object-keyed workloads O(n^2). Object-keyed Maps are the default cache/registry idiom -- measured 1,793x slower than string keys (20k insert+get: 7,173 ms vs 4 ms). Add MAP_PTR_INDEX, a third per-Map side-table keyed by MapPtrKey: objects, symbols and closures hash and compare by their raw NaN-box bits (identity, matching the linear scan's bit-equality), bigints by limb CONTENT. The stored key bits go stale whenever the generational GC evacuates a pointee, so the index is refreshed exactly like Set's SET_INDEX: a new GcRewriteHookKind::MapIndex rebuilds it from the (already rewritten) entries buffer. The three rewrite call sites (remembered-set dirty scan, copying field scan, verify/force-evacuate rewrite) each open-coded a `== GcRewriteHookKind::SetIndex` check, so route them all through one shared run_gc_rewrite_hook dispatch rather than adding a second special case; map_header_moved_for_gc migrates the outer key when the MapHeader itself moves, and every mutation path (set, delete, clear, alloc, finalize) keeps the table exact so a miss is definitive. Also fixes a latent correctness bug: bigint keys previously passed is_safe_numeric_key (the old comment misidentified BIGINT_TAG as 0x7FFE, which is INT32_TAG -- bigints are 0x7FFA) and so were indexed by raw pointer bits. `m.set(1n); m.get(1n)` with two distinct 1n allocations missed, and the indexed bits went stale across a GC move. jsvalue_eq now compares bigints by mathematical value per SameValueZero (23.1.3.9). Part of #6084.
…t content keys test_copying_minor_rebuilds_map_pointer_key_index is the make-or-break case for MAP_PTR_INDEX: an object key is evacuated by a copying minor (along with the MapHeader itself), so every bit pattern stored in the index goes stale. Asserts both objects actually moved, that the rebuilt index resolves the key at its NEW address, that numeric keys survive the rebuild, and that the stale pre-GC bits do NOT resolve. Verified to have teeth: stubbing out the GcRewriteHookKind::MapIndex dispatch makes it fail. test_map_bigint_keys_match_by_content_not_identity pins the SameValueZero semantics -- two distinct 1234n allocations are the same Map key, and re-setting through the content-equal allocation overwrites rather than appends. Both compare undefined by BITS, not as an f64: undefined is a NaN payload and NaN != NaN, so assert_eq! on the f64 can never pass. Part of #6084.
test_gc_type_metadata_covers_all_declared_types mirrors every declared GC type's metadata so that adding a hook is a conscious, reviewed change. Record map's new rewrite_hook_kind: MapIndex alongside set's SetIndex. Part of #6084.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds GC-safe Map pointer-key indexing with BigInt content equality and centralized rewrite-hook dispatch. It updates copying and verification paths, adds Map GC regression tests, and replaces timer queue removal loops with stable expired-timer partitioning for promise and callback timers. ChangesMap GC Indexing
Timer Queue Processing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CopyingNurseryCollector
participant run_gc_rewrite_hook
participant rebuild_map_ptr_index_for_gc
participant MAP_PTR_INDEX
CopyingNurseryCollector->>run_gc_rewrite_hook: dispatch MapIndex after rewritten slots
run_gc_rewrite_hook->>rebuild_map_ptr_index_for_gc: rebuild pointer-key index
rebuild_map_ptr_index_for_gc->>MAP_PTR_INDEX: store rewritten key entries
sequenceDiagram
participant TimerQueue
participant drain_expired_timers
participant order_expired_callback_batch
participant Callbacks
TimerQueue->>drain_expired_timers: provide expired and cleared entries
drain_expired_timers->>TimerQueue: preserve surviving entries
drain_expired_timers->>order_expired_callback_batch: return expired callbacks
order_expired_callback_batch->>Callbacks: invoke deadline-ordered timers and FIFO immediates
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/tests/copying.rs (1)
311-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the indexed BigInt lookup path.
With one entry,
find_key_indexuses the small-map linear scan, so this test can pass even ifMAP_PTR_INDEXBigInt hashing/equality is broken. Add enough filler entries to exceed the eight-entry threshold.Proposed test adjustment
let map = crate::map::js_map_alloc(8); + for i in 0..9 { + crate::map::js_map_set(map, i as f64, i as f64); + } let boxed = |v: i64| crate::value::js_nanbox_bigint(crate::bigint::js_bigint_from_i64(v) as i64); @@ crate::map::js_map_set(map, a, 42.0); + assert!(crate::map::test_map_ptr_index_contains(map, b)); @@ - 1, + 10,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/gc/tests/copying.rs` around lines 311 - 349, Update test_map_bigint_keys_match_by_content_not_identity to insert enough distinct filler keys after the initial map setup to exceed the eight-entry threshold and force the indexed MAP_PTR_INDEX lookup path. Adjust assertions to account for the filler entries while preserving verification that content-equal BigInt keys overwrite and different values miss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-runtime/src/gc/tests/copying.rs`:
- Around line 311-349: Update test_map_bigint_keys_match_by_content_not_identity
to insert enough distinct filler keys after the initial map setup to exceed the
eight-entry threshold and force the indexed MAP_PTR_INDEX lookup path. Adjust
assertions to account for the filler entries while preserving verification that
content-equal BigInt keys overwrite and different values miss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55236e9e-c7a8-4f4a-bfc0-436a9553f61b
📒 Files selected for processing (8)
crates/perry-runtime/src/gc/barrier.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/tests/alloc.rscrates/perry-runtime/src/gc/tests/copying.rscrates/perry-runtime/src/gc/types.rscrates/perry-runtime/src/gc/verify.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/timer.rs
…rder (#6287) (#6289) * fix(timer): fire an expired batch in event-loop order, not creation order When several timers come due in the same turn, Perry fired them in queue (creation) order. That diverged from Node on two counts: 1. Deadline order. Node's timers phase walks lists by expiry, so a 5 ms timer created AFTER a 10 ms one still fires first. Perry ran them in creation order: setTimeout(() => log("late10"), 10); setTimeout(() => log("reffed"), 5); node: reffed, late10 perry: late10, reffed 2. Timers before immediates. setImmediate runs in the CHECK phase, after the timers phase, so an expired setTimeout fires ahead of an immediate that was scheduled earlier. Perry interleaved both kinds in one creation-ordered queue: setImmediate(() => log("IMM1")); setTimeout(() => log("T5"), 5); // loop blocked past 5 ms node: T5, IMM1 perry: IMM1, T5 Order the expired callback batch as (timeouts by deadline) then (immediates in FIFO), and the expired promise-timer batch by deadline. Both sorts are STABLE, which is what preserves the two orderings Perry already had right: same-deadline timers still fire in creation order, and immediates still fire in scheduling order. Node's ordering was captured as ground truth from `node --experimental-strip-types` (deterministic across runs) rather than inferred, including the immediate-vs-timeout case that rules out a naive sort by deadline alone: immediates carry a ~now deadline and would otherwise sort ahead of an expired timeout. Fixes #6287. * test(timer): cover expired-batch event-loop ordering (#6287) Unit tests for order_expired_callback_batch: deadline order for expired timeouts, creation order preserved for same-deadline ties (the stable-sort guarantee), and timeouts-before-immediates with immediates staying FIFO -- the case that rules out a naive sort by deadline alone, since an immediate carries a ~now deadline and would otherwise sort ahead of an expired timeout. Plus a gap test (test_gap_6287_timer_batch_order.ts) that blocks the loop past every deadline so the whole batch comes due in one turn, making the ordering observable. Byte-identical to node across repeated runs, and the blocking makes it deterministic rather than timing-dependent. Part of #6287. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/timer.rs (1)
1122-1137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent same-batch
clearTimeoutfrom firing a later expired timer
drain_expired_timerssnapshotsCALLBACK_TIMERSbefore any callback runs, so a callback that clears another timer in the same expired batch can’t mark that snapshot entry.if !timer.clearedon the drained copy will still pass and the cleared timer fires anyway. Re-check against live shared state or keep canceled state outside the drained batch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/timer.rs` around lines 1122 - 1137, Update the expired-timer processing around drain_expired_timers so each timer is revalidated against live CALLBACK_TIMERS state immediately before invocation. Ensure a callback’s clearTimeout call marks later timers as canceled and prevents them from firing in the same batch, rather than relying on the drained timer copy’s cleared field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry-runtime/src/timer.rs`:
- Around line 1122-1137: Update the expired-timer processing around
drain_expired_timers so each timer is revalidated against live CALLBACK_TIMERS
state immediately before invocation. Ensure a callback’s clearTimeout call marks
later timers as canceled and prevents them from firing in the same batch, rather
than relying on the drained timer copy’s cleared field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53bcd78f-1e32-4b11-9bdb-369fff9ebc7f
📒 Files selected for processing (2)
crates/perry-runtime/src/timer.rstest-files/test_gap_6287_timer_batch_order.ts
added 2 commits
July 11, 2026 15:50
…odules The 2000-line file gate (scripts/check_file_size.sh) failed the lint job: timer.rs was 1998 lines on main -- two under the cap -- so the drain partition, the expired-batch ordering, and their tests pushed it to 2163, and the new Map pointer-index GC tests took gc/tests/copying.rs to 2040. Mechanical cut, no behavior change: - timer.rs: the whole trailing #[cfg(test)] region (test-only seed/snapshot helpers + the drain/ordering unit tests) moves to timer/tests.rs, next to the existing timer/ref_states.rs. The pub(crate) helpers are re-exported from timer.rs, so out-of-module call sites (crate::timer::test_seed_*, used by the gc root-scanner tests) resolve unchanged. 2163 -> 1913. - gc/tests/copying.rs: the two Map pointer-key index tests move to copying/map_pointer_index.rs, alongside the existing copying/ siblings. 2040 -> 1940. map.rs stays over the gate but is already allowlisted. cargo test -p perry-runtime: 1239 passed, 0 failed (unchanged). fmt, file-size, GC store-site inventory and address-classification gates all pass locally.
…ze gate This PR pushed both files past the cap (timer.rs 1998 -> 2163, copying.rs 1939 -> 2040), failing lint. Extract the incremental GC root-scan machinery into timer/gc_scan.rs (timer.rs is now 1947) and the side-table rewrite tests into gc/tests/copying_side_tables.rs (copying.rs is now 1817).
proggeramlug
added a commit
that referenced
this pull request
Jul 12, 2026
… bands (#6285 follow-up) (#6297) `main` is red: the addr-class ratchet fails on a clean checkout (`crates/perry-runtime/src/map.rs: 4 site(s), baseline allows 3`), so the lint gate blocks every PR. #6285 added two hand-rolled address floors without updating the baseline. The floors are also wrong, which is what the ratchet exists to catch. Both bare-address branches gate on `bits > 0x10000`: } else if bits >> 48 == 0 && bits > 0x10000 { // map_ptr_from_receiver_bits } else if upper == 0 && bits > 0x10000 { // bigint_ptr_from_bits but `HANDLE_BAND_MAX` is `0x100000` — an order of magnitude higher. So every fetch (`0x40000..0xE0000`), zlib and proxy handle clears that floor and is accepted as a candidate heap address. Both call sites happen to survive today because each re-checks before dereferencing (`is_registered_map` — an exact registry lookup; `try_read_gc_header` — the guarded reader), so this is a latent hazard rather than a live segfault. But it is exactly the shape that segfaults on Linux once a caller trusts the address, which is the #1843 / #4004 / #4665 / #4800 / #6271 family, and macOS's 2 TB heap floor hides it. Use the sanctioned predicate — `value::addr_class::is_above_handle_band` — as the floor instead. Strictly narrower: the only addresses it newly rejects are `[0x10001, 0x100000)`, i.e. precisely the handle bands, which must never be read as a Map or BigInt header. Real arena allocations are already above `HANDLE_BAND_MAX`. Drops map.rs from 4 handle-floor sites to 2 (baseline allows 3), so the ratchet passes and main goes green again. perry-runtime lib suite 1264/1264. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
Summary
Two of the remaining #6084 items, plus a correctness bug found while fixing them. Continues the incremental, independently-mergeable approach from #6126 (nextTick
VecDeque) and #6257 (TIMER_REF_STATESleak bound).1. Object-keyed
Mapwas O(n) per operation → O(n²) workloadsMap's O(1) side-tables covered only bits-stable numeric keys and content-hashed strings. Every object, symbol, closure and bigint key fell through to a full linear scan of the entries buffer, soget/set/has/deleteon an object-keyedMapwere each O(n). Object-keyed Maps are the default cache/registry idiom.Added
MAP_PTR_INDEX, a third per-Map side-table keyed byMapPtrKey: objects/symbols/closures hash and compare by their raw NaN-box bits (identity — matching the linear scan's existing bit-equality), bigints by limb content.Measured (20k inserts + 20k gets, same machine,
perry-dev):mainhas×20k~1,900× on insert+get, matching the 1,793× the audit measured.
GC safety (the crux)
Stored key bits go stale whenever the generational GC evacuates a pointee, so the index is refreshed exactly the way Set's
SET_INDEXalready is: a newGcRewriteHookKind::MapIndexrebuilds it from the (already rewritten) entries buffer.The three rewrite call sites — remembered-set dirty scan, copying field scan, and verify/force-evacuate rewrite — each open-coded a
== GcRewriteHookKind::SetIndexcomparison. Rather than adding a second special case to each, they now route through one sharedrun_gc_rewrite_hookdispatch, so a future hook kind needs wiring in exactly one place.map_header_moved_for_gcmigrates the outer key when theMapHeaderitself moves, and every mutation path (set, delete, clear, alloc, finalize) keeps the table exact — which is what lets a lookup miss be treated as definitive rather than falling back to a scan.2. Correctness bug: bigint
Mapkeys compared by identity, not valueBigints previously passed
is_safe_numeric_key(the old comment misidentifiedBIGINT_TAGas0x7FFE, which is actuallyINT32_TAG— bigints are0x7FFA) and so were indexed by raw pointer bits. Consequences onmaintoday:…and the indexed bits went stale across a GC move.
jsvalue_eqnow compares bigints by mathematical value per SameValueZero (23.1.3.9), and they are routed through the pointer index (content-hashed).3. Timer drain:
Vec::remove(i)inside a scan → single-pass partitionjs_timer_tick/js_callback_timer_tickremoved expired entries withqueue.remove(i)inside an index scan, shifting the whole tail once per expired timer — O(n²) on bursts of same-deadline timers. Both now share onedrain_expired_timershelper: a single-pass stable partition that preserves the exact ordering semantics of the old code (expired batch in creation order, survivors in queue order; cleared callback timers discarded in the same pass). Verified order-identical by A/B against base. TheTIMER_REF_STATESbounded-eviction logic from #6257 is untouched.Verification
cargo test -p perry-runtime: 1236 passed, 0 failed.cargo fmt --all -- --checkclean.test_copying_minor_rebuilds_map_pointer_key_index— the make-or-break case: an object key and the MapHeader are both evacuated by a copying minor, so every bit stored in the index is stale. Asserts both objects actually moved, the rebuilt index resolves the key at its new address, numeric keys survive the rebuild, and the stale pre-GC bits do not resolve. Verified to have teeth: stubbing out theMapIndexdispatch makes it fail.test_map_bigint_keys_match_by_content_not_identity— SameValueZero for bigint keys; re-setting through a content-equal allocation overwrites rather than appends.node --experimental-strip-types— byte-identical: object-key identity vs structural equality, delete/re-insert iteration order, symbol and function keys,NaN/-0/+0SameValueZero edges, bigint content equality incl.2n**70n, bigint-vs-number non-equality, and a 500-key mixed-kind map (object/string/number/bigint) with allocation churn interleaved.PERRY_GC_FORCE_EVACUATE=1,PERRY_GC_VERIFY_EVACUATION=1, and both together.Scope / follow-ups
Left for separate PRs (disjoint files, each independently mergeable):
Promise.all/PROMISE_SETTLE_LISTENERS/PROMISE_OVERFLOW_REACTIONSwhole-table scans on every settle (item 2) — needs the same GC-rekeying treatment as this index, so it gets its own PR rather than riding along.PROMISE_CONTEXTS/ AsyncLocalStorage churn (item 4).GLOBAL_DESCRIPTORS_IN_USEwrite-path gate (item 6).BinaryHeapredesign (item 3's larger half).Separately found, pre-existing, not fixed here: Perry fires an expired timer batch in creation order, whereas Node fires by deadline — so a 5 ms timer created after a 10 ms one fires second. Confirmed present on base
main(identical output withtimer.rsreverted), so it is not a regression from this PR; theBinaryHeapredesign is the natural place to fix it.Part of #6084.
Summary by CodeRabbit
Bug Fixes
Maplookups after GC by rebuilding the appropriate key lookup state after object field rewrites.Mapkeys so moved/rewritten keys remain resolvable.MapBigInt key matching to use value/content equality (not allocation identity).Tests
Mappointer-key and BigInt behaviors across minor copying GC.