perf(array): answer "is this receiver a Map/Set?" from the object's own GC tag (#7765) - #7765
Open
proggeramlug wants to merge 5 commits into
Open
perf(array): answer "is this receiver a Map/Set?" from the object's own GC tag (#7765)#7765proggeramlug wants to merge 5 commits into
proggeramlug wants to merge 5 commits into
Conversation
added 2 commits
August 10, 2026 14:07
…C tag (#7768) `js_array_get_f64` and `js_array_length` asked both collection registries whether an ordinary array was secretly a Set or a Map on every element read. On `gc-handoff/apps/asyncpipe.ts` those two probes were 13.5% of the run: the program uses Map and Set, so the #7474 latch is armed and each probe really resolved a thread-local and hashed. Every registered Map/Set IS its `arena_alloc_gc(_, _, GC_TYPE_MAP|GC_TYPE_SET)` header, so the object's own `obj_type` answers in one byte. ABA-proof by construction — the tag lives inside the candidate bytes, so recycling the address rewrites it — which is what an address-keyed negative memo could not offer (#7755). The registry stays authoritative for the positive answer. The same header read now also supplies the descriptor flags that `array_object_flags` re-derived through a second `clean_arr_ptr`.
…7768) The object field-get funnel already proves `keys` is a live `GC_TYPE_ARRAY` and caps the index below its capacity, then called `js_array_get` per key — which re-establishes both facts from scratch: a `clean_arr_ptr` forwarding walk, a lazy-header probe, the exotic-receiver classifications and a descriptor-flag read. That funnel was 78% of all `js_array_get_f64` samples on `gc-handoff/apps/asyncpipe_big.ts`. `keys_array_len_capped_to_capacity` paid the same toll through `js_array_length` once per property read. `keys_array_slot` serves the dense, descriptor-free, non-forwarded case from the array's own words and delegates everything else — a hole, an out-of-range index, a forwarded or descriptor-carrying array, a null pointer — so no general semantics move. A test-only per-thread fallback counter pins both directions, so a fast path that stopped applying and one that started swallowing a shape it should have delegated are equally red. Also switches the #7768 receiver-tag read to `addr_class::try_read_gc_header`: this file's usual `>= GC_HEADER_SIZE + 0x1000` floor sits BELOW the handle band, and `js_array_length` reaches it before proxy/handle receivers are routed. Keeps the addr-class ratchet green too.
📝 WalkthroughWalkthroughArray access now reads GC receiver tags once to gate Map and Set registry probes. Dense key arrays use direct slot reads with fallback handling. Tests cover registry suppression, recycled memory, collection tags, allocation growth, and key-array fallback behavior. ChangesArray receiver gating
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArrayAccess
participant ReceiverTag
participant MapRegistry
participant SetRegistry
ArrayAccess->>ReceiverTag: Read receiver GC type
alt Map receiver
ArrayAccess->>MapRegistry: Validate Map registration
else Set receiver
ArrayAccess->>SetRegistry: Validate Set registration
else Ordinary array
ArrayAccess-->>ArrayAccess: Skip collection probes
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/array/collection_tag_tests.rs`:
- Around line 182-191: Replace the unsafe recycling block in the collection tag
test with a genuine ArrayHeader allocation sized for two f64 elements, rather
than reinterpreting the MapHeader allocation at addr. Add a test-only helper to
place the new array address into the registry’s stale-entry state, and update
the later cleanup around the existing registry test flow so the MapHeader
entries pointer remains untouched and both allocations are safely released.
- Around line 47-60: The collection tests currently retain raw MapHeader and
SetHeader pointers across allocating calls, so they can become stale after GC
relocation. In arm_both_registries, root map and set with RuntimeHandleScope
before allocating the other collection, then reload their pointers before
registry checks and return/use. In
crates/perry-runtime/src/array/collection_tag_tests.rs:287-300, store rooted
handles rather than raw pointers while the allocation loop runs, and reload
pointers during each verification loop; apply the same change at both sites.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38cf13e6-22f6-416f-868b-8748fbc4e66a
📒 Files selected for processing (11)
changelog.d/7768-array-receiver-tag-gates-collection-probes.mdcrates/perry-runtime/src/array/collection_tag_tests.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/object/field_get_set/field_ops.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/set.rsscripts/addr_class_allowlist.txt
Comment on lines
+47
to
+60
| fn arm_both_registries() -> (*mut MapHeader, *mut SetHeader) { | ||
| let map = js_map_alloc(4); | ||
| js_map_set(map, 1.0, 10.0); | ||
| let set = js_set_alloc(4); | ||
| js_set_add(set, 5.0); | ||
| assert!( | ||
| crate::map::is_registered_map(map as usize), | ||
| "the map registry must be armed for these tests to mean anything" | ||
| ); | ||
| assert!( | ||
| crate::set::is_registered_set(set as usize), | ||
| "the set registry must be armed for these tests to mean anything" | ||
| ); | ||
| (map, set) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Root collection pointers across allocations.
js_set_alloc, js_map_alloc, and later collection allocations can evacuate existing GC objects. Raw MapHeader and SetHeader pointers do not update after relocation.
crates/perry-runtime/src/array/collection_tag_tests.rs#L47-L60: Keepmapandsetin aRuntimeHandleScopebefore allocating the other collection. Reload their pointers before use.crates/perry-runtime/src/array/collection_tag_tests.rs#L287-L300: Store rooted handles, not raw pointers, while the loop continues to allocate collections. Reload pointers during the verification loops.
Based on learnings, raw Rust pointer locals are neither GC roots nor reliable pins across allocating operations.
📍 Affects 1 file
crates/perry-runtime/src/array/collection_tag_tests.rs#L47-L60(this comment)crates/perry-runtime/src/array/collection_tag_tests.rs#L287-L300
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/array/collection_tag_tests.rs` around lines 47 - 60,
The collection tests currently retain raw MapHeader and SetHeader pointers
across allocating calls, so they can become stale after GC relocation. In
arm_both_registries, root map and set with RuntimeHandleScope before allocating
the other collection, then reload their pointers before registry checks and
return/use. In crates/perry-runtime/src/array/collection_tag_tests.rs:287-300,
store rooted handles rather than raw pointers while the allocation loop runs,
and reload pointers during each verification loop; apply the same change at both
sites.
Source: Learnings
Comment on lines
+182
to
+191
| // Recycle the bytes into a two-element dense array, registry untouched. | ||
| unsafe { | ||
| set_gc_obj_type(addr, crate::gc::GC_TYPE_ARRAY); | ||
| let recycled = addr as *mut ArrayHeader; | ||
| (*recycled).length = 2; | ||
| (*recycled).capacity = 2; | ||
| let elements = (addr as *mut u8).add(std::mem::size_of::<ArrayHeader>()) as *mut f64; | ||
| std::ptr::write(elements, 111.0); | ||
| std::ptr::write(elements.add(1), 222.0); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not write an array layout into a MapHeader allocation.
Line 188 points at the MapHeader::entries field. Line 189 overwrites the Map side-buffer pointer. Line 190 writes beyond the Map header allocation. Lines 209-216 do not restore entries, so later teardown can dereference or release corrupted memory.
Create a real array allocation and add a test-only helper that injects its address into the registry stale-entry state. Do not reuse the Map allocation as array storage.
Also applies to: 209-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/array/collection_tag_tests.rs` around lines 182 -
191, Replace the unsafe recycling block in the collection tag test with a
genuine ArrayHeader allocation sized for two f64 elements, rather than
reinterpreting the MapHeader allocation at addr. Add a test-only helper to place
the new array address into the registry’s stale-entry state, and update the
later cleanup around the existing registry test flow so the MapHeader entries
pointer remains untouched and both allocations are safely released.
proggeramlug
changed the title
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.
The finding
gc-handoff/apps/asyncpipe.ts— an async service pipeline, and at 13x node theworst gap in the corpus — spent 13.5% of its run in
set::is_registered_set+map::is_registered_map. Not on Set or Map work:on
js_array_get_f64andjs_array_lengthasking both collection registrieswhether an ordinary array was secretly a collection, on every element read.
Caller attribution on a symbolicated
sampleofasyncpipe_big.ts:is_registered_set(7.48%)js_array_get_f6476%,js_array_length11%is_registered_map(6.03%)js_array_get_f6482%,js_array_length16%#7755 made unused-feature registry probes free with monotone latches and named
Map/Set as the deliberate residual — asyncpipe uses both, so the #7474 latch is
correctly armed and each probe is real work — and named why the typed-array
trick does not transfer: an address-keyed negative memo is an ABA hazard for
Map/Set, whose headers are recyclable arena objects.
The mechanism
The object already knows.
js_map_alloc/js_set_allocallocate theirheaders through
arena_alloc_gc(_, _, GC_TYPE_MAP | GC_TYPE_SET), and each isthe single registration site for its registry — so a registered collection's
address is its GC header, and
obj_typeanswers in one byte. Both hot callsites gate their probes on it; the registry stays authoritative for the positive
answer, so nothing about when an entry is added or swept moves.
ABA-proof by construction rather than by bookkeeping: the tag lives INSIDE
the candidate bytes, so whatever allocation owns them next stamps its own
obj_typebefore the pointer is handed out. A recycled address answers for itsnew owner with no invalidation step to get wrong.
Correct for a header-less receiver too — buffers and typed arrays are
std::alloc-backed, so the bytes below them are allocator bookkeeping. Both arealready routed by the (latched, free) probes above, and either way those bytes
read the outcome is unchanged: a byte that happens to read as
GC_TYPE_SET/GC_TYPE_MAPstill falls through to the authoritative registry,and any other value skips a probe that would have answered
falseanyway.Neither call site gains a dereference — both already read this header.
The second commit takes the adjacent cluster the same way: the object field-get
funnel proves
keysis a liveGC_TYPE_ARRAYand caps the index below itscapacity, then called
js_array_getper key, which re-establishes both factsfrom scratch.
keys_array_slotserves the dense case from the array's own wordsand delegates everything else.
Tests
crates/perry-runtime/src/array/collection_tag_tests.rsasserts THE SUBJECT,not just the answer — the registry is a correct fallback, so a test that only
compared values would still pass with the gates deleted (CLAUDE.md, "four ways a
gate can be unable to fail", case 4). Both sabotage checks were run:
plain_array_element_reads_never_probe_the_collection_registries— 64 passes over a 4-element array moved the probe counters from 0 to 320is_registered_mapa_stale_registry_entry_over_recycled_bytes_does_not_read_as_a_map— the stale entry then reportstrueand the read servesMapHeader::entriesNumbers — absolute seconds
Quiet pinned M1 mini (
perry@perry-macos.local), static arm64 binaries shippedover, both arms timed back to back in the same session (the published floors
were set on a different host, so only a paired A/B means anything). Min of 7
runs. All outputs verified byte-identical to
node --experimental-strip-typesBEFORE timing. These figures were re-measured from scratch after an unrelated
machine reboot and reproduce the pre-reboot run to within 0.001 s.
churn_allocis the one number I would not call clean. Re-measuredinterleaved, 3 passes of best-of-11: base 0.3740 / 0.3728 / 0.3749, after
0.3802 / 0.3772 / 0.3798 — a reproducible +1.2 to +1.4% that sits ON its
0.38 floor rather than comfortably under it.
churn_alloc.tsiskeep.push({v, w})x 1000 x 20000 with onekeep.lengthper chunk: it executesnone of the paths this PR changes (no array element reads, no object field
reads, no Map or Set, so the #7474 latch is idle and both probes were already a
single atomic load each). The first commit measured identical to base on it
(0.3737 vs 0.3737); the drift appears with the second. That points at code
layout in
libperry_runtime.arather than an executed path, but I did notchase it down and cannot prove it, so it is reported as a real +1.2%.
Profile of
asyncpipe_big.ts(symbolicatedsample, two runs, in agreement):is_registered_set+is_registered_maparray_object_flagsjs_array_get_f64keys_array_slot, 2.6-3.1%)classify_heap_space_in_range(4.4% before) was left alone: caller attributionshows it is 100% GC-internal —
CopyingNurseryCollector::mark_addr51%,rewrite_value_bits37% — and #7742 already introduced the(space, base)form specifically to collapse the double lookup for addresses 8 bytes apart,
which
mark_addruses. Its share rises after this change only because themutator side shrank.
GC canaries.
gc-handoff/apps/iso_miss.tsprintschecksum 437840 misses 0plain, under
PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, and underPERRY_GC_VERIFY_EVACUATION=1.asyncpipestill exits 138 underPROTECT_FROMSPACEwith a from-space fault on anobj_type=5(Promise)object — that reproduces identically on the merge base, same signature, same
object type, correct output printed either way, so it is untouched here.
Gap suite (full 517,
PERRY_SKIP_BUILD=1against this build): 492 paritypass, 95.1%. Every divergence from
test-parity/gap_snapshot.jsonispre-existing and none is array/Map/Set/property-shaped — six http/fetch/net
crashes,
specabi_reassign,zlib_3285_params, tennode_fail -> parity_failstatus changes that are oracle-side (missing
node_modules,ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX; a runtime change cannot flip node's exitcode), and the two
gc_*_argument_rootingtests, whose entire diff is the[gc-zeal] forced_collections=...verdict line #7684 writes to stderr and theharness merges into stdout.
test_gap_iterator_helpers_2874goesparity_fail -> pass.Two comments claiming Map/Set headers are
alloc()-backed with noGcHeader—the stated reason the registries are consulted before any header read — are
corrected in place. They predate the move into the managed arena, and are how
the answer stayed hidden.
Summary by CodeRabbit