perf(json): batch-materialize a lazy tape array by re-parsing its blob (#7478) - #7499
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
`force_materialize_lazy` walked the tape element-by-element, which the #7478 decomposition measured at ~2.3x the direct parser's batch tree build (56 ms/iter vs 24 ms/iter on the 10k-record fixture). A lazy array only ever stands for a top-level array, so its retained blob is exactly that array's source: re-parsing it with `DirectParser` produces the same tree, and since #7483 put the DirectParser's decimal fast path on one correctly-rounded division, the same numbers bit-for-bit. The reparse runs inside a nesting-safe `GcSuppressScope`. `DirectParser` holds `input: &[u8]` derived from the blob for the whole parse, carries an unrooted raw-pointer shape cache, and fills fresh arrays through `note_array_slot_layout_only` (which skips the generational barrier on the strength of that suppression) - all three are only sound in a no-move window, which is why the first attempt SIGSEGV'd. Cached elements are patched back over the fresh slots through `store_array_slot`, so a handed-out (and possibly mutated) element keeps both its value and its identity, and a pointer landing in a RawF64-layout array downgrades the layout instead of hiding from the tracer. Once most elements are already cached the element-wise merge is the cheaper producer, so the reparse only fires below the measured crossover. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
The new #7478 test introduced three bare `get_raw_*_ptr` reads, pushing `gc/tests/runtime_roots/callback_scanners.rs` from 47 to 50 and failing `scripts/raw_handle_debt.py` (1002 vs the 999 baseline). Every one of them was a header read carried across an allocating call, which is the shape `RuntimeHandle::across_mut` exists to express: `lazy_get` and the field-set both return the post-collection header now, and the key string is allocated inside the same window with the receiver re-derived after it. The rooted key handle is gone with it - nothing allocates between its creation and its only use. Back to 999 = baseline. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
`cargo test -p perry-runtime --lib` warned `unused variable: hdr` — the refreshed header from the `lazy_get` pairing is shadowed by the one from the field-set pairing before anything reads it. Bind it as `_` and say why in a comment, rather than carrying a name that reads as if the pointer were still live. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
20566a0 to
dc0383b
Compare
The 2.3x in the #7499 comments came from #7478's per-iteration figures (56 vs 24 ms/iter), which compared scan+materialize against tree build. The quiet-host decomposition gives a cleaner apples-to-apples number: 2540 ms of element-wise materialization vs 1412 ms for 50 whole DirectParser parses of the same fixture, tokenization included. Also states the crossover as the identity it is -- a reparse rebuilds the whole array, so flipping after fraction f pays when f < 1 - 1/r -- which is what makes cached_count*2 < cached_length the right gate at r = 1.8. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
A sequential scan of a lazy JSON array was invisible to the only adaptive signal `lazy_get` had. `cumulative_walk_steps` accumulates one step per element on a sequential walk, against a threshold of 2n, so it provably never trips — every element of a scanned array was materialized one at a time at ~2.3x the batch parser's per-element rate. Add `LazyArrayHeader::sequential_streak`, a run-length of consecutive ascending cold reads, and trip it at `scan_flip_threshold` (n/64, floored at 64). The flip fires while the sparse cache is still nearly empty, which is what makes `force_materialize_lazy` choose #7499's batch reparse rather than the element-wise merge walk — firing it late is a no-op, which is why #7499 alone did not move `field_access`. The trigger carries the callee's own `cached_count * 2 < cached_length` test, so it never asks for a producer that would be declined. Tests split into json_tape_tests.rs to stay under the 2000-line cap. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
The 2.3x in the #7499 comments came from #7478's per-iteration figures (56 vs 24 ms/iter), which compared scan+materialize against tree build. The quiet-host decomposition gives a cleaner apples-to-apples number: 2540 ms of element-wise materialization vs 1412 ms for 50 whole DirectParser parses of the same fixture, tokenization included. Also states the crossover as the identity it is -- a reparse rebuilds the whole array, so flipping after fraction f pays when f < 1 - 1/r -- which is what makes cached_count*2 < cached_length the right gate at r = 1.8. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…7537) * perf(json): flip lazy-tape scans to the batch parser early (#7478) A sequential scan of a lazy JSON array was invisible to the only adaptive signal `lazy_get` had. `cumulative_walk_steps` accumulates one step per element on a sequential walk, against a threshold of 2n, so it provably never trips — every element of a scanned array was materialized one at a time at ~2.3x the batch parser's per-element rate. Add `LazyArrayHeader::sequential_streak`, a run-length of consecutive ascending cold reads, and trip it at `scan_flip_threshold` (n/64, floored at 64). The flip fires while the sparse cache is still nearly empty, which is what makes `force_materialize_lazy` choose #7499's batch reparse rather than the element-wise merge walk — firing it late is a no-op, which is why #7499 alone did not move `field_access`. The trigger carries the callee's own `cached_count * 2 < cached_length` test, so it never asks for a producer that would be declined. Tests split into json_tape_tests.rs to stay under the 2000-line cap. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * docs: changelog fragment for #7537 Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * docs(json): quote the measured 1.8x element-wise/batch ratio The 2.3x in the #7499 comments came from #7478's per-iteration figures (56 vs 24 ms/iter), which compared scan+materialize against tree build. The quiet-host decomposition gives a cleaner apples-to-apples number: 2540 ms of element-wise materialization vs 1412 ms for 50 whole DirectParser parses of the same fixture, tokenization included. Also states the crossover as the identity it is -- a reparse rebuilds the whole array, so flipping after fraction f pays when f < 1 - 1/r -- which is what makes cached_count*2 < cached_length the right gate at r = 1.8. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * test(json): pin LazyArrayHeader::cached_length to offset 0 Codegen inlines a lazy array's `.length` as a raw u32 load at offset 0 instead of calling js_array_length, so the field sitting first is a contract with the compiler. It was only ever stated in a doc comment: a reordering would have produced silently wrong .length values on every unmaterialized lazy array with the whole suite still green. Adding a field to this struct is exactly when that can happen. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * fix(json): count the whole sparse cache for scan-flip eligibility CodeRabbit review on #7537, two real findings. The flip's eligibility test approximated the cache count as `i + 1`, which is only the true count for a scan starting at zero that touches nothing else. Any earlier cold read outside the prefix made it UNDERcount, so the trigger could fire on an array force_materialize_lazy then declined to reparse -- which does not merely waste the trigger, it materializes the whole array early through the element-wise merge walk, the exact path the flip exists to avoid. Both sites now read one lazy_cached_count helper, so the trigger provably cannot ask for a producer the callee declines -- which is what the comment already claimed. Covered by a 131-element case with index 130 read first. The streak also recorded zero for a cold read that did not continue the previous run, when such a read is itself a run of length one. A scan beginning anywhere but index 0 therefore needed 65 reads to trip a threshold of 64. Roots the pre-flip JSValue in the identity test: it was a raw pointer copy held across 200 materializations plus a reparse, i.e. the very stale-local shape that test exists to detect. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * docs: final measured numbers in the #7537 changelog fragment Re-measured on the repo's own benchmarks/json_polyglot binaries after the review fixes, rather than the decomposition variants: field_access 2938 -> 2043 ms, roundtrip 201 -> 201 ms, checksums matching node on both arms. Also records the two pre-existing defects the measurement surfaced. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * chore: bump version to 0.5.1302 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Implements step 2 of #7478's roadmap. Unblocked by #7483.
What changed
force_materialize_lazywalked the tape element-by-element to build theArrayHeadertree. #7478's decomposition measured that walk at ~2.3x the directparser's batch tree build — 56 ms/iter vs 24 ms/iter on the 10k-record fixture —
and identified the fix: a lazy array only ever stands for a top-level array
(
try_parse_via_tapebuilds one only whentape_entries[0].kind == KIND_ARR_START, always withroot_idx = 0), so its retainedblob_strisexactly that array's source text. Re-parsing it with
DirectParserproduces theidentical tree in one linear pass.
"Identical" now includes the numbers: #7483 put the DirectParser's decimal fast
path on a single correctly-rounded division, so it agrees bit-for-bit with
str::parse::<f64>(whatmaterialize_numberuses) and with node. Thatdivergence (#7477) is what blocked this the first time.
The crash, root-caused
The earlier prototype SIGSEGV'd intermittently.
DirectParseris only soundinside a no-move window, in three independent ways — all three visible in
json/parser.rs:input: &'a [u8]derived from the blob'sStringHeaderpayload forthe whole parse and has no way to re-derive it;
hot_shape_keys: [*const StringHeader; 8],hot_shape_array: *mut ArrayHeader) across everyallocation — the "runtime-side cache of a raw heap pointer is a GC root, and
the static checker cannot see it" shape from CLAUDE.md;
array_push_parse_fastfills fresh arrays throughnote_array_slot_layout_only, which deliberately skips the generationalbarrier for young arrays and says so in a comment that begins "GC is
suppressed for the whole direct parse".
js_json_parsebuys all three with itsgc_suppress()window. The prototypecalled the parser without one. The fix wraps the parse in a
GcSuppressScope—nesting-safe on purpose, because
force_materialize_lazyis reachable frominside
try_stringify_lazy_array, and the flatgc_unsuppress()would end anouter window early.
Around that window the ordering follows the #7341 discipline: every header
re-read is a
RuntimeHandle::across_{mut,const}paired with the call that cancollect (no new bare
get_raw_*_ptr—json_tape.rsstays at its ceiling of22), the parsed tree is handed to
PARSE_ROOTSbefore the suppression closesand promoted to a handle-scope root before those are restored, and the function
returns the refreshed header on every exit including the declining ones.
Cache-merge semantics
The reparse rebuilds every element from source, so the sparse cache has to be
patched back over the fresh slots: a cached slot holds the JSValue user code
already has a reference to and may have mutated through it (
parsed[2].id = 99), while the blob still says the old value. The patch loop preserves both thevalue and the identity (
parsed[i] === parsed[i]), and runs inside a suppressionwindow so the header, bitmap, cache and array pointers stay valid without a
per-element re-read.
It stores through
store_array_slot, not a raw*elements.add(i) = bits: thereparsed array is frequently in
RawF64numeric layout, and a raw store wouldleave it flagged pointer-free with a live pointer inside — invisible to the
tracer.
When it fires
Only below the measured crossover (
cached_count * 2 < cached_length). Once mostelements are already cached the element-wise merge is the cheaper producer — it
copies the cached JSValues and materializes only the remainder — so a reparse
there would rebuild subtrees it is about to throw away. From the 2.3x ratio the
crossover sits at ~43% uncached; the check is on the conservative side of it.
This is deliberately a no-op for
bench_field_access, and that is not anoversight. That benchmark touches every element before stringifying, so by the
time
try_stringify_lazy_arrayforce-materializes, the bitmap is full and themerge walk is already the cheap path — all of its 2981 ms went into the 10k
lazy_getcalls, not into materialization. Closing that gap is #7478's step 3(a batch-flip trigger that materializes early, while the cache is still mostly
empty); this PR is what makes that trigger worth adding, since step 1 of the
issue measured it as neutral against the slow materializer. What benefits today
is any full-array operation on a lazy array whose elements have not been
individually walked:
.map/.filter/ spread /for…of/sort/Object.keyson a freshJSON.parseresult (the1_json_pipeline"filter-all-records" shape), and the random-access
cumulative_walk_stepstrigger, which fires after ~4 accesses on a 10k array — cache count 4 out of
10000, deep in reparse territory.
No timing claims in this PR: the host was at load average 27-40 throughout. Perf
belongs on a quiet host at review.
Validation
Unit —
cargo test -p perry-runtime --lib: 1718 passed, 0 failed, 3ignored. New/extended cases:
test_json_tape_reparse_materialize_preserves_a_mutated_cache_entry(new,gc/tests/runtime_roots/callback_scanners.rs): plants a mutated cached element(
parsed[2].id = 99), materializes via reparse, asserts the mutation survivesand the array slot is the same JSValue, then asserts the stringified text is
[{"id":0},{"id":1},{"id":99},{"id":3}].test_json_tape_force_materialize_sparse_cache_handles_survive_copied_minor_gc(existing sabotage test) keeps firing the
ForceLazyArrayRootedsafepoint —the hook forces a copying minor there and the test asserts the array handle was
refreshed. It now runs against the reparse producer, and a new assertion pins
that, so the instrument cannot silently change subject.
force_materialize_majority_cached_uses_the_merge_walk_not_a_reparse(new):pins the crossover decision, not just the values — without it the test passes
either way and the crossover could silently invert.
force_materialize_declines_reparse_when_the_tape_root_is_not_the_blob_root(new): a lazy header whose tape root is not the blob's first value must decline
and still materialize correctly through the walk.
force_materialize_*tests gained assertions that they takethe reparse path (one also pins the RawF64 -> pointer layout downgrade).
The witness for all of the above is a thread-local reparse counter, so a test can
assert which producer ran rather than only that nothing threw.
Sabotage — deleting the cache-patch loop turns 3 tests red, including the
existing sparse-cache sabotage test and the new mutation test ("the reparse must
patch the handed-out element back, not a fresh copy"). Restored.
Byte-for-byte vs node 26.5.1 — an 11-scenario probe (600 records x 13 fields
incl. floats, exponents, escapes, non-ASCII, nested arrays/objects; plus the
10k-element
i * 3.14159float array from the #7477 divergence class, edgevalues
-0.0/1e21/5e-324/1.797…e308/9007199254740993, awhitespace-bearing blob, identity checks, sort, and repeated materialization).
Scenarios print whole JSON texts, so the diff is the byte check. All three
modes —
PERRY_JSON_TAPE=1,=0, and auto — are byte-identical to node(670,312 bytes each). An instrumented runtime confirmed the reparse path ran 10x
across that probe, and that the every-element-touched scenario correctly did
not take it.
json gap tests — all 12
test-files/test_gap_*json*.tscompile, run, andmatch node 26.5.1 byte-for-byte, with matching exit codes, under both
PERRY_JSON_TAPE=1and=0.#7478's own benchmarks, correctness only —
bench_field_accesschecksum is2552985550underPERRY_JSON_TAPE=1, under=0, and under node — the value#7483 established as ground truth.
benchmarks/json_polyglot/bench.ts(theunmutated-blob memcpy roundtrip, the path this PR must not disturb) checksums
53735550in auto mode, matching node.GC instruments — the probes were compiled with
PERRY_GC_MOVING_LOOP_POLLS=1and run under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 PERRY_GC_DIAG=1. Instrument livenessconfirmed —
[gc-fromspace-protect] mode=ProtectPages retired_set=#0…#5with acopying minor that moved 144,174 objects — 120 reparses inside that protected
run, exit 0, output still byte-identical to node.
cargo fmt --all -- --checkclean.scripts/raw_handle_debt.py: 999 =baseline, all modules within ceilings.
scripts/check_file_size.sh: OK.One pre-existing failure found (not from this PR)
PERRY_GC_VERIFY_EVACUATION=1withPERRY_JSON_TAPE=1trips theold-young-edge-verifier:
A/B'd against a runtime built from this branch's merge-base with only
json_tape.rs+ the test file reverted, same binary pipeline, same probe: thebaseline fails identically (
lazy_array(9)parents,object(2)children,slot_page_ever_dirty=false; 8205 edges on the baseline vs 10095 with thischange — the same defect, differing only in how much lazy-cache traffic each arm
generates). So it is on
maintoday, not introduced here. The slot is the lazyheader's sparse element cache (
materialized_elements, a separate arenaallocation attributed to the
lazy_arrayparent), written inlazy_get. Filedas #7500 rather than folded in here.
#7478 stays open for step 3.