Repository navigation
chore: merge train 247 (v0.5.1626) - #10853
Merged
Merged
Conversation
Lane 1 of the single-fast-path property-access work. The emitted read path wants to prove everything it needs from ONE compare of the ShapeId word at receiver+4. Three invariants have to hold for that, and two of them did not. RULE 1 - every descriptor change changes an ordinary object's ShapeId. * `clear_object_descriptors` made no shape call at all. It is a BULK removal (every key goes back to writable/enumerable/configurable), so a cache primed while the object was frozen kept serving the frozen answer. It now transitions, and removes through the owner index instead of two O(table) `retain` scans - which also retires the `HANDLE_HAS_DESCRIPTORS` latch that existed only to skip those scans, and with it the silent no-op the function performed for a heap owner while no handle had ever taken a descriptor. * Seven raw `remove()` calls on the descriptor tables lived OUTSIDE descriptor_state.rs (array index redefine x2, defineProperty x3, handle expando x2). They skipped the shape transition and left the owner index - the authoritative answer to "which keys does this owner have" - naming keys the tables no longer held, so `accessor_descriptor_keys_for_obj` kept reporting deleted accessors. All seven now go through `clear_accessor_descriptor` / `clear_property_attrs`. * `transfer_descriptor_owner` is correctly shape-free: it re-keys ONE object whose allocation moved (array growth), and a transition there would retire caches for a shape the object still has. Documented, and made safe for any caller: if the replacement cell does not already carry the source's descriptor flag, the new owner is routed through the funnel rather than silently acquiring descriptors behind an unchanged header. * The funnel's scope is now stated where it is implemented: typed arrays return early, and every non-GC_TYPE_OBJECT kind fails the obj_type test, so for those receivers the descriptor flag is never set AND the shape is never transitioned. RULE 2 - ShapeIds are never reused, and nothing else mints into their range. The design note listed this as holding, with "class ids stay below 0x8000_0000 - TO CONFIRM". They do not: `SYNTHETIC_CLASS_ID_BASE` was 0x8000_0000, the ShapeId floor, and both counters started there - so synthetic class id #1 IS shape #1. A `class X extends someFunction` resolves its parent through `dynamic_value_class_id` -> `function_class_id` to that synthetic id, and `js_object_alloc_class_dynamic_parent` writes it into +4, where `is_shape_id` accepts it and `shape_record_by_id` answers with an unrelated live shape's ordered keys and live-inline-slot bound. Synthetic ids move to [0xC000_0000, 0xFFFF_0000) - a band no other producer uses - and are minted through one bounded allocator that parks at the end instead of wrapping. A `const _: () = assert!(...)` makes the disjointness a compile error to undo. RULE 3 - no pointer-tagged non-object cell holds a ShapeId-range value at +4. * `new Int8Array(2**31)` wrote `capacity = 0x8000_0000` - the first ShapeId the process ever mints. The element cap moves from u32::MAX to i32::MAX, matching `new Uint8Array(n)`, `new ArrayBuffer(n)` and `Buffer.alloc(n)`, which already stop there. * A Promise's +4 is struct padding, and neither `gc_malloc` nor the arena zeroes reused memory: a Promise born at a dead shaped object's address inherited its LIVE ShapeId. The padding is now a named zeroed field. * Debug assertions at the capacity writes for Array (alloc + grow), Map, Set, Buffer, TypedArray and NativeTypedView, and a table in the new `object/shape_rule3.rs` classifying all 21 GC kinds, walked by a test that fails when a new kind arrives unclassified. Counters: the property-get IC already counts misses by 17 causes under PERRY_IC_DIAG, but nothing counted hits (they never reach the runtime). `ic_note_hit` / `js_ic_diag_note_hit` add the hit side behind the same arming, so a lane removing a guard can tell "reads now hit" from "reads still fall to the miss handler and are merely correct". Tests: crates/perry-runtime/src/object/shape_rules_tests.rs (rules 1-2, one per mutator) and the tests module of object/shape_rule3.rs (rule 3).
`re_registering_the_same_edge_flushes_nothing` snapshots the PROCESS-global prop-plan epoch, runs eight no-op re-registrations, and asserts the epoch did not move. Any other libtest thread installing a descriptor or registering a class inside that window moves it, so the test failed for reasons unrelated to re-registration (observed: off by exactly one, and it passes under `--test-threads=1`). It now takes `global_side_table_test_lock`, which every descriptor-mutating test already holds.
Second lane-1 change, stacked on #10824. That PR left rule 3 -- "no pointer-tagged non-object cell holds a ShapeId-range value at payload +4" -- open for six kinds, and its own table pinned them: ArrayHeader, BufferHeader, DateCell, MapHeader, NativeTypedViewHeader, SetHeader. While any of them can produce a +4 word inside [0x8000_0000, 0xC000_0000), a successful shape compare does not prove the receiver is an ordinary object, so the emitted read path has to keep loading the GC header first. This closes all six. Five are COUNTS, and a count has a bound. Each is now rejected in RELEASE at its allocation and growth funnel, before any memory is reserved, with the error that kind already raises one byte further out: * ArrayHeader::capacity -- `array_capacity_or_throw` at all seven alloc funnels, at `js_array_grow`, and at `pairs_alloc`; `RangeError: Invalid array length`. The bound also removes the `old_capacity * 2` u32 overflow the grow path carried, and the `as u32` truncation in `pairs_alloc`. * MapHeader / SetHeader::capacity -- `js_map_alloc` / `js_set_alloc` and both `ensure_capacity` doublings; `RangeError: Map/Set maximum size exceeded`. * BufferHeader::capacity -- `buffer_alloc` and `alloc_shared_sab` throw `RangeError: Array buffer allocation failed`. The two wrappers over foreign memory (`buffer_alloc_foreign`, `rebind_foreign_buffer`) are reached from `extern "C"` Node-API entry points where a throw has nowhere to land, so they clamp -- the policy `instance_memory_span` already applied to a wasm memory wider than an i32 byte count -- and every caller of them now refuses an over-range length itself (napi `checked_length`, `bun_ffi::memory`, `webassembly`). * TypedArrayHeader::capacity -- the same bound at `typed_array_alloc`, which the internal callers reach without passing the constructor's check. * NativeTypedViewHeader::capacity -- `js_native_arena_view`, checked before the owner is resolved so the diagnosis names the length. The sixth is DATA, and no bound applies to it. `DateCell` held `ts: f64` at +0, so `new Date(-1)` -- 0xBFF0_0000_0000_0000 -- put 0xBFF0_0000 at +4, a live-looking ShapeId. The cell is relaid out instead: `meta` first, the timestamp at +8. Size and alignment are unchanged, +4 is now the high half of a heap pointer (zero while `meta` is null), and nothing outside date.rs reads the cell by offset -- `object::cell_meta_slot` and `thread.rs` use the named fields and perry-codegen has no DateCell offsets at all, only `js_date_*` calls. `const _: () = assert!(offset_of!(DateCell, ts) == 8)` pins it. What lane 4 may now assume: for any pointer-tagged value that passes the tag test, the u32 at payload +4 equals a live object ShapeId only if the cell is a GC_TYPE_OBJECT carrying that shape. The GC-kind load is removable from the emitted read path. User-visible maxima lowered (none reachable by a program that was working): dense array capacity, Map entries and Set elements stop at 2^31-1 (16/32/16 GiB of live storage); `Buffer.from(arrayLike)`, `Buffer.concat` and `buffer::copy_bytes` stop at 2 GiB-1 instead of clamping at 4 GiB-1, matching `Buffer.alloc` / `new ArrayBuffer` / the typed arrays, which already did; `bun:ffi` `toArrayBuffer`/`viewSource` and napi external buffers stop there too; a wasm memory's JS-visible `.buffer` exposes at most 2 GiB-1, which `instance_memory_span` already did for the instance path. Tests: `object/shape_rule3.rs` -- the pinned set of fence-keeping kinds is now empty, and eight new tests drive the real funnels at exactly SHAPE_ID_BASE and assert the rejecting message, so "refused by rule 3" is distinguishable from "the allocator refused". The DateCell test first asserts that the high word of -1.0 really is inside the ShapeId range, so it cannot quietly stop testing anything, then walks both ends of the JS time-value range plus NaN and checks the value round-trips. Verified falsifiable: reverting the DateCell layout, dropping the native-arena cap, or changing the array message fails exactly the tests that cover them. 4129 perry-runtime tests pass single-threaded; a Date/Map/Set/Buffer/typed-array program compiled by perry is byte-identical to node, including sparse `new Array(2**31)` and `length = 2**32-1`.
…_state.rs `crates/perry-runtime/src/object/descriptor_state.rs` reached 2042 lines and failed `scripts/check_file_size.sh` (cap 2000). Split boundary: everything that keys off an owner ADDRESS changing or going away moves to the new sibling `descriptor_state/owner_lifecycle.rs` -- `prune_dead_descriptor_owner_entries`, its minor-scoped twin, the shared `remove_descriptor_owner_entries` helper, `clear_object_descriptors`, `transfer_descriptor_owner` and the metadata-rewrite-phase translation `rewrite_descriptor_owner`. The install/lookup side of the same tables stays in the parent. That is a real seam rather than a line count: the five moved functions are the only ones in the file whose subject is the owner key itself, they already call one another, and `gc_scan.rs` -- the existing sibling -- is their only outside caller for `rewrite_descriptor_owner`. Follows the `gc_scan.rs` / `young.rs` idiom already in that directory: private `mod` plus explicit named `pub(crate) use` re-exports, so `crate::object::`'s own re-export list in `object/mod.rs` is unchanged. `gc_scan.rs` imports `rewrite_descriptor_owner` by name instead of relying on the parent glob, which would not have carried it. No function body changed. The only semantic edit is `rewrite_descriptor_owner` going from private to `pub(super)` so the sibling scanner can still reach it.
…t_ptr `scripts/raw_handle_debt.py` failed with `object/shape_rule3.rs: 1 raw-handle debt site(s) in a module with no ceiling`. The site is `rule3_array_growth_to_the_shape_id_floor_is_refused`, which rooted the array and then read the address back out with a bare `get_raw_mut_ptr` to hand to `js_array_grow`. That is the argument-position shape `with_mut_ptr` exists for -- a scoped pointer passed to a runtime entry point -- so it is converted rather than granted a ceiling. `across_*` would not fit: the call consumes the pointer, it does not need one re-read after the call. A ceiling would also not have been sufficient. The recorded total was already 907 against a baseline of 906, so the total rule would have fired the moment the per-module rule stopped. Converting the site puts both back at 906.
…descriptor_state split Both are fallout from the 2000-line-cap split and the new shape_rule3.rs, not from the PRs' logic. string_payload_access_inventory went 350 -> 351: shape_rule3.rs's thrown_message test helper open-coded the StringHeader payload offset. Routed through crate::string::string_data instead. origin/main is green on the gate, so the +1 was this train's. gc_rekeyed_key_tables reported rewrite_descriptor_owner as both an unclassified site at its new path and a stale entry at the old one. Re-pointed, not re-classified: the moved body is byte-identical apart from pub(super). The why's line reference was updated with it.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge train 247 — the object-model invariants the single-compare property read depends on, plus the two gate fixes needed to land them.
Contents
object model: enforce the three shape rules the single read path needsobject model: close rule 3 — no non-object cell can hold a live ShapeId at payload+4refactor: split descriptor-owner lifecycle out ofdescriptor_state.rs(2000-line cap)refactor: root the rule-3 array-growth probe throughwith_mut_ptr#10828 is stacked on #10824 — its first two commits are that PR. Carrying both is the dependency, not a choice.
The property being established
A read site that compares
*(receiver + 4)against its cached ShapeId and matches has then already proved the receiver is a shaped ordinary object — so theGC_TYPE_OBJECTload comes off the emitted read path. Three invariants have to hold for that. Two did not.Rule 1 — every descriptor change changes an ordinary object's ShapeId.
clear_object_descriptorsmade no shape call at all, so a cache primed while the object was frozen kept serving the frozen answer. Seven rawremove()calls on the descriptor tables lived outsidedescriptor_state.rsand skipped both the transition and the owner index, soaccessor_descriptor_keys_for_objkept reporting deleted accessors.Rule 2 — nothing but the shape allocator mints into the ShapeId range.
SYNTHETIC_CLASS_ID_BASEwas0x8000_0000— the ShapeId floor — so synthetic class id #1 is shape #1.class X extends someFunctionwrote that id into+4, whereis_shape_idaccepted it andshape_record_by_idanswered with an unrelated live shape's ordered keys. Synthetic ids move to[0xC000_0000, 0xFFFF_0000), minted through one bounded allocator, with aconst _: () = assert!(…)making the disjointness a compile error to undo.The existing pin
armed_ids_cannot_alias_a_class_idasserted this for hand-written constants only, never for the live allocator — so it could not have failed on this.Rule 3 — no pointer-tagged non-object cell holds a ShapeId-range value at
+4.new Int8Array(2**31)wrotecapacity = 0x8000_0000, the first ShapeId the process ever mints. And aPromise's+4is struct padding — neithergc_mallocnor the arena zeroes reused memory and Rust does not guarantee writing implicit padding, so a Promise born at a dead shaped object's address inherited its live ShapeId.#10824 left six kinds open and pinned them in a table; #10828 closes all six, in release, at each kind's allocation and growth funnel.
Rule3Word::RangeReachableis kept but never constructed, and the test that pinned the fence-keeping set now asserts it is empty — so the test fails the moment a new kind needs it again.Release enforcement, because a
debug_assert!is free in release#10828 is also the answer to a review comment on #10824 asking for "a release-enforced bound or representation" before considering fence removal. That was right, and it is the repo's own recurring lesson: a guard evidenced only by a debug assertion is unenforced in what ships. Five of the six kinds are counts, and a count has a bound; each is now rejected before any memory is reserved, with the error that kind already raises one byte further out.
A behaviour change is taken deliberately and recorded rather than hidden:
new Int8Array(n)/Int16Array/ … fornin[2**31, 2**32)now throwsRangeError: Array buffer allocation failedinstead of attempting a ≥2 GiB allocation. That matchesUint8Array,ArrayBufferandBuffer.alloc, which already stop ati32::MAX. The alternative is a capacity word the read path cannot distinguish from a live shape.The two gate fixes, and why neither is a ratchet bump
descriptor_state.rswas 2042 lines against the 2000 cap. Split at "what happens to a descriptor entry when its owner address changes or dies" — the prune pair,remove_descriptor_owner_entries,clear_object_descriptors,transfer_descriptor_owner,rewrite_descriptor_owner— intoobject/descriptor_state/owner_lifecycle.rs, following the existinggc_scan.rs/young.rsidiom. 264 lines moved, not the minimum 42: a 42-line shave leaves the file at exactly 2000 with zero headroom, which is a cap failure deferred by one commit. The boundary was also chosen to leave the file's trackedhandle-floorandgcheader-castsites in place, so no allowlist entry was needed.shape_rule3.rshad one raw-handle site in a module with no ceiling. Converted throughwith_mut_ptr, not ceiling'd — and a ceiling would not have worked anyway: the total was already 907 against a baseline of 906, so the total rule fires the moment the per-module rule stops. Now 906, unchanged.Attribution trailers stripped, trees asserted identical
Two source commits carried session-attribution trailers, which the house rule forbids. Stripped with a message filter that is self-tested in both directions before use (it removes the trailer; it does not eat ordinary body text), and the rewrite was then proved message-only: head tree
46525ece96before and after, byte-identical.Validation
Rebased onto v0.5.1625; seven gates green on the rebased, filtered tree —
check_file_size,raw_handle_debt,gc_runtime_root_holders,addr_class_inventory,shape_descriptor_census,native_result_ledger, andcargo fmt --all -- --check. Scanner outputs were diffed against a pre-edit baseline and are identical exceptaddr_class's file count 1548 → 1549, which is the new file.The first cycle aborted at
lintwith two real failures, both fallout from the split rather than the PRs' logic, and both fixed in this train:string_payload_access_inventory350 → 351 —shape_rule3.rs'sthrown_messagetest helper open-coded theStringHeaderpayload offset. Routed through the runtime's owncrate::string::string_data. Confirmed the +1 was this train's:origin/mainis green on that gate.gc_rekeyed_key_tablesreportedrewrite_descriptor_owneras both an unclassified site at its new path and a stale entry at the old one — the gate refusing to let a fix leave its own exemption behind. Re-pointed rather than re-classified: the moved body is byte-identical apart frompub(super), checked with a diff, so the custody argument carries over unchanged. Itswhyline reference moved with it.Neither gate is in the driver's ten cheap preflight gates;
lintruns 83. That gap is now closed by a preflight derived fromrun_lint_gates.sh --list, which was proved to discriminate — run against this train before the two fixes it reports exactly those two gates, and after it reports only the known-red public baseline.Second cycle green throughout:
lint6-of-6 withlint_unexpected_failures=[], 6 unit suites with an empty failing set, both compiler-output suites atfailed_workloads=[]with no transient excused,repsel_census rc=0 wasted_promotion=False,security_auditshowing only the tracked RUSTSEC-2026-0285 (#10791), and all five pinned artifacts byte-identical before and after the gap sweep.Gap sweep at
PERRY_RUN_TIMEOUT=30over the descriptor/shape surface these rules govern, plusclassandobjectas broad controls. 185 fixtures, every area asserted live, zero unexplained regressions:One area from the first attempt is worth recording:
freezeselected zero fixtures. The driver asserts area liveness only after the build, so that would have been a vacuous green discovered ~35 minutes in; the seconds-long preflight caught it and it was replaced withdefine, which is the surface rule 1 is actually about — #10824's seven rawremove()calls were in thedefinePropertypaths.