diff --git a/changelog.d/10651-dynamic-key-cold-tls.md b/changelog.d/10651-dynamic-key-cold-tls.md new file mode 100644 index 0000000000..ca60800c27 --- /dev/null +++ b/changelog.d/10651-dynamic-key-cold-tls.md @@ -0,0 +1,41 @@ +**perf(runtime): stop hoisting cold-path thread-locals into `o[k]`'s fast lane.** +`js_object_get_field_by_name` resolved two thread-locals unconditionally in its +prologue — the `PROXIES` registry and `RUNTIME_HANDLE_STACK`'s fallback — on a +dynamic-key read loop that never touches a Proxy and never uses a `.size` key. +Both belong to arms that are guarded at the Rust level and never taken. + +A `thread_local!` address resolution is `readnone` to LLVM, so once a guarded +cold arm is inlined the optimizer may hoist *just the address computation* above +its guard: the gate survives, the TLS call escapes it. Marking both arms +`#[cold] #[inline(never)]` keeps them opaque to the inliner. The +`runtime_handles` half is the general fix — `RuntimeHandleScope::new()` is called +from dozens of small guarded arms across the runtime, and splitting its fallback +lets the published fast arm stay `#[inline(always)]` without dragging a raw TLS +call to every call site. + +Also: `try_data_get_bytes` called `is_plausible_heap_addr` explicitly and again +inside `try_read_gc_header`, which LLVM could not CSE across +`classify_heap_generation`'s intervening cache write; that one site now uses +`try_read_gc_header_known_plausible`. + +**544.7 → 512.9 instructions per `o[k]` access (−5.8%)**, differenced within each +binary against a bare-loop control reading 0.00 / −0.10 so layout and fixed +per-process cost cancel. Found by disassembly, not by reading: the profile +charged both `_tlv_get_addr` calls to `js_object_get_field_by_name` itself rather +than to a callee, and `otool -tV` plus `nm` named which two thread-locals they +were. Fixing the Proxy block alone left the other in place by a different route. + +Negative results worth not re-running: `try_data_get_bytes`'s `from_utf8` and +Bloom-hash preamble is spec-required work; `is_anon_shape_class_id`'s remaining +11.2% is the per-image `current()` lookup that #10570 already reduced to a hash +plus an 8-slot probe, and a process-global mirror would be unsound across images; +`keys_find_slot_by_bytes`'s `memcmp` is genuine key-byte comparison. + +Validation: new `test_gap_dynamic_key_proxy_receiver.ts` covers trapped, +pass-through and nested Proxies plus interleaved plain/Proxy receivers — the arm +made cold must still be correct when taken — byte-identical to node 26.5.1; +#10570's read-paths test unchanged; four GC-stress runs (seeds 1 and 42, from-space +protection, evacuation verification, scan-abort) all exit 0 with `dangling=0`, +`missing_rewrites=0` and non-zero copying minors (32/29/241/247); gap suite 831/838 +with all 6 failures pre-existing; `perry-runtime --lib` 4,016 passed with the 2 +failures reproduced on pristine `origin/main`. diff --git a/crates/perry-runtime/src/gc/roots/runtime_handles.rs b/crates/perry-runtime/src/gc/roots/runtime_handles.rs index 755d767261..15859d964d 100644 --- a/crates/perry-runtime/src/gc/roots/runtime_handles.rs +++ b/crates/perry-runtime/src/gc/roots/runtime_handles.rs @@ -57,6 +57,34 @@ fn runtime_handle_stack() -> StackRef { return unsafe { &*(stack as *const RuntimeHandleStack) }; } } + runtime_handle_stack_cold() +} + +/// Fallback arm of [`runtime_handle_stack`]: the raw `thread_local!` lookup, +/// reached only before this thread's `HotTls` is published (or from inside +/// `HotTls::fill` itself). `#[inline(never)]` on purpose, not just `#[cold]`. +/// +/// `RuntimeHandleScope::new()` is called from dozens of arms throughout the +/// runtime, many of them small and gated behind a cheap guard deep inside an +/// otherwise hot function (`js_object_get_field_by_name`'s `.size`-key arm is +/// one: see its own `RuntimeHandleScope::new()` call site, guarded on the key +/// bytes equalling `"size"`, with a comment already defending against making +/// the SCOPE unconditional). `crate::tls_hot`'s #7469 note explains why that +/// defense is not enough on its own: a `thread_local!` address resolution is +/// `readnone` from the optimizer's point of view — it has no observable side +/// effect — so once the fallback arm above is visible to the inliner at such a +/// call site, LLVM can (and does) hoist JUST that address computation out of +/// every surrounding guard and run it unconditionally, regardless of how +/// deeply the Rust-level scope construction is gated. Measured: on an `o[k]` +/// loop over a two-property plain object (never touching a `.size` key or a +/// Proxy), this fallback's `_tlv_get_addr` call sat directly in +/// `js_object_get_field_by_name`'s prologue. Keeping this arm opaque to the +/// inliner is what lets the FAST (published) arm above stay `#[inline(always)]` +/// without dragging the raw TLS call along with it at every call site. +#[inline(never)] +#[cold] +#[cfg(not(any(target_os = "android", target_env = "ohos")))] +fn runtime_handle_stack_cold() -> StackRef { RUNTIME_HANDLE_STACK.with(|stack| { // SAFETY: the metadata is const-initialized and has no Drop. Its // cells remain valid throughout thread teardown. Cell is !Sync, so diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs index 5032f7d384..27241ffd51 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs @@ -22,6 +22,33 @@ fn handle_proto_inherited_field( } } +/// #2846 Proxy-receiver forwarding for a generic property read. Split out and +/// `#[inline(never)]` on purpose: `js_proxy_is_proxy` (via `lookup`) and +/// `js_proxy_get` (via `RuntimeHandleScope::new`) each resolve their own +/// `thread_local!` (the proxy registry, the transient-handle root stack). +/// Both accessors are pure address computations from LLVM's point of view — +/// `readnone`, no observable side effect — so once this code was inlined into +/// `js_object_get_field_by_name` the optimizer hoisted BOTH out of the +/// `is_proxy_id_band` guard above them and ran them unconditionally on every +/// call, proxy receiver or not. Measured on an `o[k]` loop over a two-property +/// plain object (never a Proxy): two `_tlv_get_addr` calls sitting directly in +/// `js_object_get_field_by_name`'s prologue, 9.2% of the whole access. +/// `#[inline(never)]` keeps the optimizer from seeing inside this function at +/// the call site, so it cannot hoist anything out of it; `is_proxy_id_band` +/// itself stays inline in the caller since it touches no thread-local. +#[cold] +#[inline(never)] +fn proxy_receiver_get(raw_addr: u64, key: *const crate::StringHeader) -> Option { + const POINTER_TAG: u64 = 0x7FFD_0000_0000_0000; + let boxed = f64::from_bits(POINTER_TAG | (raw_addr & 0x0000_FFFF_FFFF_FFFF)); + if crate::proxy::js_proxy_is_proxy(boxed) == 0 { + return None; + } + let key_f64 = f64::from_bits(crate::value::js_nanbox_string(key as i64).to_bits()); + let v = crate::proxy::js_proxy_get(boxed, key_f64); + Some(JSValue::from_bits(v.to_bits())) +} + #[no_mangle] pub extern "C" fn js_object_get_field_by_name( obj: *const ObjectHeader, @@ -105,12 +132,8 @@ pub extern "C" fn js_object_get_field_by_name( addr }; if crate::value::addr_class::is_proxy_id_band(raw_addr as usize) && !key.is_null() { - const POINTER_TAG: u64 = 0x7FFD_0000_0000_0000; - let boxed = f64::from_bits(POINTER_TAG | (raw_addr & 0x0000_FFFF_FFFF_FFFF)); - if crate::proxy::js_proxy_is_proxy(boxed) != 0 { - let key_f64 = f64::from_bits(crate::value::js_nanbox_string(key as i64).to_bits()); - let v = crate::proxy::js_proxy_get(boxed, key_f64); - return JSValue::from_bits(v.to_bits()); + if let Some(value) = proxy_receiver_get(raw_addr, key) { + return value; } } } diff --git a/crates/perry-runtime/src/object/native_get.rs b/crates/perry-runtime/src/object/native_get.rs index 072b8eb435..b3315dd4d3 100644 --- a/crates/perry-runtime/src/object/native_get.rs +++ b/crates/perry-runtime/src/object/native_get.rs @@ -70,7 +70,10 @@ pub(crate) unsafe fn try_data_get_bytes(receiver: JSValue, key: &[u8]) -> Option { return None; } - let header = crate::value::addr_class::try_read_gc_header(addr)?; + // `is_plausible_heap_addr(addr)` was just proven true above; skip + // `try_read_gc_header`'s own re-derivation of it (see + // `try_read_gc_header_known_plausible`'s doc comment). + let header = crate::value::addr_class::try_read_gc_header_known_plausible(addr)?; if header.obj_type != crate::gc::GC_TYPE_OBJECT || header.gc_flags & crate::gc::GC_FLAG_FORWARDED != 0 || header._reserved & crate::gc::OBJ_FLAG_TYPED_ARRAY_PROTO != 0 diff --git a/crates/perry-runtime/src/value/addr_class.rs b/crates/perry-runtime/src/value/addr_class.rs index dd5796d312..6c05edba4e 100644 --- a/crates/perry-runtime/src/value/addr_class.rs +++ b/crates/perry-runtime/src/value/addr_class.rs @@ -238,6 +238,29 @@ pub(crate) unsafe fn try_read_gc_header(addr: usize) -> Option<&'static GcHeader if !is_plausible_heap_addr(addr) { return None; } + try_read_gc_header_known_plausible(addr) +} + +/// [`try_read_gc_header`] for a caller that already ran +/// [`is_plausible_heap_addr`] on this exact `addr` earlier in the same +/// straight-line scope, with no intervening collection or reassignment of +/// `addr`. Skips re-deriving that magnitude check. +/// +/// `native_get::try_data_get_bytes`'s prototype-chain loop already branches +/// on `is_plausible_heap_addr(addr)` (paired with the arena-generation +/// classification) one statement above every call site this exists for, so +/// the plain [`try_read_gc_header`] was re-running the same handle-band / +/// heap-range compare a second time per step for free. `classify_heap_generation` +/// runs in between and writes its own cache, which is enough to stop LLVM's +/// CSE from eliding the duplicate call on its own (its side effect isn't +/// provably unrelated to `is_plausible_heap_addr`'s inputs from the +/// optimizer's point of view), so the redundancy was real, not just apparent. +/// +/// # Safety +/// As [`try_read_gc_header`], plus: `is_plausible_heap_addr(addr)` must be +/// `true` for this `addr` already (unchecked here). +#[inline(always)] +pub(crate) unsafe fn try_read_gc_header_known_plausible(addr: usize) -> Option<&'static GcHeader> { // Small-buffer slab allocations are heap-plausible but carry NO GcHeader — // `addr - GC_HEADER_SIZE` is the previous slab entry's data bytes, so a // brand probe (Temporal/Date/Map/Set `obj_type` check) would read a diff --git a/test-files/test_gap_dynamic_key_proxy_receiver.ts b/test-files/test_gap_dynamic_key_proxy_receiver.ts new file mode 100644 index 0000000000..6866b8b276 --- /dev/null +++ b/test-files/test_gap_dynamic_key_proxy_receiver.ts @@ -0,0 +1,60 @@ +// Dynamic-key reads, `o[k]`, through a Proxy receiver. +// +// `js_object_get_field_by_name`'s Proxy-forwarding block never satisfies the +// ordinary-object fast data-get lane above it (a Proxy's boxed encoding is a +// small registry id, not a real heap pointer), so it was pulled out into its +// own `#[inline(never)]` helper (`proxy_receiver_get`): inlined in place, the +// compiler proved the proxy registry's and the transient-handle root stack's +// `thread_local!` address resolutions were side-effect-free and hoisted BOTH +// out of the `is_proxy_id_band` guard, so an `o[k]` loop over a plain +// two-property object — never touching a Proxy — paid two `_tlv_get_addr` +// calls on every access. This exercises that the extraction is behavior +// preserving: trapped reads, pass-through reads (no `get` trap), a +// forward-to-target hop through a nested Proxy, and a Proxy loop running +// right alongside an ordinary-object loop on the same key. +const target: any = { a: 1, b: 2 }; + +const trapped = new Proxy(target, { + get(t, prop, receiver) { + if (prop === "special") return "trapped!"; + return Reflect.get(t, prop, receiver); + }, +}); +for (const k of ["a", "b", "special", "missing"]) { + console.log("trapped", k, String(trapped[k])); +} + +// No `get` trap: falls through to the target's own [[Get]]. +const passthrough = new Proxy(target, {}); +for (const k of ["a", "b", "missing"]) { + console.log("passthrough", k, String(passthrough[k])); +} + +// Nested Proxy: a dynamic-key read that recurses through the +// forward-to-target hop inside the outlined helper. +const inner = new Proxy(target, { + get(t, p) { + return (t as any)[p]; + }, +}); +const outer = new Proxy(inner, {}); +console.log("nested", outer["a"]); + +// An ordinary-object loop and a Proxy loop on the same key, back to back — +// the ordinary loop must not pay for the Proxy path, and the Proxy loop must +// still resolve correctly through it. +const plain: any = { k: 1.5, other: 2 }; +let plainTotal = 0; +for (let i = 0; i < 20; i++) plainTotal += plain["k"]; +console.log("plain total", plainTotal); + +let proxyTotal = 0; +for (let i = 0; i < 20; i++) proxyTotal += Number(trapped["a"]); +console.log("proxy total", proxyTotal); + +// A Proxy over an array, read by dynamic numeric-string key. +const arrTarget = [10, 20, 30]; +const arrProxy = new Proxy(arrTarget, {}); +for (const k of ["0", "1", "2", "length"]) { + console.log("arr proxy", k, String((arrProxy as any)[k])); +}