perf: don't unroll loops whose body contains a function literal (method slice B1)

changelog.d/method-literal-one-function-in-unrolled-loops.md

@@ -0,0 +1,9 @@
+### Performance
+
+The static loop unroller no longer clones a loop body that contains a function
+literal (a closure, an arrow, an object-literal method or a class expression).
+Each copy used to become its own compiled function, so objects built in a short
+counted loop such as `for (let i = 0; i < 8; i++) objs.push({ m() { ... } })`
+carried eight different code pointers for one source method, and a method call
+site over them primed past its ways and went megamorphic. One source function
+literal is now one function, and the site keeps a single entry.

changelog.d/method-receiver-parity-tests.md

@@ -0,0 +1,7 @@
+### Tests
+
+Two node-parity tests pin the receiver a method body sees: sloppy `this` is
+bound once per activation (one wrapper for a primitive, `globalThis` for
+nullish, objects unchanged), and object-literal and prototype methods called
+through a method site get their receiver on every route (nested calls, arrows,
+throws, `call`/`apply`, getters, constructors, generators and async methods).

crates/perry-transform/src/unroll/mod.rs

@@ -43,13 +43,16 @@
 //!   unrolled stmts. Out of scope for v1.
 //! - **`Stmt::Labeled`** — same reason; the label is loop-scoped and
 //!   would alias with siblings post-unroll.
-//! - **Closures capturing the IV** — each iteration needs to capture a
-//!   different value of `i`, but unrolling produces stmts at the caller
-//!   scope where `i` no longer exists. Substituting `LocalGet(i)` to
-//!   `Integer(N)` inside the closure body works only for closures that
-//!   capture-by-value at construction time AND aren't called after the
-//!   IV's loop-scope ends. Conservative: reject all closures referencing
-//!   the IV.
+//! - **Function literals** (closures, arrows, object-literal and
+//!   prototype-literal methods, class expressions) — one source function
+//!   literal must stay ONE function. Cloning the body would give every copy
+//!   its own compiled body (a fresh FuncId, #456), so a receiver built in
+//!   the loop carries a different code pointer per iteration and every
+//!   identity-keyed memo on it (method sites, call-site feedback) sees N
+//!   functions where the program has one: `for (let i = 0; i < 8; i++)
+//!   objs.push({ m() {} })` latched its method site megamorphic. The
+//!   unroller exists to fold constant indices into kernels, which never
+//!   create functions per iteration.
 //! - **`LocalSet(i, ...)` or `Update { id: i }` inside body** — user is
 //!   manually mutating the IV; unrolling would lose those writes.
 //!   Allowed only in the for-loop's own `update` slot (by definition).
