perf(runtime): skip the arg-combine Vec for no-partial-args bound callbacks (−4.6% CPU)

changelog.d/10818-callback-bound-dispatch-skip-vec-alloc.md

@@ -0,0 +1,76 @@
+Removed a per-call heap allocation from `dispatch_bound_function`
+(`crates/perry-runtime/src/closure/dispatch/bound.rs`), the runtime entry
+every `js_closure_call<N>` routes a `Function.prototype.bind` result through.
+
+The function unconditionally built a `Vec<f64>` (`Vec::with_capacity(args.len()
++ 4)`, a push loop over any bound args, then `extend_from_slice(args)`) before
+calling the bound target, even for the overwhelmingly common `.bind(thisArg)`
+shape with **no** partial-applied arguments — a plain method reference
+(`arr.forEach(obj.method.bind(obj))`). `js_function_bind` already leaves
+capture slot 2 (`bound_args_ptr`) null whenever no extra args were bound, so
+that case now skips the allocate/copy/free cycle entirely and passes the
+caller's own `args` slice straight through to `js_native_call_value`. The
+actual partial-application shape (`.bind(obj, extra)`) is untouched other than
+a tighter `Vec` capacity hint (`n + args.len()` instead of the old
+`args.len() + 4`).
+
+This targets the "method reference passed as callback" shape specifically:
+unlike a plain closure held in a local, a `.bind()`-created function is
+excluded from `direct.rs`'s per-loop dispatch hoisting (`BoundFunction` is
+deliberately not resolved by `resolve_direct_func_ptr` —
+`crates/perry-runtime/src/closure/dispatch/direct.rs:70`, inside the
+`func_ptr.is_null() || func_ptr == BOUND_METHOD_FUNC_PTR || func_ptr ==
+BOUND_FUNCTION_FUNC_PTR` early-return at lines 68-73), so `arr.forEach(fn)`
+over a bound method re-runs this allocation on every element.
+
+**Aliasing / rooting**: traced every one of `dispatch_bound_function`'s 9
+call sites (exhaustive repo grep, not just perry-runtime). Every one hands it
+either a `[f64; N]` literal array of by-value `f64` parameters living on the
+Rust native stack (`closure/dispatch/calln.rs`'s per-arity entry points,
+lines 38/83/151/181/215/255/299, and `dispatch_registered_call`'s 8 callers
+at lines 382/415/449/496/549/606/665/727, each building `let args = [arg0,
+arg1, ...]`) or a freshly Rust-`Vec`-allocated copy (`value_call.rs`'s
+`full`, built by a `push` loop over `a(i)` before dispatch, for the >16-arg /
+dynamic-call path). Neither shape is ever GC-managed memory — the collector
+only owns memory it allocates itself (the `js_*_alloc` family into the
+arena/nursery/old-gen) and has no knowledge of the Rust stack or
+`Vec`/`Box` heap. So `args`'s *backing storage* can't be moved or freed by a
+collection inside this call on any path, before or after this change, and
+handing `js_native_call_value` the caller's slice directly instead of a
+byte-copy of it is safe.
+
+That is a narrower claim than "GC-safe" and worth stating precisely: a stack
+array of NaN-boxed values is not a GC root, and neither was the old `Vec`
+copy. If a collection *moves* an object referenced by one of the argument
+*values* during `js_native_call_value` (e.g. inside `rebind_explicit_this`,
+which can allocate), neither the old buffer nor the new one gets its bits
+rewritten — copying bytes into a fresh `Vec` is not registering a root, so it
+never protected against that. This change neither introduces nor fixes that
+pre-existing exposure; it is identical before and after (and the conservative
+native-stack scan that could theoretically cover it is diagnostic-only by
+default, `Auto` → `SkipDisabled`).
+
+Measured with the repo's differential probe technique (marginal cost isolated
+from loop overhead, instructions retired plus wall/CPU time under a
+measurement mutex, best-of-N): the `.bind(obj)`-with-no-extra-args shape drops
+from ~2795 to ~2676 instructions per call (~4.3%), reproduced across two
+independent runs (N=50000 and N=250000, 7 and 15 reps). Wall-clock time on the
+measurement host was noisy under heavy unrelated contention (system load
+averaged 60-117 on a 10-core box) and did not resolve a stable direction; the
+more contention-robust process CPU-time metric (user+sys) showed no
+regression (flat-to-favorable across both runs). The partially-applied-bind
+shape (`.bind(obj, extra)`), the already-optimized loop-local-closure shape,
+and a bare-loop control all measured ~0 delta, confirming the change is
+isolated to its target shape.
+
+Added `test-files/test_gap_callback_dispatch_shapes.ts`, byte-identical
+against node 26.5.1, covering: direct arrow inline to a builtin array method,
+a closure held in a local (once and in a loop), a callback threaded through a
+second function frame, a callback parameter called directly in a loop, a
+`.bind()` method reference with and without partial args, `this` binding
+across arrow/ordinary/bound call shapes (including the receiverless-call
+`this === undefined` case), `arguments`/extra/missing-argument/`.length`
+handling, a callback that throws through one and two frames, recursion
+through a plain and a bound callback reference, closures capturing a loop
+variable (`let` and the classic `var` + IIFE idiom), and a bound method used
+as a hot per-element `forEach` callback.

