perf(runtime): accessor install 2.33x — memoized prototype intrinsic + single-mint fresh-install path (stacks on #9103) - #9113
Open
proggeramlug wants to merge 2 commits into
Conversation
added 2 commits
August 30, 2026 00:22
…ptor literal
esbuild emits __export(target, { name: () => binding, ... }) re-export blocks
at module top level -- always executed at startup; pi's 13MB bundle carries 44
sites totalling ~1,245 getter installs, plus CJS-interop
defineProperty(exports, "X", { enumerable: true, get: ... }) blocks. Each one
allocated a two-field descriptor object and re-decoded it by field name inside
js_object_define_property.
Codegen now recognises the descriptor literal { get: <expr>, enumerable: true }
(either property order; anon-shape New with exactly those two fields and a
literal `true`) at the Expr::ObjectDefineProperty lowering and emits a direct
js_object_define_get_accessor(obj, key, getter) call, skipping the descriptor
allocation entirely. Evaluation order is preserved (obj -> key -> getter; the
dropped `enumerable` argument is the effect-free literal `true`).
The new runtime entrypoint keeps defineProperty semantics byte-for-byte by
construction: a fast arm reproduces the generic ordinary-object accessor arm's
exact effects for the one case it admits (plain extensible GC_TYPE_OBJECT
receiver, plain non-numeric string key != "length", brand-new own property,
callable-or-undefined getter, unpolluted Object.prototype -- the same guard
try_decode_descriptor uses), and every other case materialises the two-field
descriptor and delegates to js_object_define_property, so proxies, handles,
class-refs, closures, typed arrays, buffers, frozen/sealed receivers, symbol
and numeric keys, redefinitions, and the ToPropertyDescriptor TypeErrors are
decided by exactly the code that decides them today. New-property attributes
match the generic arm: writable (internal accessor default), enumerable
(explicit), non-configurable (omitted).
Validation:
- cargo test -p perry-codegen --lib: 1349 passed (2 new IR-emission tests:
both literal orders take the fast call; enumerable:false / non-literal /
3-field / get-less shapes keep the generic call).
- cargo test -p perry-runtime --lib -- --test-threads=1: 2821 passed, 3 new
(fast-arm side-table state equals the generic arm's; numeric-key and
existing-key cases route through the generic arm, retaining configurable).
object::reserved_floor::tests::user_properties_read_back_through_the_get_path_at_scale
aborts on pristine origin/main (f3f4052) too -- pre-existing, excluded.
- test-files/test_gap_9053_export_getter_descriptor.ts: byte-identical output
vs node --experimental-strip-types (reads, keys order, descriptor
reflection, non-configurable redefine TypeError, no-change redefine,
configurable override through the fast literal, enumerable:false near-miss).
Kept IR shows 4 fast + 2 generic call sites, as designed.
- Micro-bench (500 getter installs x 2000 iters, quiet host, min/median of 6):
fast 5961/5967ms vs semantically-identical generic-path literal 6085/6089ms
(-2.0%); 28-key realistic shape ~-1.2%; node 148ms. The descriptor
alloc+decode is only ~2% of perry's install cost -- the accessor side-table
machinery (two HashMap<(usize,String)> inserts, epoch bumps, guard
invalidation per install) dominates and is the follow-up worth having.
The cjs_scaffolding Ptr<Shape> barrier collector is deliberately untouched:
exempting __export sites needs a target-provenance proof like the
exports/require whitelist, which this change does not establish -- noted as a
follow-up.
Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
…ters Ablation profile of the PerryTS#9103 fast arm (500 getter installs x 2000 iters, quiet host; PERRY_ABLATE bitmask, one term skipped per run, min of 3) put the ~6.0us/install cost at: ~2.7us object_prototype_has_desc_field admission probe -- ~98% of it the per-call %Object.prototype% RESOLUTION (globalThis builtin lookup + closure_get_dynamic_prop(ctor, "prototype")); the 6-name keys scan itself is ~0.04us ~2.9us ensure_key_in_keys_array (push + shape publish machinery; its dedupe probe is only ~0.09us -- the PerryTS#6743 sidecar already works) ~2.4us note_descriptor_target, ~100% of it transition_object_shape_semantics -- run TWICE per install (set_accessor_descriptor + set_property_attrs) ~1.9us owner_index_add x2 -- the O(N) Vec<String> dedupe scans ~0.4us the two (usize,String)-keyed map inserts ~0.07us guard-disable, ~0.03us epoch bumps, ~0 the rest 73ms/1M with every term ablated -- BELOW node's 148ms, so the floor (coerce, clone, alloc, loop) is already fine; the side tables are the gap. (Terms overlap heavily -- each also modulates GC frequency, and every GC cycle rescans the grown descriptor tables -- so they do not sum to 6.0us.) Two fixes, both equivalence-preserving: 1. object_prototype_has_desc_field now resolves %Object.prototype% through the memoized, root-scanned prototype-addr cache (one slot load + a forwarding heal, healed by scan_prototype_addr_cache_roots_mut) instead of re-walking globalThis + the ctor's dynamic-prop table per call. The cache IS the realm intrinsic ToPropertyDescriptor reads inherited fields through, so a rebound globalThis.Object no longer perturbs the probe; the keys scan is kept per call, so no new invalidation machinery exists. 2. install_fresh_accessor_property: a one-call install for a PROVEN-brand-new accessor property, used by the fast arm's tail in place of set_accessor_descriptor + set_property_attrs. Folds: one epoch bump, one note_descriptor_target (one semantic shape mint instead of two -- nothing can observe the intermediate generation), one idempotent guard-disable, one meta access setting both kind bits and returning their prior state -- and when a kind's bit was CLEAR, the meta summary's own contract ("a clear bit proves the tables hold no entry for that key"; every owner_index_add site sets the matching bit first, bits are sticky, removals only shrink) proves the owner index cannot hold the key, so the O(N) dedupe scan becomes a plain push. Set bits and non-meta-capable owners keep the scanning add; a violated precondition degrades to overwrite, never corruption (regression-tested). Measured (same quiet host, min/median of 5, 1M installs; node 148ms): 500-key targets: 5972 -> 2563ms (2.33x; ~40x node -> ~17x) 28-key targets: 3989 -> 1426ms (2.80x) The 2-field generic-literal path is ~unchanged (6089 -> 6015ms): its cost is dominated by the descriptor build/decode plus the duplicated install stack it still runs, and the ablation terms are GC-coupled rather than additive. Validation: cargo test -p perry-codegen --lib 1349 passed; cargo test -p perry-runtime --lib -- --test-threads=1 2823 passed (2 new: combined installer state == two-call sequence incl. owner index; repeated combined install dedupes via the prior-bit path). reserved_floor's at-scale test SIGABRTs identically on pristine origin/main db6df04 -- pre-existing, excluded. test_gap_9053 fixture stays byte-identical to node. Remaining (analysis only, not implemented): ensure_key_in_keys_array's ~2.8us -- js_array_push's per-call porch (proxy/subclass probes + clean_arr_ptr allocator resolution) plus set_object_keys_array's mint-then-stamp shape publish per append; a keys-append entry that reuses ensure's already-validated header and publishes once per batch would be the next term, but it walks straight into the PerryTS#8113 mint-then-stamp and PerryTS#9029 lineage-publish contracts, so it deserves its own change. At giant-table scale the per-GC-cycle scan_descriptor_roots_mut walk over the descriptor tables is what couples the terms; at pi's ~1.2k installs it is irrelevant. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthrough
ChangesAccessor descriptor fast path
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR speeds up eligible fresh accessor-property installs while retaining generic handling for unsupported or conflicting cases. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant __export
participant Codegen
participant js_object_define_get_accessor
participant DescriptorState
participant GenericDefineProperty
__export->>Codegen: lower { get, enumerable: true }
Codegen->>js_object_define_get_accessor: pass obj, key, getter
js_object_define_get_accessor->>DescriptorState: validate and install fresh accessor
js_object_define_get_accessor->>GenericDefineProperty: delegate rejected cases
__export->>DescriptorState: read live getter and descriptor state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the optimization, performance results, related PR, implementation details, and validation. However, it does not use the required template sections and does not provide the required checklist confirmations. The Summary, Changes, Related issue, Test plan, Screenshots / output, and Checklist sections are missing or incomplete. Resolution Rewrite the description using the repository template. Add explicit Summary, Changes, Related issue, and Test plan sections. Include the requested test commands and mark applicable checklist items. Add the required version, documentation, platform, and contribution checklist confirmations, or state why they do not apply.
✨ Finishing Touches🧪 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
1 participant
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.
Second half of the __export/defineProperty startup lane (stacks on #9103 — its commit is included here since it wasn't merged yet; rebases clean if it lands first).
The measured split first (temporary env-bitmask ablation gates, quiet host, 500×2000 installs, ~6.0µs/install baseline; terms GC-coupled so they don't sum linearly):
Fixes:
object_prototype_has_desc_fieldresolves the intrinsic through the existing memoized, root-scannedobject_prototype_addr()(one slot load + forwarding heal) — no new invalidation machinery, and MORE correct: it's the realm intrinsic ToPropertyDescriptor actually reads through, immune toglobalThis.Objectrebinding.install_fresh_accessor_propertyused by perf(runtime,codegen): direct install for __export's get-only descriptors (groundwork; honest numbers inside) #9103's fast arm: folds one epoch bump, ONE shape mint (was two), one guard-disable, one meta access, and skips the O(N) owner-index dedupe when the meta kind-bit is clear (the summary's own documented invariant: a clear bit proves the tables hold no entry; bits are sticky; violated precondition degrades to overwrite, regression-tested).Numbers (quiet host, 1M installs; node 148ms): 500-key 5972 → 2563ms (2.33×); realistic 28-key __export shape 3989 → 1426ms (2.80×). Honest caveat: the generic descriptor-literal path barely moves (its cost is the duplicated install stack it still runs) — folding it needs the redefine cases and is left out deliberately. Next term is ensure_key_in_keys_array's append porch (~2.9µs) but that walks into the #8113 mint-then-stamp and #9029 lineage contracts and deserves its own change (analysis included in the lane notes).
Validation: codegen 1349/0; runtime 2823/0 (--test-threads=1; the reserved_floor at-scale exclusion is #9108, pre-existing on the base and fixed separately in #9110);
test_gap_9053_export_getter_descriptor.tsbyte-identical to node; fmt/census/file-size green. Implemented by a subagent in an isolated worktree; reviewed and shipped by the coordinating session.Summary by CodeRabbit
Performance
Bug Fixes
Tests