@@ -502,8 +505,7 @@ fn try_unroll_for(
     //      * codegen emits one `@perry_global_*__<id>` per module-init
     //        Stmt::Let with a referenced id, and N copies of the same
     //        id cause LLVM duplicate-global errors (issue #456); and
-    //      * each iteration's `() => captured` closure is supposed to
-    //        bind a distinct value, which requires distinct capture ids.
+    //      * a later read of a per-copy `let` must see its own copy.
     let mut out: Vec<Stmt> = Vec::with_capacity((trips as usize) * body.len());
     for n in 0..trips {
         let value = lo + n;
@@ -625,31 +627,9 @@ fn expr_is_unrollable(e: &Expr, iv_id: LocalId) -> bool {
     match e {
         Expr::LocalSet(id, _) if *id == iv_id => return false,
         Expr::Update { id, .. } if *id == iv_id => return false,
-        // Closures: reject any closure that even mentions the IV. A
-        // closure captured-by-value at construction would semantically
-        // freeze the IV's current value, but our HIR captures are by
-        // ID; substituting LocalGet(iv) → Integer(N) inside the
-        // closure body works only if the closure isn't called outside
-        // the IV's live range. The image_convolution kernel doesn't
-        // create closures inside its blur loops, so this restriction
-        // is free for the target workload.
-        Expr::Closure { body, captures, .. } => {
-            if captures.contains(&iv_id) {
-                return false;
-            }
-            // Defensive: walk the closure body to catch any direct
-            // `LocalGet(iv_id)` reference that wasn't materialized as a
-            // capture entry (shouldn't happen in well-formed HIR, but
-            // checking is cheap). Closure body's break/continue are
-            // always lexically scoped to a loop *inside* the closure
-            // (free `break` outside a loop is a JS syntax error), so we
-            // start at loop_depth=1 to suppress the always-true Break/
-            // Continue rejection.
-            if !body.iter().all(|s| stmt_is_unrollable(s, iv_id, 1)) {
-                return false;
-            }
-            return true;
-        }
+        // A function literal is refused whatever it captures: the copies
+        // would each be a distinct function (see the module doc).
+        Expr::Closure { .. } | Expr::ClassExprFresh { .. } => return false,
         _ => {}
     }
     // Recurse into all sub-expressions.
@@ -1038,12 +1018,12 @@ fn refresh_in_expr(
             mutable_captures,
             ..
         } => {
-            // Each cloned closure must get its own FuncId. Codegen keys
-            // compiled functions by FuncId, so two cloned `() => captured`
-            // closures sharing one FuncId would collapse into a single
-            // compiled function — every iteration's `fns[i]()` would then
-            // read the same global. Bumping FuncId per clone keeps each
-            // closure on its own compiled body.
+            // `expr_is_unrollable` refuses bodies that contain a function
+            // literal, so an unrolled body must not be assumed to reach
+            // here; the arm keeps the refresher total for any caller. If a
+            // closure is cloned, each clone needs its own FuncId: codegen
+            // keys compiled functions by FuncId, so two clones sharing one
+            // would collapse into a single compiled function.
             *func_id = *next_func_id;
             *next_func_id = next_func_id.saturating_add(1);
 
@@ -1596,10 +1576,10 @@ mod tests {
     /// disturb.
     #[test]
     fn loop_local_let_still_refreshed_per_copy() {
-        // for (let i = 0; i < 3; i++) { let x = i; fns.push(() => x); }
+        // for (let i = 0; i < 3; i++) { let x = i; xs.push(x); }
         let i = 1u32;
         let x = 2u32;
-        let fns = 3u32; // declared outside the loop (not refreshed)
+        let xs = 3u32; // declared outside the loop (not refreshed)
         let body = vec![
             Stmt::Let {
                 id: x,
@@ -1609,22 +1589,8 @@ mod tests {
                 init: Some(ivar(i)),
             },
             Stmt::Expr(Expr::ArrayPush {
-                array_id: fns,
-                value: Box::new(Expr::Closure {
-                    func_id: 0,
-                    params: vec![],
-                    return_type: Type::Number,
-                    body: vec![Stmt::Return(Some(Expr::LocalGet(x)))],
-                    captures: vec![x],
-                    mutable_captures: vec![],
-                    captures_this: false,
-                    captures_new_target: false,
-                    enclosing_class: None,
-                    is_arrow: false,
-                    is_strict: false,
-                    is_async: false,
-                    is_generator: false,
-                }),
+                array_id: xs,
+                value: Box::new(Expr::LocalGet(x)),
                 field_writeback: None,
             }),
         ];
@@ -1635,7 +1601,7 @@ mod tests {
         assert!(changed, "expected unroll to fire");
         assert_eq!(stmts.len(), 6, "3 trips * 2 body stmts");
         // Collect the `let x` id of each copy — they must all be DISTINCT
-        // (refreshed), and the closure in each copy must capture its own id.
+        // (refreshed), and the push in each copy must read its own id.
         let mut let_ids = Vec::new();
         for pair in stmts.chunks(2) {
             let decl_id = match &pair[0] {
@@ -1644,10 +1610,10 @@ mod tests {
             };
             match &pair[1] {
                 Stmt::Expr(Expr::ArrayPush { value, .. }) => match value.as_ref() {
-                    Expr::Closure { captures, .. } => {
-                        assert_eq!(captures, &vec![decl_id], "closure captures its copy's x");
+                    Expr::LocalGet(id) => {
+                        assert_eq!(*id, decl_id, "the push reads its copy's x");
                     }
-                    other => panic!("expected Closure, got {:?}", other),
+                    other => panic!("expected LocalGet, got {:?}", other),
                 },
                 other => panic!("expected ArrayPush, got {:?}", other),
             }
@@ -1662,6 +1628,40 @@ mod tests {
         );
     }
 
+    /// Sabotage: allow a closure that does not mention the IV -> this is red.
+    /// A function literal in the body would be cloned into one function per
+    /// copy, splitting the identity every method-site memo keys on.
+    #[test]
+    fn rejects_loop_with_a_function_literal_that_ignores_the_iv() {
+        // for (let i = 0; i < 3; i++) { fns.push(function () { return 1; }); }
+        let i = 1u32;
+        let fns = 2u32;
+        let body = vec![Stmt::Expr(Expr::ArrayPush {
+            array_id: fns,
+            value: Box::new(Expr::Closure {
+                func_id: 0,
+                params: vec![],
+                return_type: Type::Number,
+                body: vec![Stmt::Return(Some(integer(1)))],
+                captures: vec![],
+                mutable_captures: vec![],
+                captures_this: false,
+                captures_new_target: false,
+                enclosing_class: None,
+                is_arrow: false,
+                is_strict: false,
+                is_async: false,
+                is_generator: false,
+            }),
+            field_writeback: None,
+        })];
+        let f = make_for(i, 0, 3, body, CompareOp::Lt);
+        assert!(
+            try_unroll(&f).is_none(),
+            "a function literal must not be cloned"
+        );
+    }
+
     #[test]
     fn unrolled_local_copies_keep_their_source_span() {
         let i = 1u32;

crates/perry/tests/method_site.rs

@@ -453,3 +453,30 @@ console.log(s, t);
          (own={own} inherited={inherited} misses={misses})"
     );
 }
+
+/// Sabotage: let the static unroller clone a loop body holding a function
+/// literal -> the 8 copies are 8 code pointers and the site primes past its
+/// ways and latches megamorphic.
+#[test]
+fn a_method_literal_built_in_a_short_counted_loop_is_one_function() {
+    let (stdout, own, inherited, misses) = run(r#"const N = process.argv.length > 99 ? 1 : 4000;
+const objs: any[] = [];
+function build() { for (let i = 0; i < 8; i++) objs.push({ a: i, b: 2, m() { return this.a; } }); }
+build();
+function run(n: number): number {
+  let h = 0;
+  for (let k = 0; k < n; k++) h += objs[k & 7].m();
+  return h;
+}
+console.log(run(N), objs[0].m === objs[7].m);
+"#);
+    // One source literal is one function: node prints `false` for the
+    // identity (each evaluation is a fresh function object) but every object
+    // shares one body, so the site keeps one entry for all 8 receivers.
+    assert_eq!(stdout, "14000 false");
+    assert!(
+        inherited == 0 && own <= 2 && misses <= 4,
+        "8 objects from one method literal must share one site entry \
+         (own={own} inherited={inherited} misses={misses})"
+    );
+}

test-files/test_gap_method_site_receiver.ts

@@ -0,0 +1,66 @@
+// Object-literal and prototype methods called through a method site receive
+// their receiver as `this`: nested method calls, arrows inheriting `this`, a
+// throw through a method, extra/missing arguments, `arguments`, the generic
+// path (call/apply/detached), an inherited method, a getter, a function
+// expression called and constructed, generator/async methods, and a strict
+// function given a primitive. Output must match node.
+const N = process.argv.length > 99 ? 1 : 3000;
+function plainFn(this: any) { return this === globalThis; }
+function mk(k: number): any {
+  return {
+    k,
+    m(x: number) { return x + this.k; },
+    outer(x: number) { const r = this.inner(x); return r + this.k; },
+    inner(x: number) { return x * 2 + this.k; },
+    thrower(x: number) { if (x < 0) throw new Error("neg" + this.k); return this.k; },
+    wrap(x: number) { try { this.thrower(-x); } catch (e) { return this.k * 100; } return this.k; },
+    arrow(x: number) { const f = () => this.k + x; return f(); },
+    plain(x: number) { return plainFn() ? x + this.k : -1; },
+    many(a: number, b: number, c: number, d: number) { return this.k + (b === undefined ? 0 : b) + (d === undefined ? 7 : d); },
+    args(x: number) { return arguments.length + this.k; },
+  };
+}
+// Generator and async methods live on their own literal.
+function mkg(k: number): any {
+  return { k, *gen() { yield this.k; yield this.k + 1; }, async am() { await null; return this.k * 3; } };
+}
+const a = mk(1);
+const b = mk(2);
+let s = 0, t = 0, v = 0, w = 0, y = 0, z = 0, q = 0, caught = 0;
+for (let i = 0; i < N; i++) {
+  const o = (i & 1) ? a : b;
+  s += o.m(i);
+  t += o.outer(i);
+  v += o.arrow(i);
+  w += o.plain(i);
+  y += o.wrap(i + 1);
+  z += o.many(i, 1);
+  q += o.args(i, i);
+  try { o.thrower(i % 7 === 0 ? -1 : i); } catch (e) { caught++; }
+}
+console.log(s, t, v, w, y, z, q, caught);
+// The generic path (no site): call/apply/detached, through the public body.
+const f = a.m;
+console.log(a.m.call(b, 10), a.m.apply({ k: 40 }, [2]), f.call({ k: 7 }, 1), a.outer.call(b, 3));
+// An inherited method through a site.
+const proto: any = { m(x: number) { return x * 3 + this.k; } };
+const kids: any[] = [];
+for (let j = 0; j < 4; j++) { const c = Object.create(proto); c.k = j; kids.push(c); }
+let r = 0;
+for (let i = 0; i < N; i++) r += kids[i & 3].m(i);
+console.log(r);
+// A getter, and a function expression stored as a method, called and constructed.
+const g: any = { k: 3, get dbl() { return this.k * 2; } };
+let gs = 0;
+for (let i = 0; i < N; i++) gs += g.dbl;
+const holder: any = { k: 9 };
+holder.F = function (this: any, x: number) { this.x = x; return this; };
+let fs = 0;
+for (let i = 0; i < N; i++) fs += holder.F(i).k;
+const made = new holder.F(4);
+console.log(gs, fs, holder.x, made.x, made === holder, made instanceof holder.F);
+// Generator and async methods keep their receiver.
+const ga = mkg(1);
+const gb = mkg(2);
+console.log(JSON.stringify([...ga.gen()]), JSON.stringify([...gb.gen()]));
+ga.am().then((x: number) => console.log("async", x));

test-files/test_gap_sloppy_this_bound_once.ts

@@ -0,0 +1,90 @@
+// Sloppy-mode `this` is bound ONCE per activation (OrdinaryCallBindThis):
+// undefined/null become globalThis, a primitive becomes ONE wrapper object,
+// objects pass unchanged. Every `this` in one activation — and every arrow
+// that inherits it — must name that same value: `this === this` holds for a
+// primitive receiver. Covers function declarations, nested declarations,
+// function expressions, object-literal methods, generators, async functions,
+// class references, and a "use strict" function in a sloppy file.
+function kind(this: any) { return typeof this; }
+function same(this: any) { return this === this; }
+function viaArrow(this: any) { const a = () => this; return a() === this && a() === a(); }
+function tagOf(this: any) { return Object.prototype.toString.call(this); }
+function isGlobal(this: any) { return this === globalThis; }
+function keep(this: any) { return this; }
+function twice(this: any) { const x = this; const y = this; return x === y; }
+function mutate(this: any) { this.extra = 1; return this.extra; }
+
+const receivers: any[] = [1, 2.5, -0, NaN, true, false, "s", "", 10n];
+for (const r of receivers) {
+  const label = typeof r === "bigint" ? "bigint" : JSON.stringify(r);
+  console.log(
+    "decl", label, kind.call(r), same.call(r), viaArrow.call(r), tagOf.call(r),
+    twice.call(r), mutate.call(r), keep.call(r) === keep.call(r),
+  );
+}
+console.log("nullish", isGlobal.call(undefined), isGlobal.call(null), isGlobal(), kind.call(undefined));
+const o = { a: 1 };
+console.log("object", keep.call(o) === o, same.call(o), kind.call(o));
+const fnRecv = function () { return 3; };
+console.log("function", keep.call(fnRecv) === fnRecv, kind.call(fnRecv));
+
+// Nested declaration and a function expression.
+function outer(this: any) {
+  function inner(this: any) { return [typeof this, this === this, this instanceof Number]; }
+  return inner.call(this);
+}
+console.log("nested", JSON.stringify(outer.call(7)), JSON.stringify(outer.call(o)));
+const expr = function (this: any) { return [typeof this, this === this, this instanceof String]; };
+console.log("expr", JSON.stringify(expr.call("x")), JSON.stringify(expr.call(4)));
+
+// Object-literal method called with a primitive.
+const lit: any = { m() { return [typeof this, this === this, this instanceof Boolean]; } };
+console.log("literal", JSON.stringify(lit.m.call(true)), JSON.stringify(lit.m.call(9)));
+
+// A method call ON a primitive: the receiver reaches the body unboxed, and
+// the body binds it once.
+(Number.prototype as any).kindOf = function (this: any) { return [typeof this, this === this, this instanceof Number]; };
+(String.prototype as any).kindOf = function (this: any) { const a = () => this; return [typeof this, this === a(), this.length]; };
+(Boolean.prototype as any).kindOf = function (this: any) { return [typeof this, this === this, this.valueOf()]; };
+const five: any = 5;
+const str: any = "abc";
+const yes: any = true;
+console.log("proto-method", JSON.stringify(five.kindOf()), JSON.stringify(str.kindOf()), JSON.stringify(yes.kindOf()));
+let hot = 0;
+for (let i = 0; i < 3000; i++) { const r = (i as any).kindOf(); if (r[0] === "object" && r[1] && r[2]) hot++; }
+console.log("proto-method-hot", hot);
+
+// A detached function expression / method called plainly binds globalThis.
+const detachedExpr = function (this: any) { return this === globalThis; };
+const detachedLit: any = { m() { return this === globalThis; } };
+const dm = detachedLit.m;
+console.log("detached", detachedExpr(), dm(), typeof (function (this: any) { return this; })());
+
+// A primitive receiver's wrapper is a fresh object per ACTIVATION.
+console.log("per-activation", keep.call(5) === keep.call(5), keep.call(5) == keep.call(5));
+console.log("value", keep.call(5).valueOf(), keep.call("ab").length, keep.call(true).valueOf());
+
+// Class reference receiver stays the class.
+class C { static tag = "C"; }
+function readTag(this: any) { return this === C ? this.tag : "boxed"; }
+console.log("classref", readTag.call(C), keep.call(C) === C);
+
+// Generators and async functions bind their receiver the same way.
+function* gen(this: any) { yield typeof this; yield this === this; const a = () => this; yield a() === this; }
+console.log("generator", JSON.stringify([...gen.call(3)]), JSON.stringify([...gen.call("q")]));
+async function af(this: any) { const before = this; await null; return [typeof this, this === before, this === this]; }
+af.call(8).then((v) => console.log("async", JSON.stringify(v)));
+
+// A "use strict" function in a sloppy file sees the primitive itself.
+function strictOne(this: any) { "use strict"; return [typeof this, this === this, this === 6]; }
+function strictKeep(this: any) { "use strict"; return this; }
+console.log("strict", JSON.stringify(strictOne.call(6)), strictKeep.call(undefined) === undefined, strictKeep.call("z") === "z");
+
+// Many activations under allocation pressure keep their own wrappers.
+let ok = 0;
+for (let i = 0; i < 20000; i++) {
+  const w = keep.call(i);
+  const junk = { i, s: "x" + i };
+  if (typeof w === "object" && w.valueOf() === i && same.call(i) && junk.i === i) ok++;
+}
+console.log("pressure", ok);