batch: land #8872, #8875, #8877 - #8878
Merged
Merged
Conversation
…ry entry
Three hot paths answered "what does THIS owner have?" by walking every
descriptor in the process and filtering on the owner address:
* js_object_keys' array branch, twice (enumeration.rs) — a full
property_descriptors walk per enumeration, just to decide whether a
per-index enumerable check was needed;
* accessor_descriptor_keys_for_obj, on the own-keys path;
* transfer_descriptor_owner, on every ArrayHeader growth;
* scan_descriptor_roots_mut, on EVERY GC cycle — so since the moving
young-gen scavenge became default (#7019) this was a per-collection
tax proportional to the whole program's descriptor count rather than
to what actually moved.
Profiling `claude -p` put 46.6% of main-thread samples in
shapes/descriptors, with a HashMap Keys iteration the single hottest
self-time entry by 4x over anything else.
DescriptorTables now carries attr_keys_by_owner / accessor_keys_by_owner
mirroring the two (owner, key) maps, so each of those becomes a lookup.
The maps stay authoritative; the index is a mirror, and the tests assert
that invariant directly (index == what a full scan would return) across
install, redefine, delete, bulk-clear and owner transfer, because the
failure mode of a mirror is silent drift, not a crash.
Also fixes a pre-existing correctness bug the new tests caught:
transfer_descriptor_owner moved descriptors to the new address but never
carried the per-object Bloom summary. A freshly grown array has a null
meta, for which owner_may_have_descriptor_entries answers false
AUTHORITATIVELY — so after an array grew, Object.keys and
getOwnPropertyDescriptor silently lost every accessor it had. That was
equally true before this change: the gate sat in front of the old scan,
so the scan never ran for the new owner.
The `lint` job failed on four gates that the PR's own changes tripped: - changelog: add the `changelog.d/8872-*` fragment for the crates/ changes. - file size: `array/indexing.rs` reached 2,024 lines after the resolved-store work; move the transactional `js_array_numeric_range_add*` kernel (a block with no raw-handle or address-classification debt, so no per-module ratchet ceiling moves) into `array/numeric_range.rs`. - local-binding-type audit: classify the synthetic `arguments.length` marker read in `property_get.rs::lower` (runtime-validated: the marker type exists only in direct-call-only clones whose caller materialized the count). - GC store-site inventory: register `store_array_slot_resolved` as a chain-verified discharge helper for the three BARRIERED markers that now lean on it, mark its own resolved slot write, and pin the second `apush` codegen marker (the unconditional element store inside `emit_dynamic_pointer_push_store`, barriered by the same stem) with the self-test tree updated to match. Every step of the lint job was replayed locally, including the raw-handle and unrooted-local ratchets against the merge base d354443. Claude-Session: https://claude.ai/code/session_01FUvFrRNZyc5qknBiJbYbby
This was referenced Aug 27, 2026
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (69)
📝 WalkthroughWalkthroughThis PR adds cross-module function inlining, scalar ChangesCompiler and code generation
Array runtime
Descriptor and native export behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: ✨ 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 |
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Aug 27, 2026
…-mutation Second merge round after the PerryTS#8872/PerryTS#8875/PerryTS#8877 batch landed. Conflicts and their resolutions: - codegen/method.rs: keep this branch's guarded-falsy/index/pshape-arg clone handling and add main's `!arguments_length_clone` exclusions. - expr/property_get.rs: keep both the Symbol-then-named-field IC dispatch (ours) and main's synthetic `arguments.length` fast path. - property_get/generic_dispatch.rs: main's native Map/Set `size` split ahead of the object PIC, with this branch's `is_object_kind` naming. - lower_call/method_override.rs: `direct_call_fn` (main, argument-length clone) is consulted first, then the pshape+index clone (ours); the two are mutually exclusive by construction. - array/element_shape.rs: adopt main's demand-driven proofs (no eager `establish` on the first store) inside this branch's `note_element_store_with_bit` / `_resolved_flags` split; the now-unused `element_identity_of_bits` goes with it, and the renamed `pushes_do_not_create_an_unrequested_element_shape_proof` test replaces the eager-establishment one. - array/header_gc_slots.rs + mod.rs: keep both resolved-head store helpers (`note_array_slot_resolved_flags` ours, `store_array_slot_resolved` main). - array/push_pop.rs: `js_array_push_f64_resolved` now stores through main's `store_array_slot_resolved`. - array/indexing.rs: the strict setter keeps this branch's dense fast path first, then main's resolved-head strict path; main moved the numeric-range helpers into `array/numeric_range.rs` (byte-identical bodies), so the in-file copies and their keepalive anchors are dropped; main's fused strict store in `js_array_set_index_or_string_strict` is ported into `indexing_keyed.rs`. - expr/index_get_claim_tests.rs: union of imports/constants and both test sets (main's canonical-i32 split tier and this branch's `Any`-key tier are complementary arms). - lower_call/property_get/dynamic_dispatch.rs grew past the 2,000-line gate; the tower-of-pshape routing moved to `dynamic_dispatch_tower.rs`. Verified locally: fmt; perry-codegen and perry-runtime lib + test targets build warning-free; both suites green; file-size, GC store-site, addr-class, raw-handle, shape-descriptor census, binding and architecture audits pass. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Aug 27, 2026
…ses densely After merging main (PerryTS#8878 / PerryTS#8872's canonical-i32 read split), a declared-array receiver with a non-static key takes the guarded plain-array tier first. On an object-backed `class X extends Array` receiver (wolf-ecs `Archetype`, `packed[sparse[x]]` in SparseSet.has/remove) that guard always misses and `js_typed_feedback_array_index_get_fallback_boxed`'s GC_TYPE_OBJECT arm stringified every index into a by-name lookup (from_utf8 + string alloc + reflection ladder per read): both wolf-ecs benchmarks regressed ~2.2x. The fallback now asks `array_subclass_fast_index_get` for a canonical (plain or INT32-boxed) non-negative index before its registry probes and the by-name path; receivers without a dense proof keep the established route. Mac mini 11-pair screen vs the pre-merge build: add/remove +0.5%, entity-cycle -1.2% (from +126% / +121%); semantics probe byte-identical to Node. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
5 tasks
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.
Batch landing of three reviewed PRs, validated once as a single merged tree. No fixes needed — all three gate-clean as submitted.
Audit of #8875
These tables are process-global maps keyed by
(owner_address, key), so an added secondary index in the same key domain is worth checking carefully.The new indexes stay consistent with the primary maps at every mutation point. Traced all of them: add (
:771,:992,:1051-1052,:1084), remove (:790,:1011), object destruction (:1210-1212), and owner transfer (:1259-1260).transfer_descriptor_ownermoves both primary maps and callsowner_index_transferon both new indexes. No desync path found.It also fixes a pre-existing correctness bug. Its own comment records it: a freshly grown array has a null
meta, soowner_may_have_descriptor_entriesanswered false authoritatively, andObject.keys/getOwnPropertyDescriptorsilently lost every accessor the array had before growth. That gate sat in front of the old full-table scan too, so the scan never ran for the new owner either. Carrying the per-object Bloom summary across the transfer is a real fix, not just an indexing change.One question flagged, not blocking.
transfer_descriptor_owneris called from array growth (array/push_pop.rs:199), not from any GC move path — so owner-address keying relies on descriptor-owning objects not being relocated. That is pre-existing: #8875 adds an index in the same key domain rather than introducing the exposure. But it is the #8393 shape (a side table keyed by a raw heap address going stale after a copying minor), and I could not locate the mechanism that guarantees it. Worth confirming whether such objects are pinned; if they are not, both the old and new tables share the exposure.Validation (merged tree)
perry-runtime2716,perry-codegen1283,perry-stdlib122,perry-hir340 — all 0 failedPERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1): 16 failed on the batch and 16 on cleanmain— same-commit A/B, none introduced. Run because perf(descriptors): index descriptors by owner instead of scanning every entry #8875 touches owner-keyed tables and perf: remove cross-module ECS dispatch and argument-bundle overhead #8872 changes array-header reuse across indexed stores.dfchecked before and after; no result produced under ENOSPCSummary by CodeRabbit
Performance
Object.keysandfor…inperformance, especially when unrelated objects have descriptors.Bug Fixes
for…ofdestructuring.fs.readFilewith expected.prototypebehavior.