feat(m4-12): boa→VM PR3.6 — Precomputed Event Shapes#75
Merged
Conversation
…Strings Adds ~30 StringId fields to WellKnownStrings covering every EventPayload variant's JS-visible property name (clientX, key, repeat, propertyName, relatedTarget, deltaMode, etc.). Pre-interning at Vm::new means the upcoming PrecomputedEventShapes builder (C2) can walk shape-add transitions without re-interning, and create_event_object (C4) can feed slot values directly into define_with_precomputed_shape without the per-dispatch strings.intern(name) hash/lookup that install_named currently incurs. CloseEvent's numeric code reuses the JS name \"code\" but is stored as close_event_code in the struct to avoid shadowing the Keyboard code field (the underlying StringId is equal via StringPool canonicalisation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ent_shapes Introduces vm/host/event_shapes.rs defining: - PrecomputedEventShapes — terminal ShapeId per EventPayload variant (mouse/keyboard/transition/animation/input/clipboard/composition/ focus/wheel/message/close_event/hash_change/page_transition/storage) plus a shared `core` shape for the 9 properties every event object carries (type, bubbles, cancelable, eventPhase, target, currentTarget, timeStamp, composed, isTrusted). Scroll and None payloads terminate at `core` directly; unknown non-exhaustive variants also fall through to `core`. - VmInner::build_precomputed_event_shapes — walks shape_add_transition once at VM creation for each variant. All variants share the core-9 prefix via the transition cache's built-in deduplication, so the total shape cost is ~30 Shape entries added to `VmInner.shapes` — a one-time allocation paid at Vm::new time to avoid ~17 transition hashmap probes and ~8 strings.intern calls per dispatched event. - PrecomputedEventShapes::shape_for — runtime lookup table used by create_event_object (C4). Match arms mirror events::set_payload_properties 1-to-1; keeping them in sync is a structural invariant because each variant's slot positions are determined by the shape's ordered_entries. Wired into register_globals after register_event_methods_prototype (engine feature only). Cached in VmInner.precomputed_event_shapes via Option<PrecomputedEventShapes>. Marked #[allow(dead_code)] on the struct and shape_for method — the consumer lands in C4 (create_event_object rewrite); the allow is removed there. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a fast-path property-install API that accepts a pre-built ShapeId plus the matching slot values, skipping the per-property transition walk that `define_shaped_property` performs. Contract: - The caller must supply slot values in the exact order the shape was built with. A debug_assert compares slot count against the shape's property_count to catch structural drift (out-of-sync callers would otherwise write values into the wrong JS-visible property names). - All slots are installed as PropertyValue::Data — accessor properties must use define_shaped_property. This matches the event-object usage pattern where own properties are all data and accessors (e.g. `defaultPrevented`) live on the shared event_methods_prototype. - Dictionary-mode objects panic — the API is meant for freshly-allocated objects that have never left Shaped storage. The event-object builder in C4 always allocates fresh at ROOT_SHAPE. Marked #[allow(dead_code)] until C4 consumes it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrites the per-dispatch event-object builder to take the fast path
introduced in C2/C3:
1. Allocate at ROOT_SHAPE, empty slot vec.
2. Assemble `Vec<JsValue>` of length 9 + N in shape order (core 9 +
payload-specific values).
3. Publish via a single `define_with_precomputed_shape` call, picking
the terminal ShapeId from `PrecomputedEventShapes::shape_for`.
Replaces the previous 9 + N `define_shaped_property` calls (each
walking the shape-add transition cache hashmap + cloning
property_map/ordered_entries) with one storage replacement, plus
eliminates the per-payload-property `strings.intern(name)` calls —
payload keys are pre-interned into WellKnownStrings (C1).
Property-name order in each payload arm of `append_payload_slots`
must match the transition order in `build_precomputed_event_shapes` —
the debug_assert in `define_with_precomputed_shape` catches count
drift, but a reordering without a matching `shape_for` update would
silently write values into wrong JS-visible property names. Comments
flag this structural invariant on both sides.
Also drops `install_*` helpers and `EventKeys`/`well_known_event_keys`
snapshot struct — all subsumed by the single slot-assembly pass.
Tests:
- Existing 7 tests in tests_create_event_object.rs remain green
unchanged (core_properties_installed_and_typed,
default_prevented_is_accessor_and_reflects_flag_live,
four_methods_installed_on_event_methods_prototype,
mouse_payload_installs_coords_and_modifier_keys,
keyboard_payload_installs_key_code_and_repeat,
focus_payload_resolves_related_target_to_wrapper,
event_object_kind_carries_flag_seed).
- Adds 3 new identity tests:
* two_mouse_events_share_one_shape — hidden-class fast path
verification (same variant → same ShapeId)
* different_payload_variants_use_different_shapes — distinct
variants do not collide into a single shape
* scroll_and_none_share_core_shape — verifies the documented
share of the `core` terminal shape for no-payload variants
Also removes the `#[allow(dead_code)]` markers from
PrecomputedEventShapes / define_with_precomputed_shape — consumers
are live now. Engine-feature-gates define_with_precomputed_shape
itself since its sole caller is engine-only.
Full `cargo test -p elidex-js --features engine --lib`: 1767 passed
(1764 + 3 new).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a per-dispatch create_event_object throughput bench covering the
three dominant payload variants:
- event_dispatch/mouse — MouseEventInit (8 payload keys)
- event_dispatch/keyboard — KeyboardEventInit (7 keys, string key/code)
- event_dispatch/none — EventPayload::None (core-9 only)
Required changes to make the bench reachable from outside the crate:
- Cargo.toml: adds `criterion` as a dev-dep and registers
`event_dispatch` as a `[[bench]]` with `required-features = ["engine"]`
(DispatchEvent / HostData / create_element_wrapper are engine-gated).
- vm/vm_api.rs: promotes `Vm::create_element_wrapper` and
`Vm::create_event_object` to pub #[cfg(feature = "engine")]. These
thin wrappers over the VmInner internals are what shell dispatch
code (PR6) will call anyway — exposing them here unblocks external
benches without requiring a separate doc-hidden hook.
Not wired into `mise run bench` (that task is for the default
elidex-css/elidex-style/elidex-layout benches which build without
feature flags). Run manually with:
cargo bench -p elidex-js --features engine --bench event_dispatch
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses findings from 3-agent review (reuse / quality / efficiency).
## Fix 1 — eliminate double Vec allocation (efficiency, high impact)
`define_with_precomputed_shape` previously took `slots: &[JsValue]`
and did `.iter().copied().map(PropertyValue::Data).collect()`
internally, producing a second Vec per dispatch on top of the
caller's `Vec<JsValue>`. Changed signature to
`slots: Vec<PropertyValue>` (owned) — the caller's vector is now
moved directly into the object's slot storage.
`create_event_object` + `append_payload_slots` now build
`Vec<PropertyValue>` directly. A quartet of local push helpers
(num/b/s/v) keeps per-variant arms readable without the
`PropertyValue::Data(JsValue::X(...))` triple-wrap.
Bench delta (criterion quick vs pre-simplify):
- Mouse 84 ns → 55 ns (additional 35% faster; vs main: 11.5×)
- Keyboard 102 ns → 75 ns (additional 27% faster; vs main: 7.8×)
- None 68 ns → 53 ns (additional 22% faster; vs main: 6.4×)
## Fix 2 — drop `close_event_code` WellKnownStrings field
CloseEvent's numeric `code` shares the JS-visible name `"code"` with
KeyboardEvent's `code`. StringPool::intern is canonical so both
resolved to the same StringId — the split Rust field was pure alias
noise. `event_shapes.rs` now uses `well_known.code` for both the
Keyboard and CloseEvent shape chains, with an explanatory comment on
CloseEvent's use site.
## Fix 4 — `#[doc(hidden)]` on bench-only pub APIs
`Vm::create_element_wrapper` / `Vm::create_event_object` return
unrooted `ObjectId`s and cannot be safely used externally without
`push_temp_root` (pub(crate)). The original doc comments claiming
"shell integration (PR6)" were speculative and misleading — PR6
hasn't landed and the API needs more rooting machinery before shell
can use it. Added `#[doc(hidden)]` and rewrote the doc to honestly
mark both as bench-only hooks.
## Fix 6 — drop stale "PR3.6" labels
Removed PR-reference tags from in-code comments (they go stale
post-merge) while preserving the substantive "why" content:
- `mod.rs` struct-field comment on `precomputed_event_shapes`
- `mod.rs` WellKnownStrings payload-key section header
- `mod.rs` `Vm::new` intern block comment
- `globals.rs` init-order comment
- `events.rs::create_event_object` doc — collapsed a 20-line
"PR3.6 fast path" section to a 4-line pointer at the
`host/event_shapes.rs` module doc
- `tests_create_event_object.rs` section header
## Fix 9 — collapse `Scroll | None` + `_` arms in `shape_for`
`EventPayload` is `#[non_exhaustive]`, so the wildcard arm is
required and subsumes `Scroll | None` (both install no payload
props). Original code separated them with
`#[allow(clippy::match_same_arms)]` for "documentation," but the
explanation fits better as a comment on the merged arm. Removes
the allow suppression.
## Verification
- `cargo test -p elidex-js --features engine --lib`: **1767 pass**
- `cargo test -p elidex-js --lib` (non-engine): 1689 pass
- `cargo clippy -p elidex-js --features engine --all-targets`: 37
errors (all pre-existing on main, same count)
- `mise run ci`: all pass
- Bench above
## Skipped findings
- Unifying the 16-arm variant lists in `event_shapes.rs` and
`events.rs` into a single source of truth (lockstep invariant
reformer) — invasive abstraction work deferred; debug_assert on
slot count already catches count drift, ordering drift would need
a bigger refactor than fits this PR.
- Making `precomputed_event_shapes` non-Option — init ordering makes
the workaround awkward; Option is a 1-byte discriminant on the
hot path and `expect()` gives early detection.
- Empty-string fast path (skip `strings.intern("")`) — speculative
perf; `StringPool` already caches the empty string via
`well_known.empty` so only saves a hash on the first call.
- Extracting `bind_vm` / `make_event` into a shared helper across
tests and benches — useful, but wider than this PR's scope; pattern
repeats across many test files.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR3 already raised `vm/mod.rs` from 938 → 1117 lines; PR3.6 was about to push it to 1274. Extracts two self-contained units to new files, bringing mod.rs back under the project's 1000-line convention (see `vm/value.rs:10`, `compiler/expr_yield_star.rs:3`, etc. for the cited convention): - `vm/well_known.rs` (289 lines) — `WellKnownStrings` struct + 80 payload/dispatch/built-in property name interns, plus `WellKnownSymbols` struct + 7 well-known symbol allocations. New `WellKnownStrings::intern_all(&mut StringPool) -> Self` and `WellKnownSymbols::alloc_all(&mut StringPool) -> (Self, Vec<SymbolRecord>)` constructors collapse the ~130-line literal init block in `Vm::new` into two calls. - `vm/temp_root.rs` (108 lines) — `VmTempRoot` RAII guard struct + `push_temp_root` constructor method + Deref/DerefMut/Drop impls. Engine-feature-gated at module level (the only caller path is host-side event-object construction). Re-exported from `vm/mod.rs` as `pub(crate) use temp_root::VmTempRoot` so `vm_api.rs`'s `super::VmTempRoot` reference resolves unchanged. ## File sizes after split | File | Before | After | |-------------------|-------:|------:| | vm/mod.rs | 1274 | 918 | | vm/well_known.rs | — | 289 | | vm/temp_root.rs | — | 108 | PR3.6 modified files all under 1000 now. (`vm/value.rs` at 1015 is pre-existing and untouched by this PR.) `Vm::new` also drops its `#[allow(clippy::too_many_lines)]` — the extraction shrank the function from ~240 to ~80 lines. `WellKnownStrings::intern_all` carries the allow instead (101 lines of field init is the shape of the data). ## Verification - `cargo test -p elidex-js --features engine --lib`: 1767 pass - `cargo test -p elidex-js --lib` (non-engine): 1689 pass - `cargo clippy --features engine --all-targets -- -D warnings`: 37 errors (all pre-existing on main, same count as before split) - `mise run ci`: all pass - Bench unchanged: Mouse 56 ns / Keyboard 75 ns / None 53 ns (±1% noise from pre-split run) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Performance-focused VM change that precomputes and caches terminal Shapes for each EventPayload variant, so create_event_object can publish all event properties in one operation (reducing per-dispatch hidden-class transition work).
Changes:
- Add
PrecomputedEventShapesbuilt duringregister_globalsand use it increate_event_objectto allocate/publish with a precomputed terminalShapeId. - Introduce
VmInner::define_with_precomputed_shapeto install a terminal shape + slot vec in a single step (engine-only fast path). - Split
WellKnownStrings/WellKnownSymbolsandVmTempRootinto dedicated modules; add shape-identity tests and a Criterion benchmark.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/script/elidex-js/src/vm/well_known.rs | New module for pre-interned WellKnownStrings (incl. event payload keys) and WellKnownSymbols. |
| crates/script/elidex-js/src/vm/vm_api.rs | Adds #[doc(hidden)] bench hooks for create_element_wrapper and create_event_object (engine-only). |
| crates/script/elidex-js/src/vm/tests/tests_create_event_object.rs | Adds tests asserting payload-variant shape sharing / separation. |
| crates/script/elidex-js/src/vm/temp_root.rs | Extracts VmTempRoot guard into its own module (engine-only). |
| crates/script/elidex-js/src/vm/mod.rs | Wires new modules, adds precomputed_event_shapes, and introduces define_with_precomputed_shape. |
| crates/script/elidex-js/src/vm/host/mod.rs | Registers new event_shapes submodule. |
| crates/script/elidex-js/src/vm/host/events.rs | Reworks create_event_object to assemble slots + publish via precomputed shapes; adds append_payload_slots. |
| crates/script/elidex-js/src/vm/host/event_shapes.rs | New engine-only shape table + builder for per-EventPayload terminal shapes. |
| crates/script/elidex-js/src/vm/globals.rs | Builds/stores precomputed_event_shapes during global registration (engine-only). |
| crates/script/elidex-js/benches/event_dispatch.rs | Adds Criterion benchmark for event-object construction throughput (engine-only). |
| crates/script/elidex-js/Cargo.toml | Adds Criterion dev-dependency and bench target gated by engine. |
| Cargo.lock | Adds Criterion to the workspace lockfile as a dependency of elidex-js. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review round 1 (2 findings, both in host/event_shapes.rs): - Module doc (L28) referenced `events::set_payload_properties` which was renamed to `events::append_payload_slots` during the C4 rewrite. - `build_precomputed_event_shapes` doc (L128) had the same stale reference plus a mention of `payload_slots()` that was never the actual function name. Both updated to point at `events::append_payload_slots` — the invariant message itself (keep shape-key order in lockstep with slot assembly) is unchanged. Doc-only; no behaviour change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Summary
Single-purpose perf PR (same model as M4-11 PR4) inserted between PR3 (Event Object + Listener, #73) and PR4 (Document / Window / DOM method suite).
Collapses `create_event_object`'s per-dispatch cost by:
Bench (criterion quick, vs `main`):
60fps × 5 listener 想定で event 構築コスト `3.17 μs → 275 ns` / frame。
Commits (7)
Design decisions
Deferred (trigger conditions recorded in memory)
New public API (engine feature, `#[doc(hidden)]` bench-only)
Both are bench hooks; returned `ObjectId` is unrooted and can't be safely persisted externally (rooting machinery `push_temp_root` is `pub(crate)`). Shell integration (PR6) will need broader API redesign.
Test plan
🤖 Generated with Claude Code