Skip to content

shape_index_migrate_after_delete steals a shared keys array's index, stranding sibling objects #9006

Description

@proggeramlug

shape_index_migrate_after_delete (added in #9002) carries a keys array's slot index onto the private clone that js_object_delete_field makes, by moving it:

let Some(mut index) = inner.indices.remove(&old_keys_id) else { return };
...
inner.indices.insert(new_keys_id, index);

The source array on that path is expected to be shared. The clone exists precisely because it is — delete_rest.rs:335:

CRITICAL: clone the keys_array before mutating it. The same keys_array is shared across all objects that built the same shape via transition_cache_lookup-hit fast paths.

So a delete from any one object strands every sibling that still shares the source array: their accelerator disappears and each rebuilds it (decode + FNV-hash every key name) on its next lookup.

The neighbouring shape_keys_grown documents exactly this rule for itself:

Callers must pass the OWNED-grow pair only: a shared array's fork is a genuine transition (the clone starts a NEW identity and the old address still describes the siblings' live shape — migrating it would corrupt them).

Not a correctness bug

Verified in the code, not just from the doc: shape_slot_lookup re-validates the stored key bytes against the requested bytes (stored == key_bytes) before returning a slot, and drops the index outright when indexed_len > key_count. A stranded or stale index can only produce a MISS and the caller's own fallback — never a wrong property. This is purely a throughput hazard.

Why #9002's measurement can't see it

bench_populated_delete.ts deletes from a single object, so there are no siblings to strand. The regression needs N objects sharing one shape where one of them deletes — at which point the other N−1 pay a full rebuild per delete, which is the very cost #9002 removes for the deleter.

Suggested fix

Branch on the source's GC_FLAG_SHAPE_SHARED bit (idiom already in field_set_by_name/tail.rs:950): move the index when the source is owned and about to die, clone it when the source is shared so siblings keep theirs. SlotList already derives Clone; ShapeIndex would need it. Cloning the table is one allocation plus a memcpy — far below the rebuild it avoids — but it is a real cost on the deleting path, so it wants its own A/B rather than being folded into #9002's numbers after the fact.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions