diff --git a/changelog.d/7795-miss-path-probe-gates.md b/changelog.d/7795-miss-path-probe-gates.md new file mode 100644 index 0000000000..8787d6d493 --- /dev/null +++ b/changelog.d/7795-miss-path-probe-gates.md @@ -0,0 +1,65 @@ +### Performance: the object property-MISS path no longer allocates for absent rare-subclass features + +**`asyncpipe` 0.722 s → 0.132 s (−81.7%)** — quiet M1 mini, best-of-5, output +verified byte-identical to node and exit code checked before timing. That moves +an async service pipeline from **9.69× node to 1.76×**, and from 1.92× scriptc +to **0.35×**. The rest of the 19-benchmark corpus moves within ±1.2% +(measurement noise): `churn` 0.424→0.420, `interp` 1.888→1.902, +`pipeline` 0.540→0.540, `tree` 1.634→1.645, `fib40` 0.394→0.393. + +`await`ing a plain object is one of the most common things async TypeScript +does, and it was the slowest thing Perry did. The spec requires a thenable +check on every promise resolution — `Get(resolution, "then")` — so every +`return ` from an `async function` performed a property lookup +that MISSES. That miss path turned out to be a cascade of un-gated +"is this receiver a rare exotic?" probes, and **four of them cost a key-string +allocation plus a full recursive `js_object_get_field_by_name` each, per miss**: + +| probe | hidden field it reads | +|---|---| +| `promise::subclass::subclass_backing_promise` | `__perry_promise_backing__` | +| `object::fetch_subclass_handle_id` | `__perry_fetch_handle__` | +| `object::temporal_subclass_cell` | `__perry_temporal_cell__` | +| `object::map_set_subclass::subclass_backing_of` | `__perry_collection_backing__` | + +Each answers "is this a `class X extends Promise / Request / Temporal.* / +Map | Set` instance?" — virtually always *no*, and knowable without touching the +object at all. Each hidden field has exactly **one** writer, so a monotone +process-wide "has one of these ever been created?" flag is an exact answer. +`FETCH_SUBCLASS_EVER` already existed for the `in`-operator fast path (#6748) +but was never consulted here; the other three flags are new and are armed at +their single stash site, *before* the field is written, so no reader can +observe a stashed field while the flag still reads "never". + +The same miss path also re-resolved the default `Object.prototype` on every +call — `globalThis.Object` (which interns an `"Object"` key string) plus +`closure_get_dynamic_prop("prototype")`. It now reads the memoized, GC-healed +`object_prototype_addr()` cache that the array index fast path already relies +on; it is a registered GC root (`scan_prototype_addr_cache_roots_mut`), and +`Object.prototype` is non-writable/non-configurable per spec so the memo cannot +go stale. The recursive prototype read itself, and the rooting that protects it, +are unchanged. + +Nothing about property semantics changes: a user-installed +`Object.prototype.then` still makes plain objects thenable, `Object.prototype` +accessors still run, and builtin members still read as functions. + +**Coverage.** Nothing in the tree used `class X extends Promise` before this +change, so the promise-subclass probe had no test at all and a wrong gate would +have broken it silently. `test-files/test_gap_7795_promise_subclass_probe_gate.ts` +exercises the gate's OPEN state (a subclass instance exists, so the probe must +still find its backing cell) alongside the plain-object fast path; +`test-files/test_gap_7795_object_prototype_miss_path.ts` pins the +`Object.prototype` semantics above. The Map/Set, fetch and Temporal gates are +covered by the existing `#6325` / `#7570` / `#7575`, fetch-subclass and +`#5587` suites. + +**This masks a pre-existing GC bug — see #7794.** `asyncpipe` exits 138 under +`PERRY_GC_PROTECT_FROMSPACE=1` on `main`, faulting on a retired from-space +`GC_TYPE_PROMISE`. Removing ~4 key-string allocations per thenable check +removes most of the collection opportunities in that window, so the program +stops faulting under default GC pacing after this change. It is **not** fixed: +with this change applied, `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1` +reproduces the identical fault. #7794 has the root cause +(`async_step_fulfill_thunk` holds two bare `*mut Promise` locals across the +step-body call) and the symbolicated backtrace. diff --git a/crates/perry-runtime/src/object/field_get_set.rs b/crates/perry-runtime/src/object/field_get_set.rs index 7de3e22dbb..f264e41ac6 100644 --- a/crates/perry-runtime/src/object/field_get_set.rs +++ b/crates/perry-runtime/src/object/field_get_set.rs @@ -92,6 +92,18 @@ pub(crate) static FETCH_SUBCLASS_EVER: std::sync::atomic::AtomicBool = /// `None` for any non-object / non-subclass receiver, so callers can fall /// through to their normal dispatch unchanged. pub(crate) unsafe fn fetch_subclass_handle_id(obj: usize) -> Option { + // #7795: nothing has ever stashed a fetch handle, so this probe cannot + // return `Some` — answer from the monotone flag instead of interning a key + // string and running a full recursive `js_object_get_field_by_name`. This + // sits on the ORDINARY-OBJECT PROPERTY-MISS path, which every `await` of a + // plain object reaches through the spec thenable check (`Get(v, "then")`), + // so an un-gated probe is a per-miss allocation in programs that never + // subclass `Request`/`Response`. `FETCH_SUBCLASS_EVER` already existed for + // the `in`-operator fast path (#6748) and is set at the single stash site + // (`attach_fetch_handle_to_this`); this just consults it here too. + if !FETCH_SUBCLASS_EVER.load(std::sync::atomic::Ordering::Relaxed) { + return None; + } // #7526: classify by BAND, not by magnitude. The old floor // (`GC_HEADER_SIZE + 0x1000`) plus `is_valid_obj_ptr` is a magnitude test // only — `is_valid_obj_ptr`'s `HEAP_MIN` is 0x1000 — so every Web Fetch @@ -148,6 +160,17 @@ pub(crate) unsafe fn fetch_subclass_handle_id(obj: usize) -> Option { #[cfg(feature = "temporal")] pub(crate) const TEMPORAL_SUBCLASS_CELL_FIELD: &[u8] = b"__perry_temporal_cell__"; +/// Has any `class X extends Temporal.` instance EVER stashed a cell in +/// this process? The sibling of [`FETCH_SUBCLASS_EVER`], for the same reason: +/// `temporal_subclass_cell` costs a key-string alloc plus a full recursive +/// property read per call, and it is consulted on the ordinary-object +/// property-MISS path. Set at the single stash site +/// (`attach_temporal_cell_to_this`, `global_this/fetch_globals.rs`), which is +/// the only writer of `TEMPORAL_SUBCLASS_CELL_FIELD`. (#7795) +#[cfg(feature = "temporal")] +pub(crate) static TEMPORAL_SUBCLASS_EVER: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + /// If `obj` (a raw heap object address) is a `class X extends Temporal.` /// instance, return the NaN-boxed value of its stashed Temporal cell. Returns /// `None` for any non-object / non-subclass receiver (so callers fall through @@ -155,6 +178,11 @@ pub(crate) const TEMPORAL_SUBCLASS_CELL_FIELD: &[u8] = b"__perry_temporal_cell__ /// longer a live Temporal cell. #[cfg(feature = "temporal")] pub(crate) unsafe fn temporal_subclass_cell(obj: usize) -> Option { + // #7795: see `fetch_subclass_handle_id`. No Temporal subclass instance has + // ever stashed a cell, so this probe cannot return `Some`. + if !TEMPORAL_SUBCLASS_EVER.load(std::sync::atomic::Ordering::Relaxed) { + return None; + } // Reject any address that isn't a plausible heap pointer. Proxy ids live // in [0xF0000, 0x100000) — they pass a naïve `>= GC_HEADER_SIZE + 0x1000` // check but are NOT heap pointers. On Linux (HEAP_MIN = 0x1000) the old diff --git a/crates/perry-runtime/src/object/field_get_set/accessors.rs b/crates/perry-runtime/src/object/field_get_set/accessors.rs index 6f57da0a27..110bfe7e00 100644 --- a/crates/perry-runtime/src/object/field_get_set/accessors.rs +++ b/crates/perry-runtime/src/object/field_get_set/accessors.rs @@ -146,12 +146,13 @@ unsafe fn default_object_prototype_property_value( let _guard = object_prototype_lookup_guard()?; // #7498: THIS IS THE FRAME `PERRY_GC_PROTECT_FROMSPACE=1` FAULTS IN on the // `[...obj.arr]` path — a 56-byte from-space `GC_TYPE_STRING`, i.e. `key`. - // Both arguments are GC-managed and both are live across the two calls - // below before their first use: `js_get_global_this_builtin_value` interns - // its own `"Object"` key (an allocation), and `closure_get_dynamic_prop` - // can run an accessor, which is user code. A copying minor at either point - // moves the key string and the receiver and rewrites only the slots it can - // see; a bare argument is not one. + // Both arguments are GC-managed and both are live across the call below + // before their first use: the recursive `js_object_get_field_by_name` on + // `Object.prototype` can run an accessor, which is user code, and user code + // allocates. A copying minor there moves the key string and the receiver and + // rewrites only the slots it can see; a bare argument is not one. + // (#7795 removed the two resolution calls that used to allocate here as + // well — the rooting is still required for the prototype read itself.) // // Root both before the first of those calls and read each back at its // point of use. NaN-boxed handles only, so this module adds no bare @@ -165,19 +166,21 @@ unsafe fn default_object_prototype_property_value( let receiver_addr = || crate::value::js_nanbox_get_pointer(receiver_h.get_nanbox_f64()) as usize; - let object_ctor = js_get_global_this_builtin_value(b"Object".as_ptr(), 6); - let ctor_value = JSValue::from_bits(object_ctor.to_bits()); - if !ctor_value.is_pointer() { + // #7795: resolve `Object.prototype` from the memoized, GC-healed cache + // instead of re-running `globalThis.Object` (which interns an `"Object"` + // key string) plus a `closure_get_dynamic_prop("prototype")` on EVERY + // ordinary-object property MISS. `object_prototype_addr` performs exactly + // this resolution, caches only a successful one, heals the address through + // the forwarding chain, and is itself a registered GC root + // (`scan_prototype_addr_cache_roots_mut`) — the array index-read fast path + // already depends on it. `Object.prototype` is non-writable and + // non-configurable per spec, so the memo cannot go stale. + let proto_addr = crate::array::object_prototype_addr(); + if proto_addr == 0 { return None; } - let ctor_ptr = ctor_value.as_pointer::() as usize; - let proto = crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype"); - let proto_value = JSValue::from_bits(proto.to_bits()); - if !proto_value.is_pointer() { - return None; - } - let proto_ptr = proto_value.as_pointer::(); - if proto_ptr.is_null() || proto_ptr as usize == receiver_addr() { + let proto_ptr = proto_addr as *mut ObjectHeader; + if proto_ptr as usize == receiver_addr() { return None; } let receiver = crate::value::js_nanbox_pointer(receiver_addr() as i64); diff --git a/crates/perry-runtime/src/object/global_this/fetch_globals.rs b/crates/perry-runtime/src/object/global_this/fetch_globals.rs index 3b31e7d9e0..cacdfc5111 100644 --- a/crates/perry-runtime/src/object/global_this/fetch_globals.rs +++ b/crates/perry-runtime/src/object/global_this/fetch_globals.rs @@ -391,6 +391,10 @@ unsafe fn attach_fetch_handle_to_this(this_box: f64, handle_box: f64) { #[cfg(feature = "temporal")] unsafe fn attach_temporal_cell_to_this(this_box: f64, cell_box: f64) { if let Some(obj) = subclass_this_object_ptr(this_box) { + // #7795: arm the probe gate before the field exists, so no reader can + // observe a stashed cell while the flag still says "never". + crate::object::field_get_set::TEMPORAL_SUBCLASS_EVER + .store(true, std::sync::atomic::Ordering::Relaxed); let key = crate::string::js_string_from_bytes( crate::object::TEMPORAL_SUBCLASS_CELL_FIELD.as_ptr(), crate::object::TEMPORAL_SUBCLASS_CELL_FIELD.len() as u32, diff --git a/crates/perry-runtime/src/object/map_set_subclass.rs b/crates/perry-runtime/src/object/map_set_subclass.rs index 0d5b592c8c..aa937c931e 100644 --- a/crates/perry-runtime/src/object/map_set_subclass.rs +++ b/crates/perry-runtime/src/object/map_set_subclass.rs @@ -27,6 +27,16 @@ use crate::value::{JSValue, POINTER_MASK}; /// `MapHeader`/`SetHeader` pointer. pub(crate) const BACKING_KEY: &[u8] = b"__perry_collection_backing__"; +/// Has any `class X extends Map | Set` instance EVER stashed a backing +/// collection in this process? Same rationale as +/// `promise::subclass::PROMISE_SUBCLASS_EVER`: `subclass_backing_of` costs a +/// key-string alloc plus a full recursive property read per call, and it is +/// reached from generic iteration/dispatch paths in programs that never +/// subclass a collection. Set at the single stash site (the only writer of +/// `BACKING_KEY`). (#7795) +pub(crate) static MAP_SET_SUBCLASS_EVER: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + #[derive(Clone, Copy)] pub(crate) enum CollectionBacking { Map(*mut MapHeader), @@ -75,6 +85,10 @@ unsafe fn instance_object_ptr(this: f64) -> Option<*mut ObjectHeader> { /// real Maps/Sets, ordinary objects, and non-objects — so callers fall through /// to their existing handling. pub(crate) fn subclass_backing_of(value: f64) -> Option { + // #7795: no Map/Set subclass instance exists, so this cannot return `Some`. + if !MAP_SET_SUBCLASS_EVER.load(std::sync::atomic::Ordering::Relaxed) { + return None; + } unsafe { let obj = instance_object_ptr(value)?; let backing = js_object_get_field_by_name_f64( @@ -180,6 +194,11 @@ pub(crate) enum CollectionKind { /// synthesize the built-in default iterator when none exists. Returns `false` /// for non-subclass values. pub(crate) fn subclass_has_iterator_override(value: f64) -> bool { + // #7795: only ever asked about Map/Set SUBCLASS instances; with no subclass + // in the process the answer is `false` without the symbol lookups below. + if !MAP_SET_SUBCLASS_EVER.load(std::sync::atomic::Ordering::Relaxed) { + return false; + } unsafe { let Some(obj) = instance_object_ptr(value) else { return false; @@ -351,6 +370,9 @@ pub extern "C" fn js_map_set_subclass_init(this: f64, kind: i32, iterable: f64) set as *mut u8 }; + // #7795: arm the probe gate before the field exists, so no reader can + // observe a stashed backing while the flag still says "never". + MAP_SET_SUBCLASS_EVER.store(true, std::sync::atomic::Ordering::Relaxed); let key = crate::string::js_string_from_bytes(BACKING_KEY.as_ptr(), BACKING_KEY.len() as u32); let backing_bits = JSValue::pointer(backing_ptr as *const u8).bits(); js_object_set_field_by_name(obj, key, f64::from_bits(backing_bits)); diff --git a/crates/perry-runtime/src/promise/subclass.rs b/crates/perry-runtime/src/promise/subclass.rs index 8460d863d1..985569858a 100644 --- a/crates/perry-runtime/src/promise/subclass.rs +++ b/crates/perry-runtime/src/promise/subclass.rs @@ -27,6 +27,17 @@ use super::Promise; /// `Promise` cell pointer. pub(crate) const BACKING_KEY: &[u8] = b"__perry_promise_backing__"; +/// Has any `class X extends Promise` instance EVER stashed a backing cell in +/// this process? `subclass_backing_promise` costs a key-string alloc plus a +/// full recursive `js_object_get_field_by_name_f64` per call, and it is +/// consulted from `js_object_get_field_by_name`'s miss path — which every +/// `await` of a plain object reaches via the spec thenable check +/// (`Get(v, "then")`). A program that never subclasses `Promise` should pay a +/// relaxed load, not an allocation. Set at the single stash site +/// (`js_promise_subclass_init`, the only writer of `BACKING_KEY`). (#7795) +pub(crate) static PROMISE_SUBCLASS_EVER: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + fn raw_ptr_from_value(value: f64) -> usize { let bits = value.to_bits(); let jsval = JSValue::from_bits(bits); @@ -66,6 +77,11 @@ unsafe fn instance_object_ptr(this: f64) -> Option<*mut ObjectHeader> { /// real `Promise` cells, ordinary objects, and non-objects — so callers fall /// through to their existing handling. pub(crate) fn subclass_backing_promise(value: f64) -> Option<*mut Promise> { + // #7795: no `class X extends Promise` instance exists, so this cannot + // return `Some`. Answer from the monotone flag. + if !PROMISE_SUBCLASS_EVER.load(std::sync::atomic::Ordering::Relaxed) { + return None; + } unsafe { let obj = instance_object_ptr(value)?; let backing = js_object_get_field_by_name_f64( @@ -132,6 +148,9 @@ pub extern "C" fn js_promise_subclass_init(this: f64, executor: f64) -> f64 { crate::closure::js_closure_call1(reject_closure, reason); } + // #7795: arm the probe gate before the field exists, so no reader can + // observe a stashed backing cell while the flag still says "never". + PROMISE_SUBCLASS_EVER.store(true, std::sync::atomic::Ordering::Relaxed); let key = crate::string::js_string_from_bytes(BACKING_KEY.as_ptr(), BACKING_KEY.len() as u32); let backing_bits = JSValue::pointer(promise as *const u8).bits(); js_object_set_field_by_name(obj, key, f64::from_bits(backing_bits)); diff --git a/test-files/test_gap_7795_object_prototype_miss_path.ts b/test-files/test_gap_7795_object_prototype_miss_path.ts new file mode 100644 index 0000000000..cdd4a49d22 --- /dev/null +++ b/test-files/test_gap_7795_object_prototype_miss_path.ts @@ -0,0 +1,65 @@ +// #7795: the ordinary-object property-MISS path now resolves the default +// `Object.prototype` from the memoized, GC-healed `object_prototype_addr()` +// cache instead of re-running `globalThis.Object` (which interns an `"Object"` +// key string) plus `closure_get_dynamic_prop("prototype")` on every miss. +// +// The miss path is what answers `Get(v, "then")` for the spec thenable check on +// every `await` of a plain object, so it must keep answering EXACTLY as before: +// builtin `Object.prototype` members still read as functions, absent keys still +// read `undefined`, and a USER-installed `Object.prototype` property must still +// be visible on plain objects (including making them genuinely thenable). + +const o: any = { a: 1 }; + +// Builtin Object.prototype members must still resolve through the miss path. +console.log("toString", typeof o.toString); +console.log("hasOwnProperty", typeof o.hasOwnProperty); +console.log("valueOf", typeof o.valueOf); +console.log("isPrototypeOf", typeof o.isPrototypeOf); +console.log("propertyIsEnumerable", typeof o.propertyIsEnumerable); +console.log("toLocaleString", typeof o.toLocaleString); +console.log("constructor", typeof o.constructor); +console.log("call-toString", o.toString()); +console.log("call-hasOwn", o.hasOwnProperty("a")); + +// A genuinely absent key still reads undefined. +console.log("absent", o.then, o.nope, o.zzz); + +// A user-installed Object.prototype property must be visible on plain objects. +(Object.prototype as any).marker = 42; +console.log("proto-marker", o.marker); +const fresh: any = {}; +console.log("proto-marker-fresh", fresh.marker); +delete (Object.prototype as any).marker; +console.log("proto-marker-deleted", o.marker, fresh.marker); + +// An accessor installed on Object.prototype must still run. +Object.defineProperty(Object.prototype, "acc", { + get() { + return 99; + }, + configurable: true, +}); +console.log("proto-accessor", ({} as any).acc); +delete (Object.prototype as any).acc; +console.log("proto-accessor-gone", ({} as any).acc); + +// The await path this optimises: a plain object is NOT thenable... +async function plain(): Promise<{ v: number }> { + return { v: 7 }; +} +plain().then((r: { v: number }) => { + console.log("await-plain", r.v); + + // ...but installing `then` on Object.prototype DOES make plain objects + // thenable, which the miss path must still observe. + (Object.prototype as any).then = function (res: (x: number) => void) { + res(123); + }; + Promise.resolve() + .then(() => ({ plainObj: true }) as any) + .then((v: any) => { + console.log("proto-then-assimilated", v); + delete (Object.prototype as any).then; + }); +}); diff --git a/test-files/test_gap_7795_promise_subclass_probe_gate.ts b/test-files/test_gap_7795_promise_subclass_probe_gate.ts new file mode 100644 index 0000000000..f0d005fe1a --- /dev/null +++ b/test-files/test_gap_7795_promise_subclass_probe_gate.ts @@ -0,0 +1,50 @@ +// #7795: `subclass_backing_promise` is now gated on a monotone +// `PROMISE_SUBCLASS_EVER` flag, so a program that never subclasses `Promise` +// stops paying a key-string allocation plus a full recursive +// `js_object_get_field_by_name` on every ordinary-object property MISS. That +// miss path is reached by the spec thenable check (`Get(v, "then")`) that runs +// on every `await`/resolve of a plain object, which made it the single hottest +// thing in an async service pipeline. +// +// A gate is only safe if the OPEN state is exercised: nothing in the tree used +// `class X extends Promise` before this file, so the probe had no coverage at +// all and a wrong gate would have silently broken Promise subclassing. This +// asserts the flag is armed at the stash site and the subclass still behaves. + +class MyPromise extends Promise { + tag(): string { + return "mine"; + } +} + +// Constructing the subclass is what arms the gate (the stash site). +const p = new MyPromise((resolve) => { + resolve(41); +}); + +console.log("is-mypromise", p instanceof MyPromise); +console.log("is-promise", p instanceof Promise); +console.log("tag", p.tag()); + +// The backing cell must still be reachable through the hidden field, i.e. the +// gated probe must return `Some` now that a subclass instance exists. +p.then((v: number) => { + console.log("then", v + 1); +}); + +const r = MyPromise.resolve(7); +console.log("static-resolve-type", r instanceof Promise); +r.then((v: number) => console.log("static-then", v)); + +async function useIt(): Promise { + const v = await p; + return v + 1; +} +useIt().then((v: number) => console.log("await", v)); + +// A plain (non-subclass) object must still resolve as a NON-thenable — this is +// the fast path the gate protects. +async function plain(): Promise<{ v: number }> { + return { v: 5 }; +} +plain().then((o: { v: number }) => console.log("plain-await", o.v));