perf(gc): cut the copying minor's per-live-object trace cost (straight-line object scan, hoisted per-object facts, exact memos) - #11676
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe copying nursery collector adds a specialized scan for eligible ordinary objects. It also adds memoized shape-layout lookup, inline traversal and classification paths, and per-parent remembering facts. Tests and audit checks cover scan behavior and helper correctness. ChangesCopying Nursery Trace Cost
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant CopyingNurseryCollector
participant scan_plain_object
participant PlainObjectPlan
participant visit_slot_with_parent_facts
CopyingNurseryCollector->>scan_plain_object: try scanning object
scan_plain_object->>PlainObjectPlan: build and enumerate eligible object slots
PlainObjectPlan->>visit_slot_with_parent_facts: visit each planned slot with parent facts
visit_slot_with_parent_facts-->>scan_plain_object: report slot updates
scan_plain_object-->>CopyingNurseryCollector: report whether the scan handled the object
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The new scan remains confined to copying-minor collections, leaving the full-cycle census path unaffected. No actionable merge-blocking risk is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 80 functions across 15 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ 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 |
b50185b to
f88e4fa
Compare
|
Rebased onto main CI failures from run 36628176053: none are caused by this PR. Method: a pristine release build of main
The two from-source tests use in-process fake servers, so no services are needed. I built and ran both arms by hand from Re-validation on
Perf and RSS numbers in the description were measured at the older base and were not re-run after this rebase. |
…arent remembering fact hoisted per object WIP (Part of #11549).
…e plan; exact shape-mask memo WIP (Part of #11549).
…eration probe when the note is a no-op WIP (Part of #11549).
… wide masks itself instead of declining WIP (Part of #11549).
…t scan's test counter
… both descriptor-walk instantiations
…shape census reads the walk's one body and the plain-object plan
f88e4fa to
02ed058
Compare
|
Rebased onto main The three remaining reds from run 36653797552 all predate this PR. Each fails identically on pristine main, and each bisects to #11682 (
The full |
- check_file_size: gc/layout.rs was 2009 lines after #11676; move the cfg(test) probe layout_descriptor_reachable into layout/test_accessors.rs (pure relocation), 1989 lines now. - class_id_collisions: static_shapes_tests.rs's new test-local ANON_CLASS_ID (0x0075_5eed) read as a drifted mirror of put_value.rs's ANON_CLASS_ID (0x8783_1001). Rename the test-local to REP_SEED_ANON_CLASS_ID; the two tests never shared an id. - shape_descriptor_census: one keys_array declaration moved from object/alloc.rs (3->2) to object/alloc_plain.rs (2->3); total unchanged.
Part of #11549
This cuts the copying minor's per-live-object trace cost by about a third, and it does not change when collections run. On a fully-live 5 MB binary tree (131,076 objects promoted in place,
btreen=20), the minor goes from 1,738 to 1,219 instructions per live object. Leaving out the first-cycle barrier-arming walk (#11668's subject, unchanged here), it goes from 1,472 to 953 (−35%). That is callgrind overgc_collect_minor_with_trigger_inneronly: 227.8 M → 159.8 M.It does not rescue #11645's small-tree rows. See "#11645 stacked on this" below.
What the profile showed (Phase 1)
I attributed every instruction of the minor to the innermost inlined perry-runtime function (callgrind
--dump-instrplusaddr2line -i), then grouped the functions by step. Per promoted object,btreen=20, mainb0bf0ae7evs this branch. The top 70 functions cover about 96% of the minor. Shared page-cache and TLS lookups are listed separately.How that compares with the hypotheses:
HeapChildSlotIteratorper object. There was aSHAPE_LAYOUTShash probe and mask clone per object, a per-object-mask filter, and a generation probe per object for the carrier note.gc_flagsis fully used and_reservedis type-overloaded (bit 6 isOBJ_FLAG_NULL_PROTOon objects, which is why the residual-prototype registry already has no per-object bit for them). The side-table lookups are cheaper here than expected: per-object layout masks already have an address filter (26/object), and overflow fields hit an empty map. I did not add a bit.barrier_parent_needs_remembering, and on a promoting cycleskip_rememberingmakes that answer irrelevant (72 → 11). The rawkeysword of every shaped receiver was also re-validated.qsminors, whole-table passes take a large share. The closure box-capture prune is 16% ofqs_parseminor instructions. The per-object layout owner prune is 12%, including asortof the young-key log. The remembered-set rebuild is 12%. These scale with dead or young records, not with the live objects traced. dotenv's full collections are dominated by the valid-pointer-set build, the sweep and per-dead-object layout clears, not by tracing.What changed
Ordinary objects take a straight-line scan (
gc/copying_object_scan.rs). ForGC_TYPE_OBJECT, the drain builds the generic walk's slot sequence as a plan, in the same order, and visits it directly:The payload selection is
heap_payload_slot_selection_fromitself, so there is one copy of the mask logic. The scan declines only up front, before any side effect: during a full trace or a layout-scan trace, or if the type table no longer describes objects this way. After that it never hands an object back, so no edge is visited twice.Drift check: in test and debug-assertion builds, every object it scans is also enumerated by the generic walk, and the slot lists must match.
The parent's remembering question is asked once per object (
ParentRemembering: Never / Always / ExternalSlotsOnly).barrier_parent_needs_remembering(parent, external)isOld(parent) || (external && malloc_parent(parent)). Both parent terms are fixed while one object's slots are visited, so the slot is classified only for a malloc parent.skip_rememberingfolds into Never.Raw words consult the mark memo before classifying. The memo only holds an address that classified successfully this cycle. That classification cannot change within the cycle: the range stays registered until the from-space reset, and forwarding does not touch the header fields that plausibility reads. Test and debug builds re-derive this on every hit.
SHAPE_LAYOUTSmemoizes its last pointer-mask answer, exactly. The map is reachable mutably only throughDerefMut, which clears the memo first. Every write path (insert, poison,entry,iter_mut) goes through it.The old-carrier note skips its generation probe when it would change nothing (both record flags are already set).
The drain's slot walk is instantiated for its visitor (
visit_gc_rewrite_slots_inline), and its classification is inlined. Every other caller keeps the shareddynwalk.gc_child_slotsand the selection are inlined into both instantiations, so the full-trace path does not get slower.Gate-maintenance edits:
gc_runtime_root_holders.json: re-audited thePASS1_MARKEDpin for the one-linegc/mod.rschange, and pinned one#[cfg(test)]counter.shape_descriptor_census.py: now reads the walk's one body and the plan, and has a new sabotage case.SHAPE_LAYOUTSis still declared with its map type, soregistry_lifetime_check.pystill sees it.Sabotage tests (each has a twin that must fail)
gc::tests::copying_object_scan:gc::tests::copy_slot_hoists:ParentRememberingagrees withbarrier_parent_needs_rememberingfor old, young and malloc parents, and a forgetting fact disagrees.gc::tests::shape_layout_table: the memo matches the map across insert, poison, remove andentry; a memo kept across a write answers stale.Measurements
Setup:
--release,PERRY_NO_AUTO_OPTIMIZE=1, under/tmp/perry-bench-lock.d.b0bf0ae7e. This branch was then rebased onto74fed830fwith no code change; see "Not run".perf stat -e instructions:u, median of 3./usr/bin/time %M, median of 11, arms interleaved, THP on (host default) and off (MIMALLOC_ALLOW_THP=0).Notes on the rows that are not better:
catch_js_throwinstances in the regex paths,value_is_callable,js_try_end. A no-op control build (main plus 4 KiB of unused rodata) moves dotenv by −0.17 M, so the shift comes from the code change, not from the build. I could not steer it.PERRY_GC_DIAG, main hit the high mode 13 times and this branch 22 times. Over 30 runs withPERRY_GC_DIAG, main hit it 4 times and this branch once. I don't see a direction attributable to this change, but I have not proven there isn't one.02+10%,retain+5%,alloc−18%) are multimodal on this host:02ranges from 25 to 56 MB in both arms. The THP-off column is the stable one, and there every row is within ±1% except moment.Stress and correctness
PERRY_GC_PROTECT_FROMSPACE=1,[gc-fromspace-protect] retired_setchecked armed on every run (unarmed = 0), output compared to Node 26.5.1.PERRY_GC_VERIFY_EVACUATION=1: binary-trees, dotenv and qs 200/200 each.RUST_TEST_THREADS=1 cargo test --release -p perry-runtime(codegen-units 16) on the final rebased head: 4804 passed, 0 failed, 5 ignored.PERRY_SKIP_BUILD=1 PERRY_NO_AUTO_OPTIMIZE=1, Node 26.5.1), identical to main on every filter:gc: 67/67 on both;array: 119/119 on both;class: 123 + the same 2 failures on both (test_gap_11499_class_object_static_writeandtest_gap_2159_defineproperty_class_prototype, the class static setter receives the class object as its value since #11651 (gap 11499 / 10480 regress on main) #11669 regression).SKIP_COMPILE_GATES=1 scripts/run_lint_gates.sh: 105 of 107 script gates pass. The two failures are the known ones: public-baseline freshness, andcargo xwinmissing on this host. The compile tier was not run.git diff --statwas clean afterwards.cargo fmt --check,check_file_size.sh,gc_runtime_root_holders.pyandthread_exit_address_globals.pyall pass.#11645 stacked on this
I built #11645's seven commits (#11668 + #11612 + the pacing) on top of this branch and measured them against the same main:
Verdict: the stack still does not meet the owner rule on every row. This PR moves several of its misses:
02_survivor_promotion: +4.3% → −2.8%;qs_parse: +0.38% → +0.06%.Still missing:
12_large_live_setTHP-off RSS +1.6% (106.5–107.5 MB vs 104.9–105.7 MB), unchanged by this PR.qs_parse+0.06% andjsonwebtoken+0.06% in instructions. Both are at noise level: jsonwebtoken's samples overlap main's.Not run
b0bf0ae7e. The PR head is the same diff rebased onto74fed830f, plus two type-level and lint-only commits:ShapeMaskMemo's generic parameter and the census update. The head passedcargo test -p perry-runtimeand the lint gates, but the perf/RSS tables and the gap A/B were not re-run on it.cargo testbeyondperry-runtime(no codegen change), the lint compile tier, and the Windowscargo xwin check.qsminors (hypothesis 5). They are named above as the next target.Summary by CodeRabbit
momentwas an exception.