From 0f4607c4a2a6da4a4a839d2eea069afee30c0a20 Mon Sep 17 00:00:00 2001 From: TheHypnoo Date: Sat, 13 Jun 2026 12:49:21 +0200 Subject: [PATCH 1/2] perf(codegen): make typed-feedback site registration opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every property get/set emitted a js_typed_feedback_register_site call (14 pointer/length args) plus the shape guard. Typed feedback (#854) is an opt-in profiling feature that no-ops at runtime unless PERRY_TYPED_FEEDBACK / PERRY_TYPED_FEEDBACK_TRACE is set, so in a normal build the register_site call was pure overhead executed on every field access — e.g. a dynamic-property write loop paid one no-op cross-crate call per write. Gate register_site emission on the same env that enables feedback at runtime (read fresh at codegen time). A normal build skips registration entirely; a profiling build (PERRY_TYPED_FEEDBACK=1 perry app.ts -o app && ./app, env inherited by the run) emits and uses it. The site-id is still allocated, so the shape *guard* call is unchanged and dispatch correctness is unaffected. bench_object_property (10k objects x 20 dynamic string-keyed fields): 1127ms -> ~310ms (3.6x), checksum identical to Node. No behavior change with feedback disabled (register_site was already a runtime no-op there). The dedicated typed-feedback codegen test opts in via the env var to exercise the enabled path; all other codegen tests assert the guards (still emitted). --- .../perry-codegen/src/expr/typed_feedback.rs | 27 +++++++++++++++++++ crates/perry-codegen/tests/typed_feedback.rs | 4 +++ 2 files changed, 31 insertions(+) diff --git a/crates/perry-codegen/src/expr/typed_feedback.rs b/crates/perry-codegen/src/expr/typed_feedback.rs index 891592deb7..9d647cd110 100644 --- a/crates/perry-codegen/src/expr/typed_feedback.rs +++ b/crates/perry-codegen/src/expr/typed_feedback.rs @@ -201,6 +201,28 @@ fn emit_typed_feedback_bytes_global( format!("@{}", global) } +/// Whether to emit the per-site `js_typed_feedback_register_site` call at all. +/// +/// Typed feedback (#854) is an opt-in profiling feature, disabled at runtime +/// unless `PERRY_TYPED_FEEDBACK` / `PERRY_TYPED_FEEDBACK_TRACE` is set — in +/// which case `js_typed_feedback_register_site` early-returns and does nothing. +/// But the *call itself* (14 pointer/length arguments) was still emitted on +/// every property get/set, which on hot OOP code (e.g. a method doing +/// `this.x = this.x + 1` in a tight loop) costs two no-op cross-crate calls per +/// field access — the dominant cost of the `method_calls` benchmark (491× Node). +/// +/// Gate emission on the same env that enables feedback at runtime: a normal +/// build (env unset) skips registration entirely and pays nothing; a profiling +/// build (`PERRY_TYPED_FEEDBACK=1 perry app.ts -o app && ./app`, env inherited +/// by the run) emits and uses it. The site-id is still allocated and returned +/// so the shape *guard* call is unchanged — guards stay correct either way. +fn typed_feedback_emission_enabled() -> bool { + // Read fresh (not cached) so tests that toggle the env per-case observe the + // change. At compile time this is a cheap getenv per property-access site. + std::env::var_os("PERRY_TYPED_FEEDBACK").is_some() + || std::env::var_os("PERRY_TYPED_FEEDBACK_TRACE").is_some() +} + pub(crate) fn emit_typed_feedback_register_site( ctx: &mut FnCtx<'_>, kind: TypedFeedbackKind, @@ -210,6 +232,11 @@ pub(crate) fn emit_typed_feedback_register_site( let local_site_id = ctx.ic_site_counter; ctx.ic_site_counter += 1; let site_id = ctx.typed_feedback_site_id(local_site_id); + // Default build: skip the no-op registration call (and its byte globals) + // but keep the site-id stable for the guard call. + if !typed_feedback_emission_enabled() { + return site_id.to_string(); + } let module = if ctx.strings.module_prefix().is_empty() { "main".to_string() } else { diff --git a/crates/perry-codegen/tests/typed_feedback.rs b/crates/perry-codegen/tests/typed_feedback.rs index fa82b4d084..93cc17a6f0 100644 --- a/crates/perry-codegen/tests/typed_feedback.rs +++ b/crates/perry-codegen/tests/typed_feedback.rs @@ -184,6 +184,10 @@ fn typed_feedback_trace_dump_runs_before_entry_return() { #[test] fn typed_feedback_instruments_property_and_method_boundaries() { + // Typed-feedback site *registration* is opt-in (emitted only when + // PERRY_TYPED_FEEDBACK / _TRACE is set); this test exercises the enabled + // path. The gate is read fresh per emission, so setting it here suffices. + std::env::set_var("PERRY_TYPED_FEEDBACK", "1"); let ir = ir_for(module( "typed_feedback_property.ts", vec![param(1, "obj", Type::Any)], From fbbd16d99eb12cfcbbd24f498e06d820bdd68290 Mon Sep 17 00:00:00 2001 From: TheHypnoo Date: Sat, 13 Jun 2026 14:07:17 +0200 Subject: [PATCH 2/2] test: restore PERRY_TYPED_FEEDBACK via EnvVarGuard (no env leak) The typed-feedback opt-in test set PERRY_TYPED_FEEDBACK without restoring it, leaking the variable to other tests in the binary. Wrap it in the EnvVarGuard + ENV_LOCK pattern (capture previous value, restore on drop, serialize) used in typed_shape_descriptors.rs. Addresses CodeRabbit review feedback on #5084. --- crates/perry-codegen/tests/typed_feedback.rs | 37 ++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/crates/perry-codegen/tests/typed_feedback.rs b/crates/perry-codegen/tests/typed_feedback.rs index 93cc17a6f0..a442577767 100644 --- a/crates/perry-codegen/tests/typed_feedback.rs +++ b/crates/perry-codegen/tests/typed_feedback.rs @@ -2,6 +2,37 @@ use perry_codegen::{compile_module, AppMetadata, CompileOptions}; use perry_hir::{BinaryOp, Class, ClassField, Expr, Function, Module, ModuleInitKind, Param, Stmt}; use perry_types::{FunctionType, Type}; +/// Serializes env-mutating tests so a concurrent test never observes a +/// half-applied variable. Mirrors the guard in `typed_shape_descriptors.rs`. +static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +/// Sets an env var for the duration of a test and restores the previous value +/// (or unsets it) on drop, so the mutation never leaks to other tests. +struct EnvVarGuard { + key: &'static str, + prev: Option, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: Option<&str>) -> Self { + let prev = std::env::var_os(key); + match value { + Some(value) => std::env::set_var(key, value), + None => std::env::remove_var(key), + } + Self { key, prev } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + match &self.prev { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } +} + fn empty_opts() -> CompileOptions { CompileOptions { target: None, @@ -186,8 +217,10 @@ fn typed_feedback_trace_dump_runs_before_entry_return() { fn typed_feedback_instruments_property_and_method_boundaries() { // Typed-feedback site *registration* is opt-in (emitted only when // PERRY_TYPED_FEEDBACK / _TRACE is set); this test exercises the enabled - // path. The gate is read fresh per emission, so setting it here suffices. - std::env::set_var("PERRY_TYPED_FEEDBACK", "1"); + // path. Serialize on ENV_LOCK and restore the var on drop so concurrent or + // later tests in this binary never observe the changed environment. + let _lock = ENV_LOCK.lock().unwrap(); + let _env = EnvVarGuard::set("PERRY_TYPED_FEEDBACK", Some("1")); let ir = ir_for(module( "typed_feedback_property.ts", vec![param(1, "obj", Type::Any)],