perf(runtime): publish a transition edge when a descriptor writes a value (#10868) - #10901
proggeramlug wants to merge 1 commit into
Conversation
…alue (#10868) `Object.defineProperty` minted 3 ShapeIds per object for a data descriptor and 4 for an accessor, linearly and without bound, while the transition cache reported lookups and ZERO inserts. This makes the data case constant. Measured rather than read. Instrumenting every decline reason showed exactly one predicate refusing, 1000 times out of 1000, at `object_ops/keys_array.rs`: defineProperty key-add, edge outcomes: no publish: new_index >= inline_capacity 1000 eligible 1000 The receiver IS eligible and the lookup DOES run. Why it refuses every time: `INLINE_SLOT_FLOOR` is 2, and #7916 documents it as a pure FOOTPRINT dial -- "purely a growth-headroom dial for objects that gain properties by name after birth". A two-field literal therefore allocates exactly two slots and has zero growth headroom, so the first key added after birth lands at `new_index == inline_capacity == 2`, spills to overflow, and the edge is never published. Every receiver forks onto a private keys array, and from there the fork cascade does the rest: once the predecessor ShapeId is private, even #10287's DETERMINISTIC generation is deterministic in a value no other object shares, which is why an accessor costs 4 ids and not 1. A dial tuned for bytes silently decided whether transition edges get published at all; neither subsystem could see that from its own side. Raising the floor is not the fix: it trades footprint against a standing RSS budget and only moves the cliff to four-field literals. The correct behaviour already exists next door -- `field_set_by_name/tail.rs`'s overflow arm publishes the edge with no inline-capacity gate at all, which is why the `pool` control (the same objects grown by [[Set]]) already mints a constant 4,534 ids however much work it does. This is missing wiring, not a missing capability. Two changes, because publishing alone would have measured as a wash: the adopter gate refused overflow-located edges too, on the stated grounds that "a keys-only install (an accessor claiming its slot) writes no value, so the overflow entry such an edge implies would never be created". That reason is correct for accessors and over-broad for data. So publish the edge when the install will write a value, AND let the adopter take one in that case. `ensure_key_in_keys_array_for_value` is the opt-in and `define_property_force_store_value` is its only caller; every keys-only claim keeps today's behaviour by construction. `cached_target_fits` is untouched (it guards a different hazard), and the live-bound bump is now guarded in BOTH adopt arms -- unreachable in the first-key arm today, since its precondition is a null keys array, but an unguarded bump beside a gate that now admits overflow slots is a trap for whoever widens that precondition. Measured with #10885's census. Six controls, two arms built from their own trees, `cmp`-distinct binaries, output byte-identical to node on every row; mints via `PERRY_SHAPE_MINT_DIAG`, instructions fitted N=2,000 -> 20,000, min of 3: fixture mints @1k/@10k before -> after instr/iter before -> after same 1 / 1 -> 1 / 1 93.58 -> 93.58 pool 4,534 / 4,536 -> 4,534 / 4,536 15596.14 -> 15595.14 del 5,516 / 14,516 -> 5,516 / 14,516 21365.89 -> 21387.20 accd 3,002 / 30,002 -> 5 / 5 16188.99 -> 12539.77 (-22.5%) acc 4,002 / 40,002 -> 4,002 / 40,002 19244.19 -> 19315.15 diff 22,516/184,516 -> 22,516/184,516 220937.30 -> 220936.49 `accd` goes CONSTANT -- 5 ids whether it runs 1,000 iterations or 10,000 -- with 22.5% fewer instructions, so this is a speedup and not merely a smaller counter. `acc` being unchanged TO THE ID is not a missed opportunity; it is the result that makes the change sound. The accessor is a keys-only claim whose implied overflow entry is never created, so it must keep the private path. A change that improved both rows would have broken the keys-only-install argument rather than fixed the data one. tsc (`ts.transpileModule`, typescript 5.8.2 from source, output identical to node): MINTS 2,288,352 -> 2,287,800 and distinct key-NAME lists 10,867 -> 10,867. Flat, as predicted before measuring: tsc's mints are 97.4% `fresh_keys_known_list` + `key_count` from the [[Set]] CoW path, which is a different fork source. The requirement here was must-not-regress, and a flat tsc row confirms the attribution. The layout-count invariant held, so shape IDENTITY did not move -- only mint volume. `cargo test -p perry-runtime` on both arms back to back, same host state, skipping the one test that aborts the process on main (`bun_compat::plugin::tests::calls_setup_for_objects_and_functions_without_ running_hooks`, a non-unwinding panic, pre-existing): 4,204 passed / 0 failed on both. `cargo check` clean in both feature configurations. This is ONE fork source among several, not a fix for #10868. The exit criterion there is: mints ~= distinct layouts on tsc, no abort, and same layout + same facts => same ShapeId always. Reaching it needs canonical shape identity, because the authoritative dedup (`by_facts`) keys on the keys-array ADDRESS and that address is produced by a lossy 16,384-entry direct-mapped cache.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Acceptance-matrix verdict: no cell moves — the lever does not touch the access pathThe ONE PATH acceptance matrix ( Binaries asserted by content, not by status: base
Per-cell diff: 0 regressed, 0 voided, 0 diverged, 0 improved, 0 expected. Every one of the Why nothing can move — from sourceThe new behaviour is gated on Does the lever touch the access path at all? — not on its own path eitherSource-derived, not measured (the two arms differ only in runtime, and verifying it would need That is the useful reading for lane 8: lever (iii) fixes shape identity (step 2.5's mint One thing this run caught in the harnessThe first diff refused to compare the two arms: the link-mode fingerprint compared binary |
|
Landed via merge train 253 (#10918) as v0.5.1633 — merge commit Expedited at the owner's request. Carried with nine other PRs; the stacked ones (#10899/#10900 on #10886, #10901 on #10885) had only their unique commits taken. Evidence on the assembled tree: Closing here rather than merging — a train lands the commits directly. |
1 participant
Lever (iii) of #10868. Stacked on #10885 (the census), because every number here is taken with that instrument.
Object.definePropertymints 3 ShapeIds per object for a data descriptor and 4 for an accessor, linearly and without bound, while the transition cache reports lookups and zero inserts. This makes the data case constant.The predicate, measured rather than read
I instrumented every decline reason rather than reasoning about the source. On
accd(one data-descriptor install per object), N = 1,000:The receiver is eligible and the cache lookup does run. Exactly one predicate refuses, at
object_ops/keys_array.rs:306:new_index < inline_capacity.Why it refuses every time — a footprint dial governing shape publication
INLINE_SLOT_FLOORis 2, and #7916's own doc comment says what it is for:A two-field literal
{a, b}therefore allocates exactly two slots and has zero growth headroom. The first key added after birth lands atnew_index == inline_capacity == 2, spills to overflow, and the edge is never published. Every receiver forks onto a private keys array, and from there the fork cascade (#10868) does the rest: once the predecessor ShapeId is private, even #10287's deterministic generation is deterministic in a value no other object shares, so every later transition on that object mints too. That is why an accessor install costs 4 ids and not 1.A dial tuned purely for footprint silently decided whether transition edges get published. Neither side could see it: #7916 was reasoning about bytes and correctly proved
alloc_limitis a fixed point of the allocation; the transition cache reusedalloc_limitas a proxy for "can this key be shared". Raising the floor is not the fix — it trades footprint against a standing RSS budget, and it only moves the cliff to four-field literals.The correct behaviour already existed next door
field_set_by_name/tail.rs's overflow arm publishes the edge with no inline-capacity gate at all, and its comment gives the reason in the words this fix needs. One path handles the situation; the other refused it. That is missing wiring, not a missing capability — and it is why thepoolcontrol (the same objects grown by[[Set]]instead) already mints a constant 4,534 ids however much work it does.Two changes, because publishing alone would have measured as a wash
The adopter gate refused overflow-located edges too, with its own stated reason:
That reason is correct for accessors and over-broad for data. A data descriptor does write the value, so the objection does not apply to it. So: publish the edge when the install will write a value, and let the adopter take one in that case.
ensure_key_in_keys_array_for_valueis the opt-in anddefine_property_force_store_value— the data-descriptor path — is its only caller. Every keys-only claim (accessor install, built-in getter, attribute-only descriptor) keeps today's behaviour by construction.cached_target_fitsis untouched: it guards a different hazard (a target whose live inline bound exceeds this receiver's allocation, which would have the collector trace past the end of the object). The live-bound bump is now guarded in both adopt arms — it is unreachable in the first-key arm today, since that arm's precondition is a null keys array and itsslot_idxis always 0, but an unguarded bump beside a gate that now admits overflow slots is a trap for whoever widens that precondition.Measured: the
accrow is the load-bearing oneSix controls, two arms built from their own trees,
cmp-distinct binaries, output byte-identical to node on every row. Mints viaPERRY_SHAPE_MINT_DIAG; instructions fitted N = 2,000 → 20,000, min of 3.samepooldelaccd(data descriptor)acc(accessor)diffaccdgoes constant — 5 ids whether it runs 1,000 iterations or 10,000 — and 22.5 % fewer instructions, so this is a speedup and not merely a smaller counter.accbeing unchanged to the id is not a missed opportunity; it is the result that makes the change sound. The accessor is a keys-only claim whose implied overflow entry is never created, so it must keep the private path. A change that improved both rows would have broken the keys-only-install argument rather than fixed the data one. The reviewer instinct that more green rows are better is exactly inverted here.tsc: flat, as predicted before measuring
ts.transpileModule, typescript 5.8.2 compiled from source, output identical to node:miss_COLLIDEThe prediction was on the record before the run: tsc's mints are 97.4 %
fresh_keys_known_list+key_countfrom the[[Set]]CoW path, a different fork source, so lever (iii) should not move it. The requirement was must not regress; a flat row confirms the attribution rather than disappointing it.The layout-count invariant held — 10,867 before and after. Shape identity did not move; only mint volume did.
Verification
cargo test -p perry-runtimeon both arms back to back under the same host state, skipping the one test that aborts the process on main (bun_compat::plugin::tests::calls_setup_for_objects_and_functions_without_running_hooks, a non-unwinding panic, pre-existing): 4,204 passed / 0 failed on both.cargo checkclean in both feature configurations.strings | grep), becauseperry-build-slot.sh's stale-check repair relinks with default features and had silently stripped the instrument out of an arm once./root/fr/mx/{same,pool,del,accd,acc,diff}.tson perrymaster.Scope
This is one fork source among several, not a fix for #10868. That issue's exit criterion is mints ≈ distinct layouts on tsc, no abort, and same layout + same facts ⇒ same ShapeId always. Reaching it needs canonical shape identity: the authoritative dedup (
by_facts) never fails, but it keys on the keys-array address, and that address is produced by a lossy 16,384-entry direct-mapped cache with no store behind it. Lever (iii) removes one way an address forks; it does not make the key canonical.Stacked on #10885 — every number here is taken with that instrument.