diff --git a/changelog.d/10127-call-apply-dispatch-cache.md b/changelog.d/10127-call-apply-dispatch-cache.md new file mode 100644 index 0000000000..543e5f01b0 --- /dev/null +++ b/changelog.d/10127-call-apply-dispatch-cache.md @@ -0,0 +1,7 @@ +### Performance + +- **`Function.prototype.call`, `Function.prototype.apply`, and direct spread + calls no longer hash-probe the closure-body registry on every invocation.** + Their rest-parameter check now shares the existing four-entry dispatch memo, + retaining late-registration invalidation and all 0–16 argument, receiver, + bound-function, rest-array, and imported-class semantics. Refs #10085. diff --git a/crates/perry-runtime/src/closure/registry.rs b/crates/perry-runtime/src/closure/registry.rs index f77ba5dcc3..48a114cefe 100644 --- a/crates/perry-runtime/src/closure/registry.rs +++ b/crates/perry-runtime/src/closure/registry.rs @@ -219,6 +219,8 @@ crate::perry_thread_local! { /// The record for `func_ptr`, if module init registered anything about it. #[inline(always)] fn body_record(func_ptr: *const u8) -> Option { + #[cfg(test)] + BODY_RECORD_LOOKUPS.with(|lookups| lookups.set(lookups.get() + 1)); CLOSURE_BODY_REGISTRY.with(|r| r.borrow().get(&(func_ptr as usize)).copied()) } @@ -307,6 +309,7 @@ crate::perry_thread_local! { #[cfg(test)] std::thread_local! { static RESOLVE_STRATEGY_SLOW_CALLS: std::cell::Cell = const { std::cell::Cell::new(0) }; + static BODY_RECORD_LOOKUPS: std::cell::Cell = const { std::cell::Cell::new(0) }; } #[derive(Clone, Copy)] @@ -437,6 +440,27 @@ mod dispatch_recent_tests { 4.0 } + extern "C" fn add_two(_: *const ClosureHeader, left: f64, right: f64) -> f64 { + left + right + } + + extern "C" fn identify_rest_array(_: *const ClosureHeader, value: f64) -> f64 { + let is_pointer = value.to_bits() >> 48 == crate::value::POINTER_TAG >> 48; + if is_pointer { + 1.0 + } else { + 0.0 + } + } + + fn stack_closure(func_ptr: *const u8) -> ClosureHeader { + ClosureHeader { + func_ptr, + capture_count: 0, + type_tag: CLOSURE_MAGIC, + } + } + #[test] fn four_alternating_bodies_stay_out_of_the_hash_lookup() { let bodies = [ @@ -486,6 +510,90 @@ mod dispatch_recent_tests { "late registration must evict a body from every recent-cache slot" ); } + + #[test] + fn repeated_array_calls_probe_the_body_registry_only_once() { + let body = add_two as *const u8; + let closure = stack_closure(body); + let args = [20.0, 22.0]; + invalidate_dispatch_strategy(body); + RESOLVE_STRATEGY_SLOW_CALLS.with(|calls| calls.set(0)); + BODY_RECORD_LOOKUPS.with(|lookups| lookups.set(0)); + + for _ in 0..32 { + assert_eq!( + unsafe { + crate::closure::js_closure_call_array( + &closure as *const ClosureHeader as i64, + args.as_ptr(), + args.len() as i64, + ) + }, + 42.0 + ); + } + + assert_eq!( + RESOLVE_STRATEGY_SLOW_CALLS.with(|calls| calls.get()), + 1, + "a warm non-rest call-array path must not hash-probe the registry per call" + ); + assert_eq!( + BODY_RECORD_LOOKUPS.with(|lookups| lookups.get()), + 1, + "32 repeated calls must perform one total closure-body hash lookup" + ); + } + + #[test] + fn late_rest_registration_invalidates_the_call_array_memo() { + let body = identify_rest_array as *const u8; + let closure = stack_closure(body); + let direct_arg = [0.0]; + invalidate_dispatch_strategy(body); + RESOLVE_STRATEGY_SLOW_CALLS.with(|calls| calls.set(0)); + BODY_RECORD_LOOKUPS.with(|lookups| lookups.set(0)); + + assert_eq!( + unsafe { + crate::closure::js_closure_call_array( + &closure as *const ClosureHeader as i64, + direct_arg.as_ptr(), + direct_arg.len() as i64, + ) + }, + 0.0, + "the first unregistered call must use direct dispatch" + ); + assert_eq!(RESOLVE_STRATEGY_SLOW_CALLS.with(|calls| calls.get()), 1); + assert_eq!(BODY_RECORD_LOOKUPS.with(|lookups| lookups.get()), 1); + + js_register_closure_rest(body, 0); + let rest_args = [1.0, 2.0, 3.0]; + for _ in 0..2 { + assert_eq!( + unsafe { + crate::closure::js_closure_call_array( + &closure as *const ClosureHeader as i64, + rest_args.as_ptr(), + rest_args.len() as i64, + ) + }, + 1.0, + "late registration must switch the cached body to rest dispatch" + ); + } + assert_eq!( + RESOLVE_STRATEGY_SLOW_CALLS.with(|calls| calls.get()), + 2, + "registration should cause one fresh registry probe, then remain cached" + ); + assert_eq!( + BODY_RECORD_LOOKUPS.with(|lookups| lookups.get()), + 2, + "the initial call and post-registration miss should be the only hash lookups" + ); + } } #[no_mangle] @@ -557,7 +665,17 @@ pub fn lookup_closure_rest(func_ptr: *const u8) -> Option { #[inline(always)] pub fn lookup_closure_rest_full(func_ptr: *const u8) -> Option<(u32, RestDispatchKind)> { - body_record(func_ptr)?.rest() + // Rest-ness is part of the same immutable-at-dispatch-time answer as + // bound routing and declared arity. In particular, do not bypass + // `DISPATCH_RECENT` here: `js_closure_call_array` probes this on every + // Function.prototype.call/apply invocation, and a direct `body_record` + // read would pay a TLS RefCell borrow plus a pointer hash lookup every + // time through that hot path. Registration invalidates the memo, so the + // #6475 call-before-registration case still observes a later rest entry. + match resolve_strategy(func_ptr).kind() { + DispatchKind::Rest(fixed_arity, kind) => Some((fixed_arity, kind)), + _ => None, + } } /// Register a closure body's declared param count (for closures WITHOUT a rest diff --git a/test-files/fixtures/issue_10085_dispatch/imported.ts b/test-files/fixtures/issue_10085_dispatch/imported.ts new file mode 100644 index 0000000000..a3acba650f --- /dev/null +++ b/test-files/fixtures/issue_10085_dispatch/imported.ts @@ -0,0 +1,5 @@ +export class ImportedClass { + static describe(value: number): string { + return `imported:${value}`; + } +} diff --git a/test-files/test_gap_10085_call_apply_dispatch.ts b/test-files/test_gap_10085_call_apply_dispatch.ts new file mode 100644 index 0000000000..44d8e025e9 --- /dev/null +++ b/test-files/test_gap_10085_call_apply_dispatch.ts @@ -0,0 +1,131 @@ +// #10085: Function.prototype.call/apply and direct spread calls all reach +// js_closure_call_array. Keep its cached non-rest path and its rest-bundling +// detour semantically identical across every supported dispatch arity. + +import { ImportedClass } from "./fixtures/issue_10085_dispatch/imported.ts"; + +function check(condition: boolean, message: string): void { + if (!condition) throw new Error(message); +} + +function restShape(first: number, ...rest: number[]): string { + return `${first}|${rest.length}|${rest.length === 0 ? -1 : rest[rest.length - 1]}`; +} + +const restFunctions: any[] = []; +restFunctions.push(restShape); +const restValue: any = restFunctions[0]; + +// Rest closures must still bundle through .call, .apply, and direct spread at +// arities 1..=16. This pins #653's high-arity fix while the metadata probe is +// switched from a registry lookup to the dispatch-strategy memo. +for (let n = 1; n <= 16; n++) { + const args: number[] = []; + for (let i = 1; i <= n; i++) args.push(i); + const expected = `1|${n - 1}|${n === 1 ? -1 : n}`; + const direct = restValue(...args); + const viaCall = restValue.call({ ignored: true }, ...args); + const viaApply = restValue.apply({ ignored: true }, args); + check(direct === expected, `rest direct arity ${n}: ${direct}`); + check(viaCall === expected, `rest call arity ${n}: ${viaCall}`); + check(viaApply === expected, `rest apply arity ${n}: ${viaApply}`); +} + +// A 16-parameter non-rest closure called with 0..=16 values exercises every +// js_closure_callN arm. Missing values must still be padded with undefined. +function arity16( + a0: any, + a1: any, + a2: any, + a3: any, + a4: any, + a5: any, + a6: any, + a7: any, + a8: any, + a9: any, + a10: any, + a11: any, + a12: any, + a13: any, + a14: any, + a15: any, +): number { + const values = [ + a0, + a1, + a2, + a3, + a4, + a5, + a6, + a7, + a8, + a9, + a10, + a11, + a12, + a13, + a14, + a15, + ]; + let count = 0; + for (const value of values) if (value !== undefined) count++; + return count; +} + +for (let n = 0; n <= 16; n++) { + const args: number[] = []; + for (let i = 0; i < n; i++) args.push(i + 1); + const direct = (arity16 as any)(...args); + const viaCall = (arity16 as any).call(null, ...args); + const viaApply = (arity16 as any).apply(null, args); + check(direct === n, `arity16 direct ${n}: ${direct}`); + check(viaCall === n, `arity16 call ${n}: ${viaCall}`); + check(viaApply === n, `arity16 apply ${n}: ${viaApply}`); +} + +function ordinary(this: { bias: number }, left: number, right: number): number { + return this.bias + left + right; +} + +const receiver = { + bias: 40, + ordinary, + makeArrow() { + return (value: number) => this.bias + value; + }, +}; +check(receiver.ordinary(1, 1) === 42, "ordinary direct receiver"); +check(receiver.ordinary.call(receiver, 1, 1) === 42, "ordinary call receiver"); +check(receiver.ordinary.apply(receiver, [1, 1]) === 42, "ordinary apply receiver"); + +const arrow = receiver.makeArrow(); +check(arrow(2) === 42, "arrow direct receiver"); +check(arrow.call({ bias: 100 }, 2) === 42, "arrow call keeps lexical receiver"); +check(arrow.apply({ bias: 100 }, [2]) === 42, "arrow apply keeps lexical receiver"); + +const bound = ordinary.bind(receiver, 1); +check(bound(1) === 42, "bound direct receiver"); +check(bound.call({ bias: 100 }, 1) === 42, "bound call keeps receiver"); +check(bound.apply({ bias: 100 }, [1]) === 42, "bound apply keeps receiver"); + +function invokeImportedClass(value: any, input: number): string { + return `${typeof value}:${value.describe(input)}`; +} + +// Imported class refs share the 0x7FFE tag with bridged int32 values. They +// must remain callable objects while call-array unboxes genuine integers. +const classArgs: any[] = [ImportedClass, 7]; +const classExpected = "function:imported:7"; +check((invokeImportedClass as any)(...classArgs) === classExpected, "class direct spread"); +check( + (invokeImportedClass as any).call(null, ...classArgs) === classExpected, + "class call spread", +); +check( + (invokeImportedClass as any).apply(null, classArgs) === classExpected, + "class apply", +); + +console.log("ok");