diff --git a/changelog.d/10119-bind-lazy-name-length.md b/changelog.d/10119-bind-lazy-name-length.md new file mode 100644 index 0000000000..eeb46c9196 --- /dev/null +++ b/changelog.d/10119-bind-lazy-name-length.md @@ -0,0 +1,14 @@ +### Performance + +- **`Function.prototype.bind` no longer eagerly builds `"bound " + name` or its + `name`/`length` property-attribute records on every call.** A bind whose + result never reads `.name`/`.length` now performs neither the runtime-string + allocation nor the two `set_builtin_property_attrs` side-table inserts — + redundant, since a closure with no dynamic-prop entry for those keys already + defaults correctly everywhere it's observed. `.name`'s string is built and + cached lazily on first actual read, through the same seam every other reader + of a closure's `.name` already goes through, so `Object. + getOwnPropertyDescriptor`, `console.log`, and a chained `.bind().bind()` all + still see the right value. `Get(Target, "name")` still runs synchronously at + bind time, so a throwing `name` getter on the target still fails `bind()` + itself. Refs #10084. diff --git a/crates/perry-runtime/src/builtins/formatting.rs b/crates/perry-runtime/src/builtins/formatting.rs index d1d7cf66ce..f3fc60928c 100644 --- a/crates/perry-runtime/src/builtins/formatting.rs +++ b/crates/perry-runtime/src/builtins/formatting.rs @@ -290,13 +290,31 @@ fn format_function_for_console(closure_ptr: *const crate::closure::ClosureHeader registered_name_string(func_ptr as usize).filter(|n| !n.is_empty()) } }; - let label = match registry_name.or_else(|| { - props - .iter() - .find(|(k, _)| k == "name") - .and_then(|(_, v)| jsvalue_string_content(*v)) - .filter(|n| !n.is_empty()) - }) { + let label = match registry_name + .or_else(|| { + props + .iter() + .find(|(k, _)| k == "name") + .and_then(|(_, v)| jsvalue_string_content(*v)) + .filter(|n| !n.is_empty()) + }) + .or_else(|| { + // #10084: a `Function.prototype.bind` result's `.name` is built + // lazily and so may be absent from both the func-ptr registry + // (bound closures share the `BOUND_FUNCTION_FUNC_PTR` sentinel, + // never registered with a per-instance name) and the `props` + // snapshot above (taken before any read materialized it). + // Synthesize (and cache) it the same way any other reader of + // `.name` would. + unsafe { + ((*closure_ptr).func_ptr == crate::closure::BOUND_FUNCTION_FUNC_PTR).then(|| { + jsvalue_string_content(crate::closure::bound_function_lazy_name( + closure_ptr as usize, + )) + }) + } + .flatten() + }) { Some(name) => format!("[Function: {name}]"), None => "[Function (anonymous)]".to_string(), }; diff --git a/crates/perry-runtime/src/closure/dispatch.rs b/crates/perry-runtime/src/closure/dispatch.rs index bdfb309264..54ec8dc8e4 100644 --- a/crates/perry-runtime/src/closure/dispatch.rs +++ b/crates/perry-runtime/src/closure/dispatch.rs @@ -21,7 +21,7 @@ mod validate; mod value_call; pub(crate) use bound::{ - bound_method_source_func_ptr, coerce_call_this, rebind_explicit_this, + bound_function_lazy_name, bound_method_source_func_ptr, coerce_call_this, rebind_explicit_this, reify_function_method_value, }; pub use bound::{dispatch_bound_function, dispatch_bound_method, js_function_bind}; diff --git a/crates/perry-runtime/src/closure/dispatch/bound.rs b/crates/perry-runtime/src/closure/dispatch/bound.rs index f1c0e8d27d..c44f61dab4 100644 --- a/crates/perry-runtime/src/closure/dispatch/bound.rs +++ b/crates/perry-runtime/src/closure/dispatch/bound.rs @@ -435,20 +435,79 @@ pub(crate) fn rebind_explicit_this(target: f64, this_arg: f64) -> f64 { f64::from_bits(crate::closure::clone_closure_rebind_this(bits, this_arg)) } -/// Read a callable's own `name` *property* as a Rust `String`, if present and a -/// String value. Covers names installed by `Object.defineProperty(fn, "name", -/// …)` and the `"bound …"` name a prior `.bind()` stores, neither of which is -/// visible through the declared-name func-ptr registry. Returns `None` when no -/// such property exists or it isn't a String. -unsafe fn read_function_name_property(closure_ptr: usize) -> Option { +/// Fallback target name for [`bound_function_lazy_name`]: the target-name +/// snapshot captured at bind time (capture slot 3) was not a String (no +/// override, or an explicit non-String `Object.defineProperty` value — both +/// collapse to the same declared-name fallback, matching the prior eager +/// behavior), so fall back to the target's *declared* name — the func-ptr +/// registry for a closure, or the class registry for a class ref. Both +/// registries are immutable for the life of the program, so resolving them +/// lazily here instead of at bind time is observationally identical. +unsafe fn bound_target_declared_name(target_value: f64) -> String { use crate::value::JSValue; - let name_val = crate::closure::closure_get_dynamic_prop(closure_ptr, "name"); - let name_jv = JSValue::from_bits(name_val.to_bits()); - if !name_jv.is_any_string() { - return None; + let target_jv = JSValue::from_bits(target_value.to_bits()); + if target_jv.is_pointer() { + let target_closure = target_jv.as_pointer::(); + if !target_closure.is_null() && (*target_closure).type_tag == CLOSURE_MAGIC { + return crate::builtins::function_name_for_ptr((*target_closure).func_ptr as usize) + .unwrap_or_default(); + } + return String::new(); } - let hdr = crate::builtins::js_string_coerce(name_val); - crate::object::has_own_helpers::str_from_string_header(hdr).map(str::to_owned) + let target_class_id = crate::object::class_ref_id(target_value).or_else(|| { + ((target_value.to_bits() >> 48) == 0x7FFE + && crate::object::class_prototype_ref_id(target_value).is_none()) + .then_some((target_value.to_bits() & 0xFFFF_FFFF) as u32) + }); + target_class_id + .and_then(crate::object::class_name_for_id) + .unwrap_or_default() +} + +/// Lazily synthesize and cache a bound function's `.name`. `ptr` must be a +/// live `BOUND_FUNCTION_FUNC_PTR` closure with no `"name"` entry in its own +/// dynamic-prop table yet (the caller — [`closure_get_dynamic_prop`], +/// `Object.getOwnPropertyDescriptor`, and the console-formatting path — all +/// check that first). Refs #10084: `js_function_bind` no longer builds +/// `"bound " + targetName` or writes it to the dynamic-prop table on every +/// call; that work happens here, once, on first actual `.name` read, and the +/// result is cached via `closure_set_dynamic_prop` so repeat reads are O(1). +/// +/// Capture slot 3 holds the raw `Get(Target, "name")` value snapshotted at +/// bind time (a String value, or a non-String sentinel — see +/// `bound_target_declared_name`); capture slot 0 holds the original bind +/// target, used for the declared-name fallback and, transitively, for a +/// chained `f.bind().bind()` (reading slot 3 of an inner bound closure +/// recurses into this same function through `closure_get_dynamic_prop`). +/// +/// GC safety: `ptr`'s address must not be trusted across the allocating +/// `js_string_coerce`/`js_string_from_bytes` calls below, so it is rooted and +/// re-derived afterward before the final cache write (mirrors +/// `js_object_get_own_property_descriptor`'s closure arm, #6943). +pub(crate) unsafe fn bound_function_lazy_name(ptr: usize) -> f64 { + use crate::value::JSValue; + + let scope = crate::gc::RuntimeHandleScope::new(); + let ptr_handle = scope.root_raw_mut_ptr(ptr as *mut u8); + let (name_value, ptr_raw) = ptr_handle.across_mut::(|| { + let closure = ptr as *const ClosureHeader; + let name_hint = js_closure_get_capture_f64(closure, 3); + let target_name = if JSValue::from_bits(name_hint.to_bits()).is_any_string() { + let hdr = crate::builtins::js_string_coerce(name_hint); + crate::object::has_own_helpers::str_from_string_header(hdr) + .map(str::to_owned) + .unwrap_or_default() + } else { + bound_target_declared_name(js_closure_get_capture_f64(closure, 0)) + }; + let bound_name = format!("bound {target_name}"); + let name_ptr = + crate::string::js_string_from_bytes(bound_name.as_ptr(), bound_name.len() as u32); + f64::from_bits(JSValue::string_ptr(name_ptr).bits()) + }); + let ptr = ptr_raw as usize; + crate::closure::closure_set_dynamic_prop(ptr, "name", name_value); + name_value } /// `Function.prototype.bind(thisArg, ...boundArgs)` — create a distinct bound @@ -457,8 +516,18 @@ unsafe fn read_function_name_property(closure_ptr: usize) -> Option { /// the BOUND_FUNCTION_FUNC_PTR sentinel; `js_closure_callN` / /// `js_native_call_value` route it through `dispatch_bound_function`. /// -/// `.name` is set to `"bound " + target.name` and `.length` to -/// `max(0, target.length - boundArgs.length)`, matching Node. Refs #2840. +/// `.name` reads as `"bound " + target.name` and `.length` as +/// `max(0, target.length - boundArgs.length)`, matching Node — but neither is +/// built eagerly (#10084). `.length`'s numeric value is cheap to compute +/// (no string work) and is stored eagerly as before; `.name`'s `"bound "` +/// string and the `set_builtin_property_attrs` calls a prior version made +/// unconditionally for both are gone from this function entirely — absent a +/// dynamic-prop table entry, a closure's `name`/`length` already default to +/// `{writable:false, enumerable:false, configurable:true}` everywhere they're +/// observed (`closure_dynamic_enumerable_props`, +/// `js_object_get_own_property_descriptor`, `closure_set_field_by_name`), so +/// those calls were redundant. `.name`'s string is built lazily by +/// `bound_function_lazy_name`, on first actual read. Refs #2840. #[no_mangle] pub unsafe extern "C" fn js_function_bind( target_value: f64, @@ -485,28 +554,67 @@ pub unsafe extern "C" fn js_function_bind( && crate::object::class_prototype_ref_id(target_value).is_none()) .then_some((target_value.to_bits() & 0xFFFF_FFFF) as u32) }); - let target_closure = if target_jv.is_pointer() { + let target_is_closure = if target_jv.is_pointer() { let ptr = target_jv.as_pointer::(); if ptr.is_null() || (*ptr).type_tag != CLOSURE_MAGIC { // Preserve the existing conservative pass-through for callable // native handles that do not use the closure representation. return target_value; } - Some(ptr) + true } else if target_class_id.is_some() { // ClassRefs are callable/constructable INT32-tagged values rather // than heap closures. They still need a real BoundFunction wrapper // so `new C.bind(_, ...args)()` prepends its captured arguments. - None + false } else { return target_value; }; + // Root the bind target across every allocating call below (`this` + // boxing, `Get(Target, "name")` — which may run a user getter — the + // partial-args array, and the bound closure itself) so none of them can + // leave a stale address in the bound closure's own capture slots after a + // copying minor. + let scope = crate::gc::RuntimeHandleScope::new(); + let target_h = scope.root_nanbox_f64(target_value); + let bound_this = if args_len >= 1 && !args_ptr.is_null() { - coerce_call_this(target_value, *args_ptr) + let arg0 = *args_ptr; + target_h + .across_nanbox(|| coerce_call_this(target_h.get_nanbox_f64(), arg0)) + .0 } else { f64::from_bits(crate::value::TAG_UNDEFINED) }; + let this_h = scope.root_nanbox_f64(bound_this); + + // Spec step 12-13: `Get(Target, "name")` must run now, synchronously — a + // target whose `name` getter throws must fail `bind()` itself (Test262 + // bind/instance-name-error.js), not a later `.name` read on the bound + // function. A class target has no analogous accessor path, so + // TAG_UNDEFINED (the "no override" sentinel `bound_function_lazy_name` + // recognizes via `bound_target_declared_name`) is captured directly. This + // is the ONLY work `.name` does at bind time now — see + // `bound_function_lazy_name` for the deferred "bound " + name build. + let name_hint = if target_is_closure { + target_h + .across_nanbox(|| { + let tclosure = + JSValue::from_bits(target_h.get_nanbox_f64().to_bits()).as_pointer::(); + crate::closure::closure_get_dynamic_prop(tclosure as usize, "name") + }) + .0 + } else { + f64::from_bits(crate::value::TAG_UNDEFINED) + }; + // `name_hint` may itself be a heap string pointer (a real `Get(Target, + // "name")` result) — root it too, or it would go stale across the + // partial-args array / bound-closure allocations below, and we'd write a + // dangling capture slot 3 (exactly the "lazily-derived name retains a + // stale address" failure mode this fix must avoid). + let name_h = scope.root_nanbox_f64(name_hint); + let bound_arg_count = args_len.saturating_sub(1); // Build the partial-args array (NaN-boxed values copied as-is). @@ -520,17 +628,38 @@ pub unsafe extern "C" fn js_function_bind( } else { std::ptr::null_mut() }; + let args_h = (!bound_args_arr.is_null()).then(|| scope.root_raw_mut_ptr(bound_args_arr)); - // Allocate the bound closure with 3 capture slots. - let bound = crate::closure::js_closure_alloc(BOUND_FUNCTION_FUNC_PTR, 3); + // Allocate the bound closure with 4 capture slots: target, bound this, + // partial-args array, and the `.name` snapshot above. + let bound = crate::closure::js_closure_alloc(BOUND_FUNCTION_FUNC_PTR, 4); + let bound_h = scope.root_raw_mut_ptr(bound as *mut u8); + let bound = bound_h.get_raw_mut_ptr::(); + let target_value = target_h.get_nanbox_f64(); + let bound_this = this_h.get_nanbox_f64(); + let name_hint = name_h.get_nanbox_f64(); + let bound_args_arr = args_h + .as_ref() + .map(|h| h.get_raw_mut_ptr::()) + .unwrap_or(std::ptr::null_mut()); js_closure_set_capture_f64(bound, 0, target_value); js_closure_set_capture_f64(bound, 1, bound_this); js_closure_set_capture_ptr(bound, 2, bound_args_arr as i64); + js_closure_set_capture_f64(bound, 3, name_hint); + + // Re-derive the target closure pointer from the (possibly refreshed) + // `target_value` for the `.length` read below — `target_is_closure`'s + // classification doesn't change, but the address might have. + let target_closure = target_is_closure + .then(|| JSValue::from_bits(target_value.to_bits()).as_pointer::()); // Spec `.length` = max(0, ToIntegerOrInfinity(Get(target, "length")) - // boundArgs.length). An `Object.defineProperty(fn, "length", {value})` // override (own dynamic prop) wins over the registered declared length, - // and the value may be NaN (→ 0), ±Infinity, or beyond int32. + // and the value may be NaN (→ 0), ±Infinity, or beyond int32. This read + // is an own-data-property lookup only (no accessor/getter support), so + // unlike `.name` above it cannot run arbitrary code and needs no + // rooting of its own. let target_len_f = if let Some(target_closure) = target_closure { match crate::closure::closure_get_own_dynamic_prop(target_closure as usize, "length") { Some(v) => { @@ -569,42 +698,6 @@ pub unsafe extern "C" fn js_function_bind( ); } - // Spec `.name` = "bound " + targetName, where targetName is `Get(Target, - // "name")` (the empty string when that is not a String). Read the target's - // `name` *property* first — it reflects an `Object.defineProperty(fn, - // "name", …)` override and a previous `.bind()`'s `"bound …"` name (so - // `f.bind().bind().name` chains to `"bound bound …"`). Fall back to the - // declared name from the func-ptr registry for plain named functions, which - // don't materialize a `name` data property. - let target_name = if let Some(target_closure) = target_closure { - read_function_name_property(target_closure as usize) - .or_else(|| crate::builtins::function_name_for_ptr((*target_closure).func_ptr as usize)) - .unwrap_or_default() - } else { - target_class_id - .and_then(crate::object::class_name_for_id) - .unwrap_or_default() - }; - let bound_name = format!("bound {target_name}"); - let name_ptr = - crate::string::js_string_from_bytes(bound_name.as_ptr(), bound_name.len() as u32); - let name_value = f64::from_bits(JSValue::string_ptr(name_ptr).bits()); - crate::closure::closure_set_dynamic_prop(bound as usize, "name", name_value); - // Spec attributes for a function's own `name`/`length`: - // { writable: false, enumerable: false, configurable: true }. Without - // these the dynamic-prop `name` slot defaults to enumerable and shows - // up in for-in / Object.keys (Test262 bind/instance-name*). - crate::object::set_builtin_property_attrs( - bound as usize, - "name".to_string(), - crate::object::PropertyAttrs::new(false, false, true), - ); - crate::object::set_builtin_property_attrs( - bound as usize, - "length".to_string(), - crate::object::PropertyAttrs::new(false, false, true), - ); - crate::gc::runtime_write_barrier_root_heap_word(bound as u64); f64::from_bits(JSValue::pointer(bound as *mut u8).bits()) } diff --git a/crates/perry-runtime/src/closure/dynamic_props.rs b/crates/perry-runtime/src/closure/dynamic_props.rs index 06e760bd7c..d2fbcaf4dd 100644 --- a/crates/perry-runtime/src/closure/dynamic_props.rs +++ b/crates/perry-runtime/src/closure/dynamic_props.rs @@ -838,6 +838,21 @@ pub fn closure_get_dynamic_prop(ptr: usize, prop: &str) -> f64 { } return crate::closure::closure_length(ptr as *const ClosureHeader).unwrap_or(0) as f64; } + // #10084: a `Function.prototype.bind` result's `.name` is built lazily — + // `js_function_bind` skips the "bound " + target-name string allocation + // on every call and only snapshots the raw target-name value (capture + // slot 3). Synthesize and cache the real string here, on first read, so + // every other reader of a closure's `.name` (ordinary property-get below, + // `Object.getOwnPropertyDescriptor`, a chained `.bind()`'s own read of an + // already-bound target) gets it for free through this one seam. Once + // cached, the `closure_props` lookup above intercepts before this runs + // again. + if prop == "name" && !closure_is_key_deleted(ptr, "name") { + let func_ptr = unsafe { (*(ptr as *const ClosureHeader)).func_ptr }; + if func_ptr == crate::closure::BOUND_FUNCTION_FUNC_PTR { + return unsafe { crate::closure::bound_function_lazy_name(ptr) }; + } + } // #36 / #321: own prop miss — walk the closure's static prototype chain // (`Object.setPrototypeOf(closure, protoObj)`). Reads a string-keyed field // off the proto object. Lets effect's `TagClass._op` resolve to "Tag" on diff --git a/crates/perry-runtime/src/closure/mod.rs b/crates/perry-runtime/src/closure/mod.rs index 20666f8b15..93177371db 100644 --- a/crates/perry-runtime/src/closure/mod.rs +++ b/crates/perry-runtime/src/closure/mod.rs @@ -53,7 +53,7 @@ pub use registry::{ }; pub(crate) use dispatch::{ - bound_method_source_func_ptr, coerce_call_this, rebind_explicit_this, + bound_function_lazy_name, bound_method_source_func_ptr, coerce_call_this, rebind_explicit_this, reify_function_method_value, reset_throw_not_callable_counter, }; pub use dispatch::{ diff --git a/test-files/test_gap_10084_bind_lazy_name_length.ts b/test-files/test_gap_10084_bind_lazy_name_length.ts new file mode 100644 index 0000000000..a636137845 --- /dev/null +++ b/test-files/test_gap_10084_bind_lazy_name_length.ts @@ -0,0 +1,114 @@ +// Gap test for #10084: `Function.prototype.bind` eagerly materialized the +// bound function's `.name` string and `set_builtin_property_attrs` records +// for `.name`/`.length` on every call, even when neither was ever read. The +// fix defers building "bound " + name (and the dynamic-prop cache entry) to +// first actual `.name` read, and drops the now-redundant attrs calls +// entirely. This test pins the full spec surface the fix must preserve. + +function add(this: { bias: number }, a: number, b: number): number { + return this.bias + a + b; +} + +// A bind that never reads .name/.length must still work correctly when +// called — the lazy-metadata path must not affect invocation. +const bound = add.bind({ bias: 10 }, 1); +console.log("call result:", bound(2)); // 13 + +// `.name` reads "bound " + target name, and `.length` = max(0, target.length +// - boundArgs.length). +console.log("name:", bound.name); // bound add +console.log("length:", bound.length); // 1 + +// Chained bind: reading the outer bound function's name must recurse through +// the inner (also-lazy) bound function's own name synthesis. +const chained = add.bind({ bias: 0 }).bind({ bias: 0 }); +console.log("chained name:", chained.name); // bound bound add +console.log("chained length:", chained.length); // 2 + +// `Object.defineProperty` override on the target, observed through bind. +function target() {} +Object.defineProperty(target, "name", { value: "renamedTarget" }); +console.log("override name:", target.bind().name); // bound renamedTarget + +// Non-string override on the target falls back to the empty string, not the +// declared name. Matches Test262's bind/instance-name-non-string.js exactly: +// the function expression is passed directly to `defineProperty` (never +// bound to a variable, so no NamedEvaluation name inference applies) — a +// truly nameless target sidesteps a pre-existing, unrelated ambiguity +// between "no name override" and "name explicitly set to `undefined`" (both +// read back as the same sentinel) that a named target would otherwise hit. +const anon = Object.defineProperty(function () {}, "name", { + value: undefined, +}); +console.log("non-string override name:", anon.bind().name); // bound + +// name/length are non-enumerable: absent from Object.keys/for-in, and +// hasOwnProperty still reports them present. +const enumKeys: string[] = []; +for (const k in bound) enumKeys.push(k); +console.log("for-in keys:", JSON.stringify(enumKeys)); // [] +console.log("Object.keys:", JSON.stringify(Object.keys(bound))); // [] +console.log( + "hasOwnProperty name/length:", + bound.hasOwnProperty("name"), + bound.hasOwnProperty("length"), +); // true true + +// Property descriptor attributes match spec defaults even though bind never +// wrote them explicitly. +const desc = Object.getOwnPropertyDescriptor(bound, "name")!; +console.log( + "name descriptor:", + desc.value, + desc.writable, + desc.enumerable, + desc.configurable, +); // bound add false false true +const lenDesc = Object.getOwnPropertyDescriptor(bound, "length")!; +console.log( + "length descriptor:", + lenDesc.value, + lenDesc.writable, + lenDesc.enumerable, + lenDesc.configurable, +); // 1 false false true + +// A write to .name/.length throws under strict mode (non-writable) — +// this file runs as an ES module, so every write attempt is strict. +let nameWriteThrew = false; +try { + (bound as any).name = "clobbered"; +} catch { + nameWriteThrew = true; +} +let lengthWriteThrew = false; +try { + (bound as any).length = 99; +} catch { + lengthWriteThrew = true; +} +console.log( + "write threw / unchanged:", + nameWriteThrew, + lengthWriteThrew, + bound.name, + bound.length, +); // true true bound add 1 + +// Beyond-u32 (here +Infinity) target length forwards through bind's own +// dynamic-prop fallback path. +function infLen() {} +Object.defineProperty(infLen, "length", { value: Infinity }); +console.log("infinity length:", infLen.bind().length); // Infinity + +// A class target still binds correctly and reports its name lazily. +class Widget { + static tag = "w"; +} +const BoundWidget = Widget.bind(null); +console.log("class bind name:", BoundWidget.name); // bound Widget + +// console.log on a bound function whose .name was NEVER read must still +// display the synthesized name (formatting must not bypass the lazy path). +const neverRead = add.bind({ bias: 0 }, 1); +console.log("display:", neverRead);