perf(gc,codegen): collect dead lazy JSON arrays and stop re-classifying indexed reads - #10136
proggeramlug wants to merge 6 commits into
Conversation
The brand test is the first thing every indexed read on an unknown receiver executes, and it computed the entire guard set before finding out the receiver was not a typed array: the element kind load, its range test, both index range checks and three ANDs. A JSON.parse array -- and any ordinary Array behind an erased receiver -- is not a typed array, so it paid all of that on every element read, forever, to reach a branch it was always going to take. Decide on the tag alone and leave. The kind and index guards only mean anything once the tag says typed array, so they move behind it into tav.get.kind_guard; the typed-array fast path reaches the same guard set by the same AND-reduction and is unchanged. Retired instructions per read, measured against the pre-#10114 compiler on the JSON access fixtures: -2.4% to -3.5% on all twelve rows. That also erases #10114's one disclosed cost -- the 20 MiB rows, which are ordinary Arrays above the lazy admission bound, go from +0.7..+1.8% against that reference to -0.4..-2.6%, i.e. below it.
A lazy cluster born old is never swept by a minor, so a DEAD one holds its
whole element graph live through the remembered set until a full collection.
On records_array_16k:scan that full collection never arrives -- the arena
rebaselines its own trigger 134M->268M->536M->1073M while old_in_use climbs
past 48 MB -- and every minor reports survival_permille=996,
copied_objects=0, freed_bytes=0. That is 205 MiB peak RSS against Node's
62 MiB.
GC_TYPE_LAZY_ARRAY was pinned for two reasons, both now removed the way
GC_TYPE_REGEXP removed its own:
- json_tape_store keys a tape by its owner's address, so a moved header
orphaned it. Added json_tape_store::owner_moved plus
GcMoveHookKind::LazyArrayTape, mirroring GcMoveHookKind::RegExpSideTables.
- the copying minor's flip runs no per-object finalize hook, so a header
dying young leaked its tape. Added
finalize_dead_copied_minor_from_space_lazy_tapes, the twin of the
sweep-entry collect_owners pass, wired into the flip beside
map/set/errors/regex and reported in the diag line.
With both present the type is movable and the cluster's generation is
decided by cache size -- decided ONCE, so #7546's rule that header, cache
and bitmap share a generation still holds. Large clusters stay old exactly
as before.
Four tests that asserted immovability were retargeted: the large-cluster one
still pins old-gen residency on size, and the handle tests now assert the
stronger property -- that the rooted handle resolves to wherever the
collector left the header, and that alloc_lazy_array returns the refreshed
address rather than the stale one. One test was itself holding a raw header
across a forced evacuation and faulting on the 0xDEADBEEFBAADF0DE poison
fill; it is rooted now.
Dead-owner tape release for a NURSERY header is not handled here -- a dead
nursery header can carry a stale GC_FLAG_MARKED from an earlier cycle, so
the full trace's dead-owner predicate, which assumed old-gen residency,
never reports it dead. That is mark-bit semantics rather than movability,
so it is the next commit rather than this one.
Completes the movability change. Two tape-release tests asserted that a full mark-sweep reclaims a dead owner, which was true only while every owner was old-gen: the old-gen sweep finalizes an unmarked payload directly. A nursery-resident owner is reclaimed by a MINOR instead, through the new finalize_dead_copied_minor_from_space_lazy_tapes, exactly as Map/Set/Error/ RegExp reclaim theirs. A full sweep leaves nursery mark bits to the minor, so asserting on it was asserting against the wrong pass -- measured directly: six consecutive full sweeps released nothing, and the first minor released exactly the dead owner's bytes. The same probe confirmed GcMoveHookKind::LazyArrayTape works: across that minor the surviving owner relocated and the registry followed it to the new address. Both tests also held raw headers across collections, which only became visible once headers could move. They re-read through the roots they already had -- the shadow slots, which the collector rewrites -- rather than keeping the address owned_small returned. A RuntimeHandleScope is NOT a root in this file unless register_runtime_handle_root_scanner_for_tests ran, which these two do not call, so the shadow slot is the correct root to read back from. 3611 runtime tests pass, 0 fail.
arena_alloc_gc keeps two large-object lines apart, and its comment says why: tenuring a POINTER-BEARING object does not cost its own bytes, it costs "every object it can reach, held live through the remembered set by a container nothing refers to any more". The sparse cache is a block of JSValues, so it is exactly that container, and a lazy array is the case the distinction was drawn for. Use the 128 KB line rather than the flat 16 KB one -- V8's kMaxRegularHeapObjectSize, inside the copier's own ceilings, so a cluster admitted by it is always movable. Peak RSS on records_array_16k:scan, against main: 205 MiB -> 51 MiB, where Node is 62 MiB and Bun 77 MiB. test_json_tape_lazy_get_records_its_cache_store_as_an_external_edge needed a bigger fixture to keep its premise: it wants a born-old cluster, which used to be free because every lazy header was born old unconditionally. At 4096 elements its cache is 32 KB and would now be nursery-resident, so the test would still pass its later assertions while exercising none of the containment branch it exists for. 20 000 elements is ~156 KB, over the line. 3611 runtime tests pass, 0 fail.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughLazy JSON arrays now use size-based generation and movable headers. Tape registrations follow relocated headers, and dead from-space tapes are finalized during copied-minor collection. Typed-array indexed reads perform a tag-only brand check before other guards. ChangesLazy JSON array GC and indexed-read performance
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The lazy-array GC and indexed-read changes retain their required ownership, collection, and guard behavior, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies the scoped GC changes for Resolution Complete the Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
`json_owned_tape_*`'s post-collection check dereferenced a raw `get_raw_mut_ptr` across the collection it had just forced, which is one new raw-handle debt site in a module with no ceiling (#7341) and fails the per-module ratchet. Read `materialized` inside `with_mut_ptr` instead: the header may have moved, and the scoped read is the protocol that says so. This fix was made and verified on this branch before it was first pushed, then lost to an uncommitted-tree reset while checking an unrelated gate, so the pushed head still carried the raw site. Ratchet: 944 (baseline 944), exit 0.
|
Correction to the PR description. It says the raw-handle ratchet is back to baseline (944/944). That was true of my local tree and not of the head I pushed ( Fixed in |
|
Validation status for taking this out of draft:
|
Closes #10098. Closes #10118.
Two independent costs on the same object, split into reviewable commits. Both were
measured on the JSON benchmark matrix; numbers below are from the quiet bench mini.
#10098 — a dead lazy JSON array was uncollectable by a minor
A lazy cluster was born into old-gen and pinned there, so a dead one held its whole
element graph live through the remembered set until a full collection. On
records_array_16k:scanthat full collection never arrived: the arena rebaselined its owntrigger 134M->268M->536M->1073M while
old_in_useclimbed past 48 MB, and every minorreported
survival_permille=996,copied_objects=0,freed_bytes=0.GC_TYPE_LAZY_ARRAYwas pinned for two concrete reasons, both removed the wayGC_TYPE_REGEXPremoved its own:json_tape_storekeys a tape by its owner's address, so a moved header orphaned it.Added
json_tape_store::owner_moved+GcMoveHookKind::LazyArrayTape, mirroringGcMoveHookKind::RegExpSideTables.leaked its tape. Added
finalize_dead_copied_minor_from_space_lazy_tapes— the twin ofthe sweep-entry
collect_ownerspass — wired into the flip beside map/set/errors/regexand reported in the diag line.
The cluster's generation is still decided once, by cache size against the
pointer-bearing threshold, so #7546's rule that header, cache and bitmap share a generation
holds. Large clusters stay old exactly as before.
Dead-owner tape release for a nursery header is a separate concern — a dead nursery
header can carry a stale
GC_FLAG_MARKEDfrom an earlier cycle, so the full trace'sdead-owner predicate, which assumed old-gen residency, never reported it dead. That is
mark-bit semantics rather than movability, and it is its own commit here.
#10118 — indexed reads re-classified the receiver on every access
The indexed inline cache's brand check rejected
GC_TYPE_LAZY_ARRAYoutright, forcingevery read through four layers of re-classification. The brand test now decides on the
tag alone, with the kind and index guards moved into their own
tav.get.kind_guardblock, and the shared pointer proof is computed once in the entry block instead of per
tier.
The IC slot is placed in
arrlike.ic.headerrather than the entry block: the entryplacement dominates all uses but is not array-only, and cost
string_a:parse +4.98%.arrlike.ic.headeralso dominates every use ofkey_cacheand is array-only (+0.03%).Results
records_array_16k:scanpeak RSSrecords_array_1m:scanpeak RSSrecords_array_16k:scanCPUrecords_array_1m:scanCPUrecords_array_20m:repeatThe access window showed zero separated regressions.
Validation
cargo test --release -p perry-runtime --lib: 3627 passed, 0 failed.scripts/run_lint_gates.sh: 82 green, 1 red —benchmarks/ci_public_baseline_check.py,which fails identically on a clean
origin/maintree (exit 2, same message). Noharness path is touched by this branch.
json_tape_owned.rsis a real scoped-read conversion, not an|| ()wrapper.Note for the reviewer
This branch touches collector behaviour (
gc_type_is_movable, a new move hook, a newfrom-space finalizer). It is opened as a draft because the gc-ratchet corpus has not
been run before/after by someone with that setup — that should gate the merge, not the
runtime suite alone.
Summary by CodeRabbit