crates/perry-runtime/src/closure/dispatch/bound.rs

@@ -306,26 +306,45 @@ pub unsafe fn dispatch_bound_function(closure: *const ClosureHeader, args: &[f64
 
     // Collect the partial-applied (bound) leading args, then append the
     // call-time args. `g = f.bind(obj, 2); g(3)` calls `f` with `(2, 3)`.
-    let mut combined: Vec<f64> = Vec::with_capacity(args.len() + 4);
-    if !bound_args_ptr.is_null() {
+    //
+    // The overwhelmingly common shape is `.bind(thisArg)` with NO partial
+    // args at all — a plain method reference (`arr.forEach(obj.method.bind(
+    // obj))`), the callback shape `direct.rs` cannot hoist out of a loop
+    // (BoundFunction is deliberately excluded from `resolve_direct_func_ptr`
+    // — see that module's doc), so this function runs on every element.
+    // `js_function_bind` leaves capture slot 2 (`bound_args_ptr`) null
+    // whenever `bound_arg_count == 0`, so that's exactly the free-to-detect
+    // case: skip the allocate-copy-free `Vec` and hand `js_native_call_value`
+    // the caller's own `args` slice directly. Only the actual
+    // partial-application shape (`.bind(obj, extra)`) still needs a combined
+    // buffer.
+    let mut combined: Vec<f64>;
+    let (call_ptr, call_len): (*const f64, usize) = if bound_args_ptr.is_null() {
+        if args.is_empty() {
+            (std::ptr::null(), 0)
+        } else {
+            (args.as_ptr(), args.len())
+        }
+    } else {
         let n = crate::array::js_array_length(bound_args_ptr) as usize;
+        combined = Vec::with_capacity(n + args.len());
         for i in 0..n {
             combined.push(crate::array::js_array_get_f64(bound_args_ptr, i as u32));
         }
-    }
-    combined.extend_from_slice(args);
+        combined.extend_from_slice(args);
+        if combined.is_empty() {
+            (std::ptr::null(), 0)
+        } else {
+            (combined.as_ptr(), combined.len())
+        }
+    };
 
     // A bound concise/object-literal method reads `this` from its baked capture
     // slot, not IMPLICIT_THIS — rebind it to the bound receiver so the bound
     // `this` is honored (arrows/non-captures_this targets are returned as-is).
     let target = rebind_explicit_this(target, bound_this);
     let this_scope = crate::gc::RuntimeHandleScope::new(); // #9445
     let prev_this = this_scope.root_nanbox_f64(crate::object::js_implicit_this_set(bound_this));
-    let (call_ptr, call_len) = if combined.is_empty() {
-        (std::ptr::null::<f64>(), 0usize)
-    } else {
-        (combined.as_ptr(), combined.len())
-    };
     let result = js_native_call_value(target, call_ptr, call_len);
     crate::object::js_implicit_this_set(prev_this.get_nanbox_f64());
     result

test-files/test_gap_callback_dispatch_shapes.ts

@@ -0,0 +1,227 @@
+// Callback-dispatch shapes audited for perf/callback-dispatch (calling a
+// JSValue that holds a closure through the various paths codegen/runtime
+// recognize: direct arrow-inline array-method callback, a closure held in a
+// local, a closure threaded through a second function frame, a
+// `Function.prototype.bind` method reference with and without partial args,
+// and a hand-written tight loop over a callback parameter).
+//
+// This exercises exactly the shape `dispatch_bound_function`
+// (crates/perry-runtime/src/closure/dispatch/bound.rs) treats specially:
+// `.bind(thisArg)` with NO extra bound args skips the old per-call `Vec`
+// combine-copy and passes the call-time args straight through. What is easy
+// to break doing that: `this` binding (arrow vs ordinary function vs bound),
+// `arguments`, extra/missing arguments and `.length`, a callback that
+// throws (stack must stay correct), recursion through a callback reference,
+// and a callback closing over a loop variable.
+
+function log(label: string, value: unknown): void {
+  console.log(label + ": " + JSON.stringify(value));
+}
+
+// --- shape: direct arrow inline to a builtin array-iteration method -------
+{
+  const arr = [1, 2, 3, 4, 5];
+  let s = 0;
+  arr.forEach((x) => { s += x; });
+  log("forEach_arrow_inline", s);
+}
+
+// --- shape: callback held in a local, called once -------------------------
+{
+  const cb = (x: number) => x + 1;
+  log("local_once", cb(5));
+}
+
+// --- shape: callback held in a local, called in a tight loop --------------
+{
+  const cb = (x: number) => x * 2;
+  let s = 0;
+  for (let i = 0; i < 20; i++) s += cb(i);
+  log("local_loop", s);
+}
+
+// --- shape: callback threaded through a second function frame -------------
+function invokeOnce(cb: (x: number) => number, x: number): number {
+  return cb(x);
+}
+function twoFrames(cb: (x: number) => number, n: number): number {
+  let s = 0;
+  for (let i = 0; i < n; i++) s += invokeOnce(cb, i);
+  return s;
+}
+log("two_frames", twoFrames((x) => x + 3, 15));
+
+// --- shape: callback parameter called directly in a tight loop ------------
+function directLoop(cb: (i: number) => number, n: number): number {
+  let s = 0;
+  for (let i = 0; i < n; i++) s += cb(i);
+  return s;
+}
+log("direct_loop_param", directLoop((x) => x - 1, 12));
+
+// --- shape: method reference via .bind(), NO extra bound args -------------
+class Adder {
+  base: number;
+  constructor(base: number) { this.base = base; }
+  add(x: number): number { return this.base + x; }
+  // reads `this` explicitly, so a wrong receiver after bind() is observable.
+  describe(): string { return "Adder(" + this.base + ")"; }
+}
+{
+  const a = new Adder(10);
+  const fn = a.add.bind(a);
+  const arr = [1, 2, 3, 4, 5];
+  let s = 0;
+  arr.forEach((x) => { s += fn(x); });
+  log("bound_no_extra_args_forEach", s);
+  log("bound_no_extra_args_once", fn(7));
+
+  const describeFn = a.describe.bind(a);
+  log("bound_this_reads_correctly", describeFn());
+}
+
+// --- shape: method reference via .bind(), WITH partial-applied args -------
+{
+  const a = new Adder(100);
+  const fn2 = a.add.bind(a, 5); // bound arg `5` prepended... but `add` takes
+  // only ONE param, so the extra bound arg is simply ignored per spec (bound
+  // length caps at 0, extra bound args beyond declared arity are dropped by
+  // the underlying call, not by bind itself -- Node and Perry must agree).
+  log("bound_with_extra_args", fn2(1));
+
+  function sum3(a: number, b: number, c: number): number { return a + b + c; }
+  const boundSum = sum3.bind(null, 1, 2);
+  log("bound_plain_fn_partial", boundSum(3));
+}
+
+// --- shape: this binding across arrow / ordinary function / bound ---------
+{
+  const obj = {
+    v: 42,
+    arrowGet(this: any) { return (() => this.v)(); },
+    ordinary(this: any) { return this.v; },
+  };
+  function grabThis(this: any): unknown { return this; }
+  const boundGrab = grabThis.bind(obj);
+  log("this_arrow_capture", obj.arrowGet());
+  log("this_ordinary_direct", obj.ordinary());
+  log("this_bound_grab", (boundGrab() as { v: number }).v);
+
+  // A receiverless call of an ordinary function callback observes
+  // `this === undefined` (strict-mode-like OrdinaryCallBindThis) even when
+  // an enclosing method call left an IMPLICIT_THIS around.
+  function receiverless(this: unknown): string {
+    return this === undefined ? "undefined" : "leaked:" + JSON.stringify(this);
+  }
+  function callIt(cb: () => string): string { return cb(); }
+  const holder = {
+    m(): string { return callIt(receiverless); },
+  };
+  log("this_receiverless_no_leak", holder.m());
+}
+
+// --- shape: arguments object + extra/missing args + .length ---------------
+{
+  function variadic(): string {
+    // eslint-disable-next-line prefer-rest-params
+    const args = arguments as unknown as ArgumentsLike;
+    const parts: string[] = [];
+    for (let i = 0; i < args.length; i++) parts.push(String(args[i]));
+    return parts.join(",");
+  }
+  interface ArgumentsLike { length: number; [i: number]: unknown; }
+  function callWithN(cb: (...a: unknown[]) => string, ...a: unknown[]): string {
+    return cb(...a);
+  }
+  log("arguments_object_extra", callWithN(variadic as any, 1, 2, 3, 4));
+  log("arguments_object_missing", callWithN(variadic as any));
+  log("function_length_declared", ((a: number, b: number, c: number) => a + b + c).length);
+
+  function needsThree(a: number, b: number, c: number): string {
+    return `${a},${b},${c}`;
+  }
+  log("missing_args_become_undefined", (needsThree as any)(1));
+  log("extra_args_ignored", (needsThree as any)(1, 2, 3, 4, 5));
+}
+
+// --- shape: callback that throws, stack must stay correct -----------------
+{
+  function boom(): never { throw new Error("boom"); }
+  function callThrow(cb: () => never): string {
+    try {
+      cb();
+      return "no-throw";
+    } catch (e) {
+      return "caught:" + (e as Error).message;
+    }
+  }
+  log("callback_throws_caught", callThrow(boom));
+
+  const boundBoom = boom.bind(null);
+  let threwFromBound = "no";
+  try {
+    boundBoom();
+  } catch (e) {
+    threwFromBound = "caught:" + (e as Error).message;
+  }
+  log("bound_callback_throws", threwFromBound);
+
+  function outer(): string {
+    function inner(cb: () => never): string {
+      try {
+        cb();
+        return "unreachable";
+      } catch (e) {
+        return (e as Error).message;
+      }
+    }
+    return inner(boom);
+  }
+  log("callback_throws_through_two_frames", outer());
+}
+
+// --- shape: recursion through a callback reference -------------------------
+{
+  function makeCountdown(): (n: number) => number {
+    const step = (n: number): number => (n <= 0 ? 0 : n + step(n - 1));
+    return step;
+  }
+  const countdown = makeCountdown();
+  log("recursive_callback", countdown(10));
+
+  // Recursion through a bound reference to itself.
+  let fact: (n: number) => number;
+  fact = (n: number): number => (n <= 1 ? 1 : n * fact(n - 1));
+  const boundFact = fact.bind(null);
+  log("recursive_bound_callback", boundFact(6));
+}
+
+// --- shape: closure captured in a loop variable ----------------------------
+{
+  const callbacks: Array<() => number> = [];
+  for (let i = 0; i < 5; i++) {
+    callbacks.push(() => i * i);
+  }
+  log("loop_var_capture_let", callbacks.map((f) => f()));
+
+  const callbacksVar: Array<() => number> = [];
+  for (var j = 0; j < 5; j++) {
+    // eslint-disable-next-line no-loop-func
+    callbacksVar.push((function (captured) { return () => captured; })(j));
+  }
+  log("loop_var_capture_var_iife", callbacksVar.map((f) => f()));
+}
+
+// --- shape: bound method used as a hot forEach callback across many calls -
+{
+  class Acc {
+    total: number = 0;
+    add(x: number): number { this.total += x; return this.total; }
+  }
+  const acc = new Acc();
+  const boundAdd = acc.add.bind(acc);
+  const many: number[] = [];
+  for (let i = 0; i < 200; i++) many.push(i);
+  many.forEach((x) => boundAdd(x));
+  log("bound_hot_loop_total", acc.total);
+}