perf(gc): defer the JSON materialiser's per-slot layout notes to one finalize (#7630) - #7633
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR moves GC slot-store helpers into ChangesJSON materializer layout optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JSONParser
participant ObjectStorage
participant BarrierStore
participant GCLayout
JSONParser->>ObjectStorage: Store a JSON object field
ObjectStorage->>BarrierStore: Write value with deferred layout tracking
BarrierStore-->>ObjectStorage: Return pointer-bearing status
ObjectStorage-->>JSONParser: Accumulate pointer state
JSONParser->>GCLayout: Finalize boxed-object layout
GCLayout-->>JSONParser: Preserve POINTER_FREE or mark GC_LAYOUT_UNKNOWN
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Audit (soundness half) — draft respected, not merged. One finding you need before the gap suite lands.The design reasoning is right and I checked the parts that are checkable. The Runtime suite 1,886/0; JSON gap family 7/7 byte-identical to node. The finding: I cannot make the unsafe branch fault, and neither does your zeal armI sabotaged
I built the second probe specifically because the first was vacuous (it held So one of these is true, and the PR should say which:
This matters for the PR's evidence, not its correctness: your "GC zeal + I have no objection to the change itself; I'd merge it on the argument alone Two smaller notes
|
…finalize (#7630) Step-zero profiling on the pinned mini put the per-slot layout machinery at the top of json_pipeline's cost families (~52 samples: layout_note_slot, descriptor visits, layout_transfer, layout_forget_object). The payer is the parse cohort: records are born POINTER_FREE, the first string field builds a per-object side-table pointer mask, and every subsequent field store pays a hashmap round-trip -- then layout_transfer moves the mask on promotion and layout_forget_object drops it at death. The materialiser owns each object's whole construction, so its store loops now use runtime_store_jsvalue_slot_layout_deferred -- bit-for-bit the shared helper minus the layout note (canonicalization, string addref demote, and the write barrier with its SATB shade all kept) -- returning the one fact the notes were computing (pointer-bearing), which the loop accumulates and settles ONCE via layout_finish_deferred_boxed_object: - no pointer stored: the POINTER_FREE birth state is still the truth and keeps its whole-payload trace skip (number-only records); - any pointer stored: GC_LAYOUT_UNKNOWN, the tag-checked scan-all state. A pointer mask can never skip anything for a cohort whose every slot is a NaN-boxed JSValue, so the mask machinery bought nothing here. Routed through layout_mark_unknown so a mask created by the shaped path's by-name fallback mid-construction is removed, not stranded. The shaped path's finalize runs on the live pointer re-read from the parse root, so a mid-parse collection cannot leave it on a stale copy.
….rs (2000-line cap)
e0fc6b1 to
1865edb
Compare
Merged as v0.5.1361 — on the argument, with the evidence question split outGap suite result accepted: the one reported regression is the known host-local Re-verified here: runtime 1,902/0, Why I merged despite my open finding: the change is sound by argument — But I chased the vacuity question further and the benign explanation is Parse value strings are not longlived or interned — So the line in this PR's body — "GC zeal + fromspace-protect arm: output One data point for #7630 while you are in here: my census on current main reads |
1 participant
Fixes #7630 — the top cost family from the step-zero profile that issue records.
The cost
Parse-built records are born
POINTER_FREE; the first string field builds a per-object side-table pointer mask, and from then on every field store pays a hashmap round-trip (layout_note_slot),layout_transfermoves the mask on every promotion, andlayout_forget_objectdrops it at death. At 200k records × ~13 slots that machinery topped the profile (~52 samples, ahead of the old-page family the in-flight deferral PR targets).The observation that makes it deletable: a pointer mask can never skip anything for this cohort. Every slot the materialiser writes is a NaN-boxed
JSValue, and the tracer's tag check rejects non-pointers anyway — the mask machinery only ever pays for itself by skipping raw-f64 slots, which parse objects do not have.The fix
The materialiser owns each object's whole construction, so its store loops use
runtime_store_jsvalue_slot_layout_deferred— bit-for-bit the shared choke-point helper minus the layout note (typed-slot canonicalization, string addref demote, and the write barrier with its SATB shade all kept — the #7602 lesson) — returning the one fact the notes were computing (pointer-bearing). The loop accumulates it and settles the layout state once per object vialayout_finish_deferred_boxed_object:POINTER_FREEbirth state is still the truth, and it keeps its whole-payload trace skip (number-only records lose nothing);GC_LAYOUT_UNKNOWN, the tag-checked scan-all state. Routed throughlayout_mark_unknown, not a bare state store, so a mask that the shaped path's by-name fallback DID create mid-construction (shape-overflow records) is removed rather than stranded.The shaped path's finalize runs on the live pointer re-read from the parse root, so a mid-parse collection cannot leave it operating on a stale copy.
Also:
barrier.rscrossed the 2000-line cap with the new helper; its slot-store helpers moved tobarrier_store.rs(pure move).Measured — pinned mini, interleaved ×3, hash-identical every round
Census structure identical both arms (3 cycles, 0.0 MB survivor-copied, 101 MB promoted — the moved-bytes census is load-independent). Post-fix profile:
layout_note_slot,layout_forget_object,layout_only,fixed_slotare gone from the top; the remaining leaders are the old-page/promote family (the deferral PR's target) and remembered-set inserts.Verification
12_large_live_set.heap_used_bytesat −504 bytes — the documented sample-dependent ungated cell, well inside its recorded 9,072-byte spread. All other movement is ungated RSS/wall ≤0.44 %.PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1): output identical — no stranded children under forced evacuation.perry-runtimesuite 1,886 passed / 0 failed;cargo fmt --checkandcheck_file_size.shclean.parity_report_20260808_101857.json): no real regressions. The one reported regression,test_gap_zlib_4917_levelpass → compile_fail, is the documented host-local runtime-dir resolution flake, disproven on this branch directly: withPERRY_RUNTIME_DIRpinned to the freshly built archives the test compiles and matches node byte-for-byte. The 10node_fail → parity_failstatus changes are oracle-coverage transitions (pre-existing gaps the pinned Node now reaches — identical set to the previous clean run on an unrelated branch), andtest_gap_iterator_helpers_2874improved to pass.Summary by CodeRabbit