perf(typed-feedback): stop emitting recording calls into default builds (#7480) - #7702
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 (16)
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 — BLOCKED on one broken test, and on the reason CI can't tell youThe gating mechanism is right and the anti-vacuity test you wrote for it is the best part of the PR. But this breaks a currently-green test, and I want to be precise about why neither of us saw it. The regression
ir.contains("call void @js_typed_feedback_record_fallback_call")
&& ir.contains("call i64 @js_array_push_f64")Bisected against your own parent commit:
Not an edge case: the value is The fix is the same one you already applied to the other three assertions: drop the The part worth more than the fixCI structurally could not have caught this. Per-PR So Everything else checks out
Fix that one assertion and rebase; I'll merge it. |
…ds (#7480) Typed feedback is opt-in, and every recording helper begins if site_id == 0 || !typed_feedback_enabled() { return; } which is false unless PERRY_TYPED_FEEDBACK[_TRACE] is set. Codegen emitted them regardless, on every execution of every dynamic property boundary. On churn_read_big.ts that is 22.3% of the program (record_guard_pass 11.2% + observe_property 11.1%, sample, 2465 leaf samples) -- not recording, just the call and the LazyLock acquire load each one performs to answer "no". js_typed_feedback_register_site has been gated on this same env since #5093's follow-up for this same reason; the gate stopped short of the recording it registers for. Route all five pure-bookkeeping helpers through emit_typed_feedback_record_call at all nine emit sites. Helpers that also perform the operation or pick the dispatch stay unconditional, enforced by a debug_assert on the callee name and asserted in the new test. Since registration was already gated, a default-built binary could only ever produce unattributed feedback anyway; the trace dump now says so out loud rather than writing an empty file.
…CORDING The test required record_fallback_call alongside js_array_push_f64, which conflated the two. The recording call is what a default build no longer emits; the push is the load-bearing half. Asserting the recording's absence makes this a second witness for the emission gate on the ordinary arr.push(x) shape. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
22d34ba to
f24da0d
Compare
Resolved and merging as v0.5.1407I fixed the assertion on the branch rather than bouncing it back.
Full Everything in the original audit stands: the gate is compile-time and already hashed into the object-cache key, the one CI consumer sets the var for both Gates 26/26. |
…r a disabled counter `heap_payload_slot_selection` runs once per traced object per GC walk (mark, rewrite, verify). For every GC_TYPE_OBJECT it computed `raw_numeric_object_slots` via `with_typed_descriptor_for_query` — a per-object map probe plus, for every class instance, a `SHAPE_LAYOUTS` hash lookup behind a TLS RefCell borrow. That number has exactly one consumer, `record_layout_raw_numeric_object_field_range_skipped`, which returns on its first line unless PERRY_GC_LAYOUT_SCAN_TRACE armed the counter. So the shipped collector paid a hash lookup per object to produce a number nothing read — the same shape as #7702, where a facility disabled at runtime was still having its arguments evaluated. Gate the computation on `layout_scan_trace_active()`. Second item, same walk: `shape_shared_pointer_mask` returned `shape_shared_descriptor(user_ptr).map(|d| d.pointer_mask)`, cloning the whole `TypedLayoutDescriptor` to keep one of its two masks. `LayoutSlotMask` is `Heap(Vec<u64>)` above 64 slots, so a traced wide object allocated and freed a second vector — the `raw_f64_mask` — on every walk. Borrow through `with_shape_shared_descriptor` and clone only the mask returned; `shape_shared_descriptor` had no other caller and is removed rather than left as dead code.
…he doc (#7690, #7682) (#7721) * fix(gc): make the moving-loop poll default ON in the code, not just the doc (#7690) #7690 wrote the entire default-ON argument into two doc comments — the runtime's `moving_loop_polls_enabled_from_env` and codegen's `moving_safepoint_polls_enabled` — and changed neither body. Both still matched `1|on|true`, i.e. default OFF, and no test pinned the default in either direction, even though the runtime predicate had been factored out expressly to make it "unit-testable without touching process env". That is not a slower configuration, it is a different collector. Nursery pressure has exactly two precise collection points, the loop back-edge poll and the outermost microtask-pump boundary. With no poll emitted, a compute-only program reaches neither, so every nursery collection happened at the register-imprecise allocation point — where #7687 had just made it correctly non-moving. The shipped result was a collector with no nursery evacuation at all. Measured on the quiet bench host, best-of-3, `PERRY_NO_AUTO_OPTIMIZE=1` with a pinned `PERRY_RUNTIME_DIR`, against `a853135aa` binaries rerun back-to-back on the same host: | bench | main | this | a853135 | |---|--:|--:|--:| | churn | 1.01 | 0.45 | 0.66 | | churn_alloc | 0.90 | 0.42 | 0.36 | | push_cls | 0.89 | 0.40 | 0.34 | | retain | 2.33 | 1.37 | 1.33 | | tree | 5.06 | 1.63 | 5.97 | | tree_wide | 7.26 | 2.11 | 12.38 | | cycles | 0.29 | 0.19 | 0.96 | `churn_alloc` ran 13 whole-arena full collections (0.477 s of pause) where the same program at `a853135aa` ran 105 copying minors (0.016 s). `tree`'s GC pause falls 4.107 s -> 0.626 s and its max pause 266 ms -> 23 ms; `trace_worklist` drops from 2,877 ms out of the top six phases entirely. The #7161 blocker that made polls-off a stopgap is separately discharged: a poll at every back-edge defeated the #7480 element-shape fast clone, and step 4 of that work now refuses to emit a poll inside a call-free-by-construction clone. Measured both ways, `churn_read` is 0.02 s. Costs, measured rather than argued: `deeplist` 0.03 -> 0.33 and `retain1` 0.03 -> 0.42. Both are workloads whose heap stays under the initial 64 MB threshold, so they previously ran ZERO collections and the moving nursery is pure added cost; both still beat `a853135aa` (1.09 / —). `push_num` 0.16 -> 0.17. Three tests pin what was unpinned: `polls_default_is_on` and its codegen mirror `moving_safepoint_poll_default::unset_emits_the_poll` each pin one half against the full spelling table, and `polls_default_matches_codegen_mirror` pins that the two crates agree — the disagreement is silent in both directions, so it needs its own assertion rather than being left to two doc comments claiming they match. * perf(gc): stop paying a shape-layout hash lookup per traced object for a disabled counter `heap_payload_slot_selection` runs once per traced object per GC walk (mark, rewrite, verify). For every GC_TYPE_OBJECT it computed `raw_numeric_object_slots` via `with_typed_descriptor_for_query` — a per-object map probe plus, for every class instance, a `SHAPE_LAYOUTS` hash lookup behind a TLS RefCell borrow. That number has exactly one consumer, `record_layout_raw_numeric_object_field_range_skipped`, which returns on its first line unless PERRY_GC_LAYOUT_SCAN_TRACE armed the counter. So the shipped collector paid a hash lookup per object to produce a number nothing read — the same shape as #7702, where a facility disabled at runtime was still having its arguments evaluated. Gate the computation on `layout_scan_trace_active()`. Second item, same walk: `shape_shared_pointer_mask` returned `shape_shared_descriptor(user_ptr).map(|d| d.pointer_mask)`, cloning the whole `TypedLayoutDescriptor` to keep one of its two masks. `LayoutSlotMask` is `Heap(Vec<u64>)` above 64 slots, so a traced wide object allocated and freed a second vector — the `raw_f64_mask` — on every walk. Borrow through `with_shape_shared_descriptor` and clone only the mask returned; `shape_shared_descriptor` had no other caller and is removed rather than left as dead code. * test(gc): declare the pacing the alloc-point rooting tests actually assert at Four `runtime_roots` tests took no pacing guard, so they inherited the process default — which this stack changes. They are not asserting about the default; they are asserting that a specific runtime helper's object survives a collection that happens at the allocation point, and they reach that collection through the direct alloc-point minor. Under moving-loop polls that pressure is deferred to a precise safepoint, and a Rust unit test has no loop back-edge poll to drain it, so no collection runs and `assert_automatic_minor_gc_progressed` reports neither a finished assist nor an ACTIVE budgeted cycle. `force_legacy_gc_pacing` is the wrong repair and the tests say so themselves. Three of them carry an evacuation witness — "the minor did not evacuate, so nothing here was exercised and a green result would be meaningless" — and legacy pacing hands the work to the budgeted stepper, which is deliberately non-moving. Pinning it turns a failed assist assertion into a failed liveness assertion, which is the witness doing its job. `force_alloc_point_minor_pacing` (polls OFF, scavenge ON) is the one combination in which both halves hold, and it is the configuration these tests were written against. `symbol_description` has no evacuation witness and takes `force_legacy_gc_pacing`. The moving default's rooting coverage for these helpers is the gap suite's `test_gap_gc_*_rooting.ts` cases and the zeal + from-space-protect runs, not this vehicle — recorded in each test so the next reader does not mistake a pinned pacing for the default being untested. * docs(changelog): fragment for the moving-loop poll default (#7714) * docs(changelog): key the fragment to its PR number (#7721) * chore: bump version to 0.5.1418 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Part 2 of P2 (#7480 step 4). Independent of the element-shape PR; either can
land first.
The finding
The P2 brief asked for "feedback quiescence": a site that has seen the same shape
N times should stop paying
observe_property/record_guard. Reading theruntime first turned up something simpler. Typed feedback is opt-in, and
every recording helper begins:
typed_feedback_enabled()is false unlessPERRY_TYPED_FEEDBACK/PERRY_TYPED_FEEDBACK_TRACEis set. Codegen emitted the calls anyway, on everyexecution of every dynamic property boundary. On
churn_read_big.ts—200k × 1000 reads of
keep[j].v + keep[j].w— the two on the monomorphic-IC hotpath are 22.3% of the whole program:
main(generated code)js_dynamic_string_or_number_addtyped_feedback::record_guard_passtyped_feedback::observe_property(
sample, 2465 leaf samples,PERRY_DEBUG_SYMBOLS=1.) None of that isrecording. It is the cross-crate call, plus the
LazyLock<bool>acquire loadeach helper performs in order to decide it has nothing to do.
So there is no quiescence mechanism to build. There is a gate that already exists
and stops one line short.
The change
js_typed_feedback_register_sitehas been compile-gated on exactly this envsince #5093's follow-up, for exactly this reason — its doc comment says so, and
cites the same benchmark class. It simply never covered the recording it
registers for.
emit_typed_feedback_record_callnow routes all fivepure-bookkeeping helpers —
observe_property_{get,set},record_guard_{pass,fail},record_fallback_call— through the same switch, atall nine emit sites (generic-get diamond, array-push fallback, index realloc arm,
the two by-name lookup arms, method-override fallback, closure-call fallback).
The line between gated and not is asserted, not described. Helpers that also
perform the operation or pick the dispatch —
js_typed_feedback_*_guard,…_object_set_field_by_name_fast,…_object_get_field_by_name_f64,…_native_call_method— are real calls on real paths and stay unconditional. Adebug_assert!on the callee name rejects any attempt to route one of themthrough the eliding helper, and the new test asserts a default build still emits
js_typed_feedback_object_set_field_by_name_fast.This is strictly simpler than per-site quiescence and cannot freeze a site into a
wrong shape: recording is either fully on (profiling build) or fully absent
(default build), and the PIC's own deopt path is untouched.
Results
Quiet M1 mini. Measured together with the element-shape PR (both arms rebased
onto the #7690 stack —
origin/mainis GC-livelocked, see that PR):churn_read_hugeprofile after both: 100% of self time inmain.observe_property,record_guard_pass,js_dynamic_string_or_number_addand_tlv_get_addr(16.8% at baseline — TLS lookups these calls were driving) do notappear at all. The brief asked for ≤2% each.
No regression anywhere:
churn_alloc/push_cls/push_num/cycles/deeplist/tree/tree_wideall 1.00–1.03×, and a tape-defeatingjson_roundtripprobe (parse → touch every record → stringify, so the lazy tapeis materialized inside the timed region) is 0.761 s on both arms.
Behaviour
Profiling build unchanged.
typed_feedback_instruments_property_and_method_boundariessets the env and asserts every helper is still emitted; it passes untouched.
Polymorphic-after-monomorphic deopt (the brief's explicit ask) verified
end to end: a site converged on shape A for 1000 iterations, then shape B for
1000, then alternating, plus an
Object.createprototype read and adefinePropertyaccessor installed over a data property. Base, this PR, andnode all print
12053 1 7 42 11 undefined. This exercises the PIC miss arm,which is where two of the removed calls lived.
An empty trace now says so. Because registration was already compile-gated,
a default-built binary run with
PERRY_TYPED_FEEDBACK_TRACEcould only everproduce unattributed sites. With recording gated too it produces nothing,
which is the same amount of information and looks far more like success.
js_typed_feedback_maybe_dump_tracenow prints a one-line note instead ofwriting
"total_sites": 0.The note names the compile-time env as the most common cause rather than
asserting it, because it is not the only one: an empty registry from a
correctly-built binary reproduces identically on the pre-change compiler, so
there is at least one other path that instruments a site and records nothing.
That is pre-existing and out of scope; the wording is deliberately chosen so it
cannot send anyone chasing the wrong cause.
Validation
cargo test -p perry-codegen: 783/0 lib;--test typed_feedback17/0.cargo test -p perry-runtime --release: 1930/0.cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/addr_class_inventory.py: clean.the base-compiler reproduction of the seven host-noise entries).
Three existing assertions changed, all of them the incidental
record_fallback_callrider on a test about something else(
typed_feedback_guards_direct_class_field_specialization,…_direct_closure_call_specialization, and the class-field fallback-lookupdata-flow test). Each keeps the assertion it actually exists for.
Note for review
The
changelog.d/fragment is keyed on a guessed PR number and should be renamedto the real one.