diff --git a/changelog.d/8971-key-scan-kills-read-lane.md b/changelog.d/8971-key-scan-kills-read-lane.md new file mode 100644 index 0000000000..537af0c66b --- /dev/null +++ b/changelog.d/8971-key-scan-kills-read-lane.md @@ -0,0 +1,36 @@ +The last four per-element key scans on the property paths now resolve through +the shape hash index. + +#8936 and #8950 replaced the `js_array_get` + `js_string_key_matches` walks on +the `[[Set]]`, `delete` and `[[Get]]`-fallback paths, but four copies survived +in the plan-miss and cache-miss fallbacks: + +- `js_object_get_field_by_name`'s FAST LANE — the read-plan cache's miss path, + a full walk of up to 4096 keys. The plan is epoch-guarded and flushed on + every GC and on descriptor / prototype / delete mutations, so this ran in + full each time it was flushed. It put `js_string_key_matches` at **9.6% self + time** in a computed-key read loop, second only to the entry itself. +- the write fast path's read-plan miss fallback, and +- the write tail and the read tail, whose loops walked every key (two of them + via `js_array_get`, which additionally probes each index for a per-index + accessor). + +All four go through `keys_find_slot_by_key_ptr`: shape hash index first, raw +dense-slot scan as its own fallback and correctness backstop. The two tail +sites keep their original `js_string_key_matches` test as the gate, so the +resolver can only narrow the candidate slot, never widen what is accepted. + +Interleaved A/B, min-of-15 at quiet load (node on the same host in brackets): + +| loop | main | this PR | | +|---|---|---|---| +| read only | 38 ms (25) | **34 ms** | −10.5% min, −18.6% mean | +| combined overwrite | 79 ms (29) | **77 ms** | −2.5% min, −13.2% mean | +| write only | 44 ms (23) | 43 ms | −2.3% | + +The read-only loop is where this lands: in the combined loop the write primes +the read plan, so the read never reaches the miss path at all. + +Computed-key differential output (delete/re-add ordering, `Map`/`Set` keys, +the SSO boundary, non-ASCII, floats, negatives, 1e21) is byte-identical to +node. Suite: 2779 passed, 0 failed. 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 bde9b72270..d9fdf0e043 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 @@ -189,23 +189,39 @@ pub extern "C" fn js_object_get_field_by_name( let key_count = crate::array::keys_array_len_capped_to_capacity(keys); if key_count <= 4096 { - for i in 0..key_count { - let kv = crate::array::keys_array_slot(keys, i as u32); - if crate::string::js_string_key_matches(kv, key) { - super::super::prop_plan::read_plan_record( - keys as usize, - key as usize, - i as u32, - ); - return if i < alloc_limit { - super::accessors::js_object_get_field(o, i as u32) - } else { - match super::super::overflow_get(raw, i) { - Some(b) => JSValue::from_bits(b), - None => JSValue::undefined(), - } - }; - } + // #8936/#8950's shared resolver: the shape's + // hash index answers in O(1), with the raw + // dense-slot scan as its own fallback. The + // open-coded `keys_array_slot` + + // `js_string_key_matches` walk this replaces + // was the read-plan cache's MISS path, so it + // ran in full — up to `key_count` string + // compares — every time the epoch-guarded + // plan was flushed (each GC, and every + // descriptor / prototype / delete mutation). + // On a 500-key receiver that put + // `js_string_key_matches` at 9.6% self time + // in a computed-key read loop, second only to + // this function itself. + if let Some(i) = crate::object::keys_find_slot_by_key_ptr( + keys, + key_count as u32, + key, + ) { + let i = i as usize; + super::super::prop_plan::read_plan_record( + keys as usize, + key as usize, + i as u32, + ); + return if i < alloc_limit { + super::accessors::js_object_get_field(o, i as u32) + } else { + match super::super::overflow_get(raw, i) { + Some(b) => JSValue::from_bits(b), + None => JSValue::undefined(), + } + }; } } } diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs index 6e90a4a9ab..f56ff584e1 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs @@ -1670,7 +1670,12 @@ pub(crate) fn get_field_by_name_object_tail( return JSValue::undefined(); } - for i in 0..key_count { + // #8936/#8950's resolver narrows the candidate to one slot via the + // shape hash index instead of walking every key; the original match + // below still gates the hit, so this cannot widen what is accepted. + if let Some(i) = crate::object::keys_find_slot_by_key_ptr(keys, key_count as u32, key) + .map(|v| v as usize) + { let key_val = crate::array::keys_array_slot(keys, i as u32); // #1781: accept inline SSO short keys here too — the // slow-path lookup is what backs `obj[k]` for ≤5-byte diff --git a/crates/perry-runtime/src/object/field_set_by_name/fast_paths.rs b/crates/perry-runtime/src/object/field_set_by_name/fast_paths.rs index c9001ca497..0f8c379d89 100644 --- a/crates/perry-runtime/src/object/field_set_by_name/fast_paths.rs +++ b/crates/perry-runtime/src/object/field_set_by_name/fast_paths.rs @@ -111,13 +111,15 @@ pub(crate) unsafe fn try_existing_own_data_overwrite( if key_count > 4096 { return false; } - for i in 0..key_count { - let kv = crate::array::js_array_get(keys, i as u32); - if crate::string::js_string_key_matches(kv, key) { - super::prop_plan::read_plan_record(keys_addr, key_addr, i as u32); - own_idx = Some(i as u32); - break; - } + // The write twin of the read lane's resolver: shape hash index first, + // raw dense-slot scan as its own fallback. The open-coded walk this + // replaces ran `js_array_get` (which additionally probes for per-index + // accessors) plus a string compare per key, in full, every time the + // epoch-guarded read plan was flushed — the same miss-path cost #8936 + // and #8950 removed from their sides of the property paths. + own_idx = crate::object::keys_find_slot_by_key_ptr(keys, key_count as u32, key); + if let Some(i) = own_idx { + super::prop_plan::read_plan_record(keys_addr, key_addr, i); } } let Some(idx) = own_idx else { diff --git a/crates/perry-runtime/src/object/field_set_by_name/tail.rs b/crates/perry-runtime/src/object/field_set_by_name/tail.rs index 7c5b1fee44..5d5d80ae42 100644 --- a/crates/perry-runtime/src/object/field_set_by_name/tail.rs +++ b/crates/perry-runtime/src/object/field_set_by_name/tail.rs @@ -856,7 +856,13 @@ pub(crate) fn set_field_by_name_object_tail( return; } - for i in 0..key_count { + // #8936/#8950's resolver narrows the candidate to one slot via the + // shape hash index instead of walking every key (and probing each for + // per-index accessors via `js_array_get`); the original match below + // still gates the hit, so this cannot widen what is accepted. + if let Some(i) = crate::object::keys_find_slot_by_key_ptr(keys, key_count as u32, key) + .map(|v| v as usize) + { let key_val = crate::array::js_array_get(keys, i as u32); // #1781: SSO-aware match — keys are stored as either a // STRING_TAG pointer OR a SHORT_STRING_TAG inline value for