perf(typed-feedback): stop emitting recording calls into default builds (#7480)

[proggeramlug]

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 the
runtime first turned up something simpler. Typed feedback is opt-in, and
every recording helper begins:

if site_id == 0 || !typed_feedback_enabled() { return; }

typed_feedback_enabled() is false unless PERRY_TYPED_FEEDBACK /
PERRY_TYPED_FEEDBACK_TRACE is set. Codegen emitted the calls anyway, on every
execution 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 hot
path are 22.3% of the whole program:

symbol	share of self time
main (generated code)	66.0%
js_dynamic_string_or_number_add	11.7%
typed_feedback::record_guard_pass	11.2%
typed_feedback::observe_property	11.1%

(sample, 2465 leaf samples, PERRY_DEBUG_SYMBOLS=1.) None of that is
recording. It is the cross-crate call, plus the LazyLock<bool> acquire load
each 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_site has been compile-gated on exactly this env
since #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_call now routes all five
pure-bookkeeping helpers — observe_property_{get,set},
record_guard_{pass,fail}, record_fallback_call — through the same switch, at
all 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. A
debug_assert! on the callee name rejects any attempt to route one of them
through 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/main is GC-livelocked, see that PR):

churn_read_huge profile after both: 100% of self time in main.
observe_property, record_guard_pass, js_dynamic_string_or_number_add and
_tlv_get_addr (16.8% at baseline — TLS lookups these calls were driving) do not
appear at all. The brief asked for ≤2% each.

No regression anywhere: churn_alloc / push_cls / push_num / cycles /
deeplist / tree / tree_wide all 1.00–1.03×, and a tape-defeating
json_roundtrip probe (parse → touch every record → stringify, so the lazy tape
is materialized inside the timed region) is 0.761 s on both arms.

Behaviour

• Profiling build unchanged. typed_feedback_instruments_property_and_method_boundaries
  sets 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.create prototype read and a
  defineProperty accessor installed over a data property. Base, this PR, and
  node 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_TRACE could only ever
  produce 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_trace now prints a one-line note instead of
  writing "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_feedback 17/0.
• cargo test -p perry-runtime --release: 1930/0.
• cargo fmt --all -- --check, scripts/check_file_size.sh,
  scripts/addr_class_inventory.py: clean.
• Gap suite: no regression attributable to this change (see the sibling PR for
  the base-compiler reproduction of the seven host-noise entries).

Three existing assertions changed, all of them the incidental
record_fallback_call rider on a test about something else
(typed_feedback_guards_direct_class_field_specialization,
…_direct_closure_call_specialization, and the class-field fallback-lookup
data-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 renamed
to the real one.

[coderabbitai]

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 74166bd2-8237-43e8-9860-ccf178df932b

📥 Commits

Reviewing files that changed from the base of the PR and between 827d4e7 and f24da0d.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (16)

• CLAUDE.md
• Cargo.toml
• changelog.d/7702-typed-feedback-recording-emission-gate.md
• crates/perry-codegen/src/expr/array_push.rs
• crates/perry-codegen/src/expr/index.rs
• crates/perry-codegen/src/expr/mod.rs
• crates/perry-codegen/src/expr/property_get.rs
• crates/perry-codegen/src/expr/property_get/generic_dispatch.rs
• crates/perry-codegen/src/expr/property_get/helpers.rs
• crates/perry-codegen/src/expr/typed_feedback.rs
• crates/perry-codegen/src/lower_call/early_branches.rs
• crates/perry-codegen/src/lower_call/method_override.rs
• crates/perry-codegen/tests/typed_feedback.rs
• crates/perry-codegen/tests/typed_shape_descriptors.rs
• crates/perry-runtime/src/typed_feedback.rs
• crates/perry-runtime/src/typed_feedback/trace.rs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Audit — BLOCKED on one broken test, and on the reason CI can't tell you

The 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

crates/perry-codegen/tests/typed_shape_descriptors.rs:405-410, number_typed_local_array_push_keeps_layout_note_and_barrier, asserts:

ir.contains("call void @js_typed_feedback_record_fallback_call")
    && ir.contains("call i64 @js_array_push_f64")

Bisected against your own parent commit:

checkout	result
94ad78435 (this PR's parent)	ok
fb8806f25 (the typed-feedback commit)	FAILED — "wrong runtime values must keep a boxed runtime push fallback"

Not an edge case: the value is Expr::LocalGet(1), and expr_produces_canonical_raw_f64 explicitly excludes LocalGet, so array_push.rs:461's keep_guarded_numeric_push is true either way — this is the ordinary arr.push(x) shape.

The fix is the same one you already applied to the other three assertions: drop the record_fallback_call half, keep the js_array_push_f64 half. The test's intent — "the boxed runtime push fallback survives" — is still true and still worth asserting; only the recording call is gone.

The part worth more than the fix

CI structurally could not have caught this. Per-PR cargo-test runs --lib --bins (test.yml:829) — no tests/*.rs integration suite runs on a PR at all. e2e-scoped runs only suites the diff names, plus a hand-maintained SOURCE_SUITE_MAP that maps all of crates/perry-codegen/src/ to exactly three suites, none of them typed_shape_descriptors. ci_e2e_scope.py's own header names this gap: "a source change that regresses an existing suite the diff doesn't name (#6037 class)… can land green and sit red for days."

So cargo test -p perry-codegen --lib + --test typed_feedback — a reasonable-looking validation, and what the PR body reports — is exactly the command that cannot see this. The one that can is the full cargo test -p perry-codegen. I'm running that against main now to separate your regression from the branch's stale base; two native_proof_regressions failures and one integer_arithmetic_array_push_… failure are pre-existing and not yours.

Everything else checks out

• Compile-time, not runtime (typed_feedback.rs:249, env::var_os in the compiler process), already correctly hashed into the object-cache key.
• The one CI consumer is not vacuated: test_typed_feedback_runtime_evidence.py sets the var for both compile_env and run_env.
• No production path loses an optimization — js_typed_feedback_class_field_get_guard returns on the fast contract check without touching the registry when !enabled && !descriptors_in_use(), so default-build guard/PIC decisions never read recorded data. Your framing as pure overhead removal holds.
• Both directions asserted — the new a_default_build_emits_no_typed_feedback_recording_calls proves the calls are gone and that the property boundaries are still lowered, which is what stops it from passing on a build that emits nothing at all.

Fix that one assertion and rebase; I'll merge it.

[proggeramlug]

Resolved and merging as v0.5.1407

I fixed the assertion on the branch rather than bouncing it back.

number_typed_local_array_push_keeps_layout_note_and_barrier required record_fallback_call and js_array_push_f64 together. Those are two different things, and only one of them is this test's subject: the boxed fallback push is load-bearing, the fallback recording is exactly what a default build no longer emits. So the assertion now keeps the push as a positive and adds the recording's absence as a negative — which turns the test into a second witness for your emission gate, on the ordinary arr.push(x) shape rather than a contrived one (LocalGet is excluded from expr_produces_canonical_raw_f64, so this is the common path).

Full cargo test -p perry-codegen: 6 failures, byte-identical to the set main already carries. Those 6 are pre-existing and unrelated — I'm filing them separately, since per-PR CI runs --lib --bins and never sees them.

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 compile_env and run_env, no production path loses an optimization (js_typed_feedback_class_field_get_guard returns on the fast contract check without touching the registry when !enabled && !descriptors_in_use()), and your a_default_build_emits_no_typed_feedback_recording_calls asserts both directions properly — the calls are gone and the property boundaries are still lowered, so it can't pass on a build that emits nothing.

Gates 26/26.