feat(runtime): ShapeObjectKind::Dictionary, in ShapeRecord padding that was already paid for (#10868 step 2.5 stage 1a) - #10967
proggeramlug wants to merge 1 commit into
Conversation
…ng that was already paid for (#10868 step 2.5 stage 1a) The object kind was encoded as ONE flag bit (RECORD_FLAG_KIND_CLASS), so it could represent exactly two values, and facts_key folded it as a bool. A third variant added on that footing would have read back as Ordinary from object_kind() — and since facts_match compares the full enum, that is a wrong identity match, not a hash collision. Replace flags: u8 + _pad: [u8; 3] with one flags_and_kind: u32 — the same four bytes in the same place — holding the flag byte in bits 0-7 and a 2-bit kind in bits 8-9. The record stays 32 bytes and 8-aligned, both const assertions hold unchanged, and the slab geometry is untouched: the kind lives in padding that was already allocated. facts_key now folds the kind discriminant rather than == Class, so Ordinary and Dictionary no longer share a hash contribution. Every existing consumer guards with == Ordinary or != Ordinary rather than a match, so a Dictionary shape declines all seven fast-path sites with no further edit — including ic_miss.rs is_regular and delete_rest.rs stable_candidate. That is the point: a keyless shape otherwise reads as this receiver has no own properties to a long tail of consumers, two of whose members are correct only by accident, and a set with accidental membership cannot be secured by enumerating it. Tests (single-threaded): record geometry and the O(1) probe property asserted together, so a future field cannot quietly make the kind cost memory; every kind reaches the fold distinctly; a record round-trips all three kinds beside its flags. Must-fail verified: restoring the bool fold reddens every_object_kind_reaches_the_facts_fold with Ordinary and Dictionary collide and leaves the round-trip test green.
|
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 |
|
Landed on main in merge train 256 (#11018, v0.5.1638), main Carried at head Trains rebase-merge, so commits get new SHAs and GitHub cannot mark this PR merged. Closed as landed. |
2 participants
ShapeObjectKindcould represent exactly two values: the kind round-tripped through ONE flag bit (RECORD_FLAG_KIND_CLASS) andfacts_keyfolded it as a bool. A third variant on that footing would have read back asOrdinaryfromobject_kind()— and sincefacts_matchcompares the full enum, that is a wrong identity match, not a hash collision.This replaces
flags: u8+_pad: [u8; 3]with oneflags_and_kind: u32— the same four bytes in the same place — holding the flag byte in bits 0-7 and a 2-bit kind in bits 8-9. The record stays 32 bytes and 8-aligned, both const assertions hold unchanged, and the slab geometry is untouched: the kind costs no memory, it lives in padding that was already allocated.facts_keyfolds the kind discriminant rather than== Class, soOrdinaryandDictionaryno longer share a hash contribution.Why this is worth landing on its own
Measured by lane 16b in three arms, which is the argument — a fact on the shape retiring a hand-maintained list of guards, including ones that were wrong:
Eight of nine hand-written guards became unnecessary. The deletions include the memory-unsafe out-of-bounds store at
fast_paths.rs:87(bound taken from the descriptor, keys pointer fromobject_keys_array— different arrays, so a write that lands is out of bounds). Andjs_shape_ordinary_inline_slot_for_keyturned out to be correct by accident — its conjunct passing on0 == 0— and is now correct by construction.That is the case for a kind rather than a side-table predicate: a keyless shape otherwise reads as "this receiver has no own properties" to a long tail of consumers, and a set whose membership is partly accidental cannot be secured by enumerating it.
The clean build WAS the finding
I expected the new variant to break exhaustive matches and NAME its consumers. It broke none — because every consumer guards with
== Ordinary/!= Ordinaryrather than amatch:mod.rs:1725,ic_miss.rs:1067(literallylet is_regular = shape.object_kind == Ordinary),fast_paths.rs:87,371,delete_rest.rs:484,883,native_get.rs:84. So a Dictionary shape declines all seven fast-path sites with no further edit. I verified the polarity of the two positive-sense guards rather than assuming it, because "it compiled" is not "the compiler found everything" when catch-all arms exist.Tests
Record geometry and the O(1) probe property asserted together, so a future field cannot quietly make the kind cost memory; every kind reaches the fold distinctly; a record round-trips all three kinds beside its flags.
Must-fail verified: restoring the bool fold reddens
every_object_kind_reaches_the_facts_foldwith its own message ("Ordinary and Dictionary collide — the bool fold is back") and leaves the round-trip test green — targeted, not blanket.object::shapessingle-threaded on this base: 48 passed / 0 failed.Scope
Two files, +128/−21. No latch trigger, no record re-layout, no
ObjectMetachange. Stacked onfeat/dictionary-mode(#10938), which consumes the variant. The canonical-keys half of step 2.5 (#10868) continues separately and is not entangled with this.