From 7741706eb379b4baafa9aa8dc866e73156c96892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 11:08:08 +0200 Subject: [PATCH 1/3] perf(hir): hoist a loop-invariant property array receiver out of counted for-loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `for (let i = 0; i < holder.arr.length; i++) l = holder.arr[i];` repeated a by-name property lookup on every iteration — the emitted body carried js_object_get_field_by_name_f64 plus IC-miss handling — and, worse, the loop never entered the packed-array machinery at all, because that matcher requires the array expression to be a bare local. Writing the hoist by hand (`const a = holder.arr;`) was worth 27x, which is the ceiling this reaches automatically. Mac mini, ns/op, perry before -> after (node): module-global receiver 20.59 -> 0.66 (0.54) local object receiver 3.46 -> 0.47 (0.49) hand-hoisted control 0.50 -> 0.50 (0.49) The local-receiver row now edges out node, and the module-global row goes from 38x slower to 1.2x. Parameter (6.85) and captured (20.29) receivers are unchanged: they are not `const x = { … }` bindings, so the data-property proof below does not cover them yet. Equivalence rests on three checks, all made before rewriting: 1. The property is a DATA field. Only receivers bound by `const x = { … }` whose initializer lowered to a closed-shape record class qualify; is_closed_shape rejects getters and setters, so reading such a field cannot run user code. This is keyed on the INITIALIZER, not the binding's type: a getter-bearing literal happens to infer as `Any`, but an annotated structural object type can still be backed by an accessor, so a type check would be unsound. 2. Nothing in the loop can rebind the receiver. `holder = other` would leave the temp pointing at the previous object's array, and no runtime check can recover this — two objects from the same literal share a shape, so the rewrite simply refuses. 3. Nothing in the loop can write the property or call anything. A call could assign `holder.arr` behind our back; a direct write is visible syntactically. The scan rejects calls, closures, `new`, property and index writes, and anything it does not positively recognise. Differential vs node: basic sum, receiver reassigned mid-loop to a same-shaped object, property overwritten by a call inside the loop, a getter receiver (invocation count preserved), nested loops over `m.rows[i][j]`, empty array, array grown during iteration, and string elements — byte-identical. Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p --- .../perry-hir/src/destructuring/var_decl.rs | 13 + crates/perry-hir/src/lower/context.rs | 5 + crates/perry-hir/src/lower/locals.rs | 10 + .../perry-hir/src/lower/lowering_context.rs | 9 + crates/perry-hir/src/lower/mod.rs | 1 + .../src/lower/property_array_hoist.rs | 391 ++++++++++++++++++ crates/perry-hir/src/lower_decl/body_stmt.rs | 38 +- 7 files changed, 461 insertions(+), 6 deletions(-) create mode 100644 crates/perry-hir/src/lower/property_array_hoist.rs diff --git a/crates/perry-hir/src/destructuring/var_decl.rs b/crates/perry-hir/src/destructuring/var_decl.rs index 0590e746df..f23a8e9c6d 100644 --- a/crates/perry-hir/src/destructuring/var_decl.rs +++ b/crates/perry-hir/src/destructuring/var_decl.rs @@ -328,6 +328,19 @@ pub(crate) fn lower_var_decl_with_destructuring( None if !is_var_decl => Some(Expr::Undefined), other => other, }; + // Remember `const x = { … }` bindings whose initializer became a + // closed-shape record class. That is the only proof available that + // every property of `x` is a data field rather than an accessor — + // `is_closed_shape` rejects getters and setters — and the + // loop-invariant property hoist refuses to fire without it. + if !mutable { + if let Some(Expr::New { class_name, .. }) = init.as_ref() { + if class_name.starts_with("__AnonShape_") { + ctx.closed_shape_literal_locals + .insert(id, class_name.clone()); + } + } + } result.push(Stmt::Let { id, name, diff --git a/crates/perry-hir/src/lower/context.rs b/crates/perry-hir/src/lower/context.rs index 466384c522..ee1a734224 100644 --- a/crates/perry-hir/src/lower/context.rs +++ b/crates/perry-hir/src/lower/context.rs @@ -201,6 +201,7 @@ impl LoweringContext { mixin_funcs: HashMap::new(), anon_shape_classes: HashMap::new(), anon_shape_fields: HashMap::new(), + closed_shape_literal_locals: HashMap::new(), prefer_exported_method_shape_seed: false, forward_class_names: std::collections::HashSet::new(), forward_class_decl_depth: std::collections::HashMap::new(), @@ -987,6 +988,10 @@ impl LoweringContext { self.locals.lookup_type(name) } + pub(crate) fn lookup_local_type_by_id(&self, id: LocalId) -> Option<&Type> { + self.locals.lookup_type_by_id(id) + } + pub(crate) fn lookup_func(&self, name: &str) -> Option { self.functions_index .get(name) diff --git a/crates/perry-hir/src/lower/locals.rs b/crates/perry-hir/src/lower/locals.rs index a55e100ed4..8007344128 100644 --- a/crates/perry-hir/src/lower/locals.rs +++ b/crates/perry-hir/src/lower/locals.rs @@ -81,6 +81,16 @@ impl Locals { } /// `Type` of the innermost binding named `name`, if any. O(1). + /// Type of the innermost binding carrying `id`. Needed when a + /// transform holds a `LocalId` from already-lowered HIR and has no name. + pub(crate) fn lookup_type_by_id(&self, id: LocalId) -> Option<&Type> { + self.entries + .iter() + .rev() + .find(|(_, entry_id, _)| *entry_id == id) + .map(|(_, _, ty)| ty) + } + pub(crate) fn lookup_type(&self, name: &str) -> Option<&Type> { self.lookup_index(name).map(|i| &self.entries[i].2) } diff --git a/crates/perry-hir/src/lower/lowering_context.rs b/crates/perry-hir/src/lower/lowering_context.rs index 5426f0b510..154815cf05 100644 --- a/crates/perry-hir/src/lower/lowering_context.rs +++ b/crates/perry-hir/src/lower/lowering_context.rs @@ -776,6 +776,15 @@ pub struct LoweringContext { /// (e.g. recognizing a bundled mysql2 `createPool(config)` by its option /// names) recovers them here. pub(crate) anon_shape_fields: HashMap>, + /// `const x = { … }` bindings whose initializer lowered to a closed-shape + /// record class (`__AnonShape_*`), mapped to that class name. Membership + /// is a proof that every property of `x` is a DATA field: `is_closed_shape` + /// rejects getters and setters, so reading one is side-effect free. The + /// loop-invariant property hoist needs exactly that guarantee, and cannot + /// get it from the binding's TYPE — a getter-bearing literal infers as + /// `Any`, but an *annotated* structural object type can still be backed by + /// an accessor. + pub(crate) closed_shape_literal_locals: HashMap, /// Set while lowering a directly exported binding/default expression. /// Eligible method literals consume this flag and retain the seeded IIFE /// representation needed to publish an exact cross-module own-method diff --git a/crates/perry-hir/src/lower/mod.rs b/crates/perry-hir/src/lower/mod.rs index ed53b5be54..bec7f52d7c 100644 --- a/crates/perry-hir/src/lower/mod.rs +++ b/crates/perry-hir/src/lower/mod.rs @@ -97,6 +97,7 @@ pub(crate) use lowering_context::{ }; mod locals; +pub(crate) mod property_array_hoist; pub(crate) use locals::Locals; mod typed_parse; diff --git a/crates/perry-hir/src/lower/property_array_hoist.rs b/crates/perry-hir/src/lower/property_array_hoist.rs new file mode 100644 index 0000000000..73ebbdeb0a --- /dev/null +++ b/crates/perry-hir/src/lower/property_array_hoist.rs @@ -0,0 +1,391 @@ +//! Hoist a loop-invariant `RECV.PROP` array receiver out of a counted `for`. +//! +//! `for (let i = 0; i < holder.arr.length; i++) l = holder.arr[i];` re-runs a +//! by-name property lookup on **every iteration** — the emitted body carries +//! `js_object_get_field_by_name_f64` plus IC-miss handling — and, worse, the +//! packed-array machinery never engages at all, because its matcher requires +//! the array expression to be a bare local. Measured on a quiet host, that +//! shape costs 20.58 ns/iteration against node's 0.54; writing the hoist by +//! hand (`const a = holder.arr;`) drops it to 0.50, i.e. parity. This pass +//! performs that rewrite when it is observationally equivalent. +//! +//! Equivalence rests on three conditions, all checked before rewriting: +//! +//! 1. **The property is a data field, not an accessor.** Only receivers whose +//! static type is a class this module synthesized for a closed-shape object +//! literal (`__AnonShape_*`) qualify. `is_closed_shape` rejects getters and +//! setters outright, so such a class's fields are data by construction and +//! reading one is side-effect free — hoisting cannot change how many times +//! user code runs. +//! 2. **Nothing in the loop can rebind the receiver.** A `holder = other` +//! assignment would leave the hoisted temp pointing at the previous +//! object's array. Note no shape- or identity-based runtime check can +//! recover this: two objects from the same literal share a shape, so the +//! rewrite must simply refuse. +//! 3. **Nothing in the loop can write the property or call anything.** A call +//! could assign `holder.arr` behind our back, and a direct write is visible +//! syntactically. The scan below rejects calls, closures, `new`, property +//! and index writes, and anything it does not positively recognise. +//! +//! Condition 3 is deliberately stricter than necessary; it matches the shape +//! the packed-array machinery admits anyway, which is exactly where the win +//! is, and it keeps this pass from having to reason about aliasing. + +use crate::ir::{Expr, Stmt}; +use crate::lower::LoweringContext; +use crate::types::Type; + +/// Rewrites `condition`/`body` to read a hoisted local and returns the `Let` +/// that materialises it, or `None` when the loop does not qualify. +pub(crate) fn hoist_loop_invariant_property_array( + ctx: &mut LoweringContext, + condition: &Expr, + update: Option<&Expr>, + body: &[Stmt], +) -> Option<(Stmt, Expr, Vec)> { + let (recv_id, property) = counted_loop_property_array(condition)?; + if !property_is_anon_shape_data_field(ctx, recv_id, &property) { + return None; + } + if !loop_is_hoist_safe(condition, update, body, recv_id) { + return None; + } + // Reads of `RECV.PROP` must actually occur in the body, or the rewrite + // moves work without removing any. + if !body.iter().any(|stmt| stmt_reads_property(stmt, recv_id, &property)) { + return None; + } + + let element_ty = ctx + .closed_shape_literal_locals + .get(&recv_id) + .cloned() + .and_then(|class_name| anon_shape_field_type(ctx, &class_name, &property)) + .or_else(|| match ctx.lookup_local_type_by_id(recv_id) { + Some(Type::Object(obj)) => obj.properties.get(&property).map(|p| p.ty.clone()), + _ => None, + }) + .unwrap_or(Type::Any); + + let hoist_id = ctx.define_local(format!("__perry_hoist_{property}"), element_ty.clone()); + ctx.immutable_locals.insert(hoist_id); + + let init = Expr::PropertyGet { + object: Box::new(Expr::LocalGet(recv_id)), + property: property.clone(), + byte_offset: 0, + }; + let hoist = Stmt::Let { + id: hoist_id, + name: format!("__perry_hoist_{property}"), + ty: element_ty, + mutable: false, + init: Some(init), + }; + + let new_condition = rewrite_expr(condition, recv_id, &property, hoist_id); + let new_body = body + .iter() + .map(|stmt| rewrite_stmt(stmt, recv_id, &property, hoist_id)) + .collect(); + Some((hoist, new_condition, new_body)) +} + +/// `i < RECV.PROP.length` — returns `(RECV, PROP)`. +fn counted_loop_property_array(condition: &Expr) -> Option<(u32, String)> { + let Expr::Compare { op, right, .. } = condition else { + return None; + }; + if !matches!(op, crate::ir::CompareOp::Lt | crate::ir::CompareOp::Le) { + return None; + } + let Expr::PropertyGet { + object, property, .. + } = right.as_ref() + else { + return None; + }; + if property != "length" { + return None; + } + let Expr::PropertyGet { + object: recv, + property: array_prop, + .. + } = object.as_ref() + else { + return None; + }; + match recv.as_ref() { + Expr::LocalGet(id) => Some((*id, array_prop.clone())), + _ => None, + } +} + +/// Condition 1: the receiver's static type is a synthesized closed-shape +/// literal class, whose members are data fields by construction. +fn property_is_anon_shape_data_field( + ctx: &LoweringContext, + recv_id: u32, + property: &str, +) -> bool { + // Keyed on the INITIALIZER, not the binding's type. A getter-bearing + // literal infers as `Any` so a type check would happen to reject it, but + // an annotated structural object type can still be backed by an accessor — + // only "this binding was initialized by a closed-shape literal" actually + // proves the read is side-effect free. + let Some(class_name) = ctx.closed_shape_literal_locals.get(&recv_id) else { + return false; + }; + ctx.anon_shape_fields + .get(class_name) + .is_some_and(|fields| fields.iter().any(|field| field == property)) +} + +fn anon_shape_field_type( + ctx: &LoweringContext, + class_name: &str, + property: &str, +) -> Option { + let idx = *ctx.classes_index.get(class_name)?; + ctx.pending_classes + .get(idx) + .or_else(|| ctx.pending_classes.iter().find(|c| c.name == class_name)) + .and_then(|class| { + class + .fields + .iter() + .find(|field| field.name == property) + .map(|field| field.ty.clone()) + }) +} + +/// Conditions 2 and 3 over the whole loop. +fn loop_is_hoist_safe( + condition: &Expr, + update: Option<&Expr>, + body: &[Stmt], + recv_id: u32, +) -> bool { + expr_is_hoist_safe(condition, recv_id) + && update.is_none_or(|expr| expr_is_hoist_safe(expr, recv_id)) + && body.iter().all(|stmt| stmt_is_hoist_safe(stmt, recv_id)) +} + +fn stmt_is_hoist_safe(stmt: &Stmt, recv_id: u32) -> bool { + match stmt { + Stmt::Expr(expr) => expr_is_hoist_safe(expr, recv_id), + Stmt::Let { id, init, .. } => { + *id != recv_id + && init + .as_ref() + .is_none_or(|expr| expr_is_hoist_safe(expr, recv_id)) + } + Stmt::If { + condition, + then_branch, + else_branch, + } => { + expr_is_hoist_safe(condition, recv_id) + && then_branch.iter().all(|s| stmt_is_hoist_safe(s, recv_id)) + && else_branch + .as_ref() + .is_none_or(|b| b.iter().all(|s| stmt_is_hoist_safe(s, recv_id))) + } + Stmt::Break | Stmt::Continue => true, + _ => false, + } +} + +fn expr_is_hoist_safe(expr: &Expr, recv_id: u32) -> bool { + match expr { + // Condition 2: never let the receiver be rebound. + Expr::LocalSet(id, value) => *id != recv_id && expr_is_hoist_safe(value, recv_id), + Expr::Update { id, .. } => *id != recv_id, + Expr::LocalGet(_) | Expr::Number(_) | Expr::Integer(_) | Expr::String(_) + | Expr::Bool(_) | Expr::Null | Expr::Undefined => true, + Expr::PropertyGet { object, .. } => expr_is_hoist_safe(object, recv_id), + Expr::IndexGet { object, index } => { + expr_is_hoist_safe(object, recv_id) && expr_is_hoist_safe(index, recv_id) + } + Expr::Binary { left, right, .. } | Expr::Logical { left, right, .. } => { + expr_is_hoist_safe(left, recv_id) && expr_is_hoist_safe(right, recv_id) + } + Expr::Compare { left, right, .. } => { + expr_is_hoist_safe(left, recv_id) && expr_is_hoist_safe(right, recv_id) + } + Expr::Unary { operand, .. } => expr_is_hoist_safe(operand, recv_id), + Expr::NumberCoerce(inner) | Expr::BooleanCoerce(inner) => { + expr_is_hoist_safe(inner, recv_id) + } + Expr::Conditional { + condition, + then_expr, + else_expr, + } => { + expr_is_hoist_safe(condition, recv_id) + && expr_is_hoist_safe(then_expr, recv_id) + && expr_is_hoist_safe(else_expr, recv_id) + } + // Condition 3: anything that could call, allocate, or store is out, + // as is anything this pass does not positively recognise. + _ => false, + } +} + +fn stmt_reads_property(stmt: &Stmt, recv_id: u32, property: &str) -> bool { + match stmt { + Stmt::Expr(expr) => expr_reads_property(expr, recv_id, property), + Stmt::Let { init, .. } => init + .as_ref() + .is_some_and(|expr| expr_reads_property(expr, recv_id, property)), + Stmt::If { + condition, + then_branch, + else_branch, + } => { + expr_reads_property(condition, recv_id, property) + || then_branch + .iter() + .any(|s| stmt_reads_property(s, recv_id, property)) + || else_branch.as_ref().is_some_and(|b| { + b.iter().any(|s| stmt_reads_property(s, recv_id, property)) + }) + } + _ => false, + } +} + +fn expr_reads_property(expr: &Expr, recv_id: u32, property: &str) -> bool { + if is_target_property(expr, recv_id, property) { + return true; + } + match expr { + Expr::PropertyGet { object, .. } => expr_reads_property(object, recv_id, property), + Expr::IndexGet { object, index } => { + expr_reads_property(object, recv_id, property) + || expr_reads_property(index, recv_id, property) + } + Expr::Binary { left, right, .. } + | Expr::Logical { left, right, .. } + | Expr::Compare { left, right, .. } => { + expr_reads_property(left, recv_id, property) + || expr_reads_property(right, recv_id, property) + } + Expr::Unary { operand, .. } => expr_reads_property(operand, recv_id, property), + Expr::NumberCoerce(inner) | Expr::BooleanCoerce(inner) => { + expr_reads_property(inner, recv_id, property) + } + Expr::LocalSet(_, value) => expr_reads_property(value, recv_id, property), + Expr::Conditional { + condition, + then_expr, + else_expr, + } => { + expr_reads_property(condition, recv_id, property) + || expr_reads_property(then_expr, recv_id, property) + || expr_reads_property(else_expr, recv_id, property) + } + _ => false, + } +} + +fn is_target_property(expr: &Expr, recv_id: u32, property: &str) -> bool { + matches!( + expr, + Expr::PropertyGet { object, property: prop, .. } + if prop == property && matches!(object.as_ref(), Expr::LocalGet(id) if *id == recv_id) + ) +} + +fn rewrite_stmt(stmt: &Stmt, recv_id: u32, property: &str, hoist_id: u32) -> Stmt { + match stmt { + Stmt::Expr(expr) => Stmt::Expr(rewrite_expr(expr, recv_id, property, hoist_id)), + Stmt::Let { + id, + name, + ty, + mutable, + init, + } => Stmt::Let { + id: *id, + name: name.clone(), + ty: ty.clone(), + mutable: *mutable, + init: init + .as_ref() + .map(|expr| rewrite_expr(expr, recv_id, property, hoist_id)), + }, + Stmt::If { + condition, + then_branch, + else_branch, + } => Stmt::If { + condition: rewrite_expr(condition, recv_id, property, hoist_id), + then_branch: then_branch + .iter() + .map(|s| rewrite_stmt(s, recv_id, property, hoist_id)) + .collect(), + else_branch: else_branch.as_ref().map(|b| { + b.iter() + .map(|s| rewrite_stmt(s, recv_id, property, hoist_id)) + .collect() + }), + }, + other => other.clone(), + } +} + +fn rewrite_expr(expr: &Expr, recv_id: u32, property: &str, hoist_id: u32) -> Expr { + if is_target_property(expr, recv_id, property) { + return Expr::LocalGet(hoist_id); + } + let rec = |e: &Expr| Box::new(rewrite_expr(e, recv_id, property, hoist_id)); + match expr { + Expr::PropertyGet { + object, + property: prop, + byte_offset, + } => Expr::PropertyGet { + object: rec(object), + property: prop.clone(), + byte_offset: *byte_offset, + }, + Expr::IndexGet { object, index } => Expr::IndexGet { + object: rec(object), + index: rec(index), + }, + Expr::Binary { op, left, right } => Expr::Binary { + op: *op, + left: rec(left), + right: rec(right), + }, + Expr::Logical { op, left, right } => Expr::Logical { + op: *op, + left: rec(left), + right: rec(right), + }, + Expr::Compare { op, left, right } => Expr::Compare { + op: *op, + left: rec(left), + right: rec(right), + }, + Expr::Unary { op, operand } => Expr::Unary { + op: *op, + operand: rec(operand), + }, + Expr::NumberCoerce(inner) => Expr::NumberCoerce(rec(inner)), + Expr::BooleanCoerce(inner) => Expr::BooleanCoerce(rec(inner)), + Expr::LocalSet(id, value) => Expr::LocalSet(*id, rec(value)), + Expr::Conditional { + condition, + then_expr, + else_expr, + } => Expr::Conditional { + condition: rec(condition), + then_expr: rec(then_expr), + else_expr: rec(else_expr), + }, + other => other.clone(), + } +} diff --git a/crates/perry-hir/src/lower_decl/body_stmt.rs b/crates/perry-hir/src/lower_decl/body_stmt.rs index 56e955f02d..b48d594e7d 100644 --- a/crates/perry-hir/src/lower_decl/body_stmt.rs +++ b/crates/perry-hir/src/lower_decl/body_stmt.rs @@ -873,13 +873,39 @@ pub fn lower_body_stmt(ctx: &mut LoweringContext, stmt: &ast::Stmt) -> Result { + result.push(hoist); + result.push(Stmt::For { + init, + condition: Some(new_condition), + update, + body: new_body, + }); + } + None => result.push(Stmt::For { + init, + condition, + update, + body, + }), + } } ast::Stmt::Try(try_stmt) => { // try body is its own lexical scope From 745ce2050fae4c0a98d7d681889e90382cf3142b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 11:30:38 +0200 Subject: [PATCH 2/3] test(hir): pin the property-array hoist and add its kill switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PERRY_LOOP_PROPERTY_HOIST=0 restores the pre-hoist lowering, which makes the pass A/B-able on one build and switchable off in the field. The test compiles each program twice, with the hoist on and off, and asserts both print the same thing — running the kill-switch build against the same expectation is what makes it a test of equivalence rather than a test of the hoisted path only. Both builds run under PERRY_GC_FORCE_EVACUATE, since the claim that the hoisted temp is GC-tracked like any other local is worth proving rather than assuming. Most of the cases pin refusals, which is the half a regression would break silently: receiver rebound mid-loop to a SAME-SHAPED object (no runtime shape check could catch it, so the refusal has to be syntactic), a call that overwrites the property, a getter receiver whose five invocations must all survive, and a direct write to the property inside the loop. Plus an array grown during iteration, nested loops, string elements and an empty array. Every expected value was checked against node first. Measured on the dev box with the switch, the same binary either way: module-global receiver 56.05 -> 0.52 ns/op, local receiver 7.65 -> 0.49. size(1) .text: 10963348 -> 10963028, i.e. -320 bytes (-0.003%) over 2 hoisted sites, about -160 bytes per site: the pass replaces a per-iteration by-name lookup and its IC-miss path with one load, so it removes code rather than adding it. Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p --- .../src/lower/property_array_hoist.rs | 13 ++ .../perry/tests/loop_property_array_hoist.rs | 206 ++++++++++++++++++ 2 files changed, 219 insertions(+) create mode 100644 crates/perry/tests/loop_property_array_hoist.rs diff --git a/crates/perry-hir/src/lower/property_array_hoist.rs b/crates/perry-hir/src/lower/property_array_hoist.rs index 73ebbdeb0a..51e90cc26f 100644 --- a/crates/perry-hir/src/lower/property_array_hoist.rs +++ b/crates/perry-hir/src/lower/property_array_hoist.rs @@ -43,6 +43,9 @@ pub(crate) fn hoist_loop_invariant_property_array( update: Option<&Expr>, body: &[Stmt], ) -> Option<(Stmt, Expr, Vec)> { + if !hoist_enabled() { + return None; + } let (recv_id, property) = counted_loop_property_array(condition)?; if !property_is_anon_shape_data_field(ctx, recv_id, &property) { return None; @@ -91,6 +94,16 @@ pub(crate) fn hoist_loop_invariant_property_array( Some((hoist, new_condition, new_body)) } +/// `PERRY_LOOP_PROPERTY_HOIST=0` restores the pre-hoist lowering, so the pass +/// can be A/B'd on a single build and switched off in the field if a program +/// ever slips past the three equivalence checks. +fn hoist_enabled() -> bool { + !matches!( + std::env::var("PERRY_LOOP_PROPERTY_HOIST").as_deref(), + Ok("0") | Ok("off") | Ok("false") + ) +} + /// `i < RECV.PROP.length` — returns `(RECV, PROP)`. fn counted_loop_property_array(condition: &Expr) -> Option<(u32, String)> { let Expr::Compare { op, right, .. } = condition else { diff --git a/crates/perry/tests/loop_property_array_hoist.rs b/crates/perry/tests/loop_property_array_hoist.rs new file mode 100644 index 0000000000..9a14dce21d --- /dev/null +++ b/crates/perry/tests/loop_property_array_hoist.rs @@ -0,0 +1,206 @@ +//! Coverage for the loop-invariant property-array hoist. +//! +//! `for (let i = 0; i < holder.arr.length; i++) … holder.arr[i] …` reads the +//! property once into a local instead of on every iteration. The rewrite is +//! only sound where the property is provably a data field, the receiver cannot +//! be rebound, and nothing in the loop can write the property — so most of the +//! cases below exist to pin the *refusals*, which are the part a regression +//! would silently break. `PERRY_LOOP_PROPERTY_HOIST=0` restores the old +//! lowering, and every program here must produce identical output either way. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile(dir: &Path, source: &str, hoist: bool) -> PathBuf { + let entry = dir.join("main.ts"); + let output = dir.join(if hoist { "main_on" } else { "main_off" }); + std::fs::write(&entry, source).expect("write entry"); + + let mut cmd = Command::new(perry_bin()); + cmd.current_dir(dir) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1"); + if !hoist { + cmd.env("PERRY_LOOP_PROPERTY_HOIST", "0"); + } + let compile = cmd.output().expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed (hoist={hoist})\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + output +} + +fn run(bin: &Path, dir: &Path) -> Output { + Command::new(bin) + .current_dir(dir) + // The hoisted value is an ordinary local, so it is GC-tracked like any + // other binding; forcing evacuation proves that rather than assuming it. + .env("PERRY_GC_FORCE_EVACUATE", "1") + .env("PERRY_GC_VERIFY_EVACUATION", "1") + .output() + .expect("run compiled binary") +} + +fn stdout_of(bin: &Path, dir: &Path) -> String { + let out = run(bin, dir); + assert!( + out.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + out.status, + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).into_owned() +} + +/// Compiles `source` twice and asserts both builds print `expected`. Running +/// the kill-switch build against the same expectation is what makes this a +/// test of equivalence and not merely of the hoisted path. +fn assert_same_with_and_without_hoist(name: &str, source: &str, expected: &str) { + let dir = tempfile::tempdir().expect("tempdir"); + let on = compile(dir.path(), source, true); + let off = compile(dir.path(), source, false); + let got_on = stdout_of(&on, dir.path()); + let got_off = stdout_of(&off, dir.path()); + assert_eq!(got_on.trim(), expected.trim(), "{name}: hoisted output"); + assert_eq!( + got_off.trim(), + expected.trim(), + "{name}: kill-switch output" + ); +} + +#[test] +fn hoists_a_loop_invariant_property_array() { + assert_same_with_and_without_hoist( + "basic", + r#" + const holder = { arr: [1, 2, 3, 4], n: 4 }; + let s = 0; + for (let i = 0; i < holder.arr.length; i++) s += holder.arr[i]; + console.log(s); + "#, + "10", + ); +} + +#[test] +fn refuses_when_the_receiver_is_rebound_in_the_loop() { + // Both objects share a shape, so no runtime shape check could catch this: + // the rewrite has to refuse it syntactically. A stale array would keep + // reading a1 and print 60 instead of 510. + assert_same_with_and_without_hoist( + "receiver rebound", + r#" + const a1 = { arr: [10, 20, 30], n: 3 }; + const a2 = { arr: [100, 200, 300], n: 3 }; + let h = a1; + let t = 0; + for (let i = 0; i < h.arr.length; i++) { t += h.arr[i]; h = a2; } + console.log(t); + "#, + "510", + ); +} + +#[test] +fn refuses_when_a_call_can_overwrite_the_property() { + assert_same_with_and_without_hoist( + "call overwrite", + r#" + const o: any = { arr: [1, 2, 3] }; + function swap(): number { o.arr = [9, 9, 9]; return 0; } + let u = 0; + for (let i = 0; i < o.arr.length; i++) { u += o.arr[i] + swap(); } + console.log(u); + "#, + "19", + ); +} + +#[test] +fn refuses_a_getter_receiver_and_preserves_invocation_count() { + // A getter runs user code, so collapsing the reads is observable. The + // count matters as much as the sum. + assert_same_with_and_without_hoist( + "getter", + r#" + let reads = 0; + const g = { get arr() { reads++; return [5, 6]; } }; + let v = 0; + for (let i = 0; i < g.arr.length; i++) v += g.arr[i]; + console.log(v + " " + reads); + "#, + "11 5", + ); +} + +#[test] +fn refuses_when_the_property_is_written_directly_in_the_loop() { + assert_same_with_and_without_hoist( + "direct write", + r#" + const o: any = { arr: [1, 2, 3, 4] }; + let s = 0; + for (let i = 0; i < o.arr.length; i++) { + s += o.arr[i]; + if (i === 1) o.arr = [7, 7]; + } + console.log(s); + "#, + "3", + ); +} + +#[test] +fn tracks_an_array_grown_during_iteration() { + // `length` is re-read every iteration by the condition; only the property + // lookup is hoisted, and the array object itself is shared, so pushes must + // still be observed. + assert_same_with_and_without_hoist( + "grown mid-loop", + r#" + const o = { arr: [1, 2, 3] }; + let s = 0; + for (let i = 0; i < o.arr.length; i++) { + s += o.arr[i]; + if (o.arr.length < 6) o.arr.push(10); + } + console.log(s + " " + o.arr.length); + "#, + "36 6", + ); +} + +#[test] +fn handles_nested_loops_and_string_elements() { + assert_same_with_and_without_hoist( + "nested and strings", + r#" + const m = { rows: [[1, 2], [3, 4]] }; + let s = 0; + for (let i = 0; i < m.rows.length; i++) { + const row = m.rows[i]; + for (let j = 0; j < row.length; j++) s += row[j]; + } + const w = { parts: ["a", "b", "c"] }; + let out = ""; + for (let i = 0; i < w.parts.length; i++) out += w.parts[i]; + const empty = { arr: [] as number[] }; + let e = 0; + for (let i = 0; i < empty.arr.length; i++) e += empty.arr[i]; + console.log(s + " " + out + " " + e); + "#, + "10 abc 0", + ); +} From 231dd337b76eae24219d6d28a05ab713f4e9e38d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 12:37:42 +0200 Subject: [PATCH 3/3] fix(perry): register PERRY_LOOP_PROPERTY_HOIST as a build-cache input --- .../src/lower/property_array_hoist.rs | 32 +++++++++---------- .../perry/src/commands/compile/build_cache.rs | 4 +++ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/crates/perry-hir/src/lower/property_array_hoist.rs b/crates/perry-hir/src/lower/property_array_hoist.rs index 51e90cc26f..c818a29b2d 100644 --- a/crates/perry-hir/src/lower/property_array_hoist.rs +++ b/crates/perry-hir/src/lower/property_array_hoist.rs @@ -55,7 +55,10 @@ pub(crate) fn hoist_loop_invariant_property_array( } // Reads of `RECV.PROP` must actually occur in the body, or the rewrite // moves work without removing any. - if !body.iter().any(|stmt| stmt_reads_property(stmt, recv_id, &property)) { + if !body + .iter() + .any(|stmt| stmt_reads_property(stmt, recv_id, &property)) + { return None; } @@ -137,11 +140,7 @@ fn counted_loop_property_array(condition: &Expr) -> Option<(u32, String)> { /// Condition 1: the receiver's static type is a synthesized closed-shape /// literal class, whose members are data fields by construction. -fn property_is_anon_shape_data_field( - ctx: &LoweringContext, - recv_id: u32, - property: &str, -) -> bool { +fn property_is_anon_shape_data_field(ctx: &LoweringContext, recv_id: u32, property: &str) -> bool { // Keyed on the INITIALIZER, not the binding's type. A getter-bearing // literal infers as `Any` so a type check would happen to reject it, but // an annotated structural object type can still be backed by an accessor — @@ -155,11 +154,7 @@ fn property_is_anon_shape_data_field( .is_some_and(|fields| fields.iter().any(|field| field == property)) } -fn anon_shape_field_type( - ctx: &LoweringContext, - class_name: &str, - property: &str, -) -> Option { +fn anon_shape_field_type(ctx: &LoweringContext, class_name: &str, property: &str) -> Option { let idx = *ctx.classes_index.get(class_name)?; ctx.pending_classes .get(idx) @@ -215,8 +210,13 @@ fn expr_is_hoist_safe(expr: &Expr, recv_id: u32) -> bool { // Condition 2: never let the receiver be rebound. Expr::LocalSet(id, value) => *id != recv_id && expr_is_hoist_safe(value, recv_id), Expr::Update { id, .. } => *id != recv_id, - Expr::LocalGet(_) | Expr::Number(_) | Expr::Integer(_) | Expr::String(_) - | Expr::Bool(_) | Expr::Null | Expr::Undefined => true, + Expr::LocalGet(_) + | Expr::Number(_) + | Expr::Integer(_) + | Expr::String(_) + | Expr::Bool(_) + | Expr::Null + | Expr::Undefined => true, Expr::PropertyGet { object, .. } => expr_is_hoist_safe(object, recv_id), Expr::IndexGet { object, index } => { expr_is_hoist_safe(object, recv_id) && expr_is_hoist_safe(index, recv_id) @@ -261,9 +261,9 @@ fn stmt_reads_property(stmt: &Stmt, recv_id: u32, property: &str) -> bool { || then_branch .iter() .any(|s| stmt_reads_property(s, recv_id, property)) - || else_branch.as_ref().is_some_and(|b| { - b.iter().any(|s| stmt_reads_property(s, recv_id, property)) - }) + || else_branch + .as_ref() + .is_some_and(|b| b.iter().any(|s| stmt_reads_property(s, recv_id, property))) } _ => false, } diff --git a/crates/perry/src/commands/compile/build_cache.rs b/crates/perry/src/commands/compile/build_cache.rs index 5bcd50918e..dd03eaa531 100644 --- a/crates/perry/src/commands/compile/build_cache.rs +++ b/crates/perry/src/commands/compile/build_cache.rs @@ -62,6 +62,10 @@ const BUILD_CACHE_ENV_VARS: &[&str] = &[ // settings emit different call sequences, so a cached object from one // must not serve the other. "PERRY_METHOD_INLINE_PROBE", + // #9149: gates hoisting a loop-invariant property-array receiver out of + // counted for-loops — on and off emit different loop bodies, so a cached + // object from one must not serve the other. + "PERRY_LOOP_PROPERTY_HOIST", // #9122: gates the shape-cache path for object literals whose methods // capture `this` — on and off emit different literal-birth sequences, // so a cached object from one must not serve the other.