perf(runtime): delete REGEX_SOURCE_TABLE; identify RegExps by header - #11518
Conversation
`REGEX_SOURCE_TABLE` was an address-keyed thread-local map whose only payload was `registered_owner: bool`. Every construction inserted into it, every copying minor rekeyed it, and every collection walked it twice (the copied-minor from-space finalizer and the sweep-entry dead-regexp subphase) only to clear dead RegExps' expandos, which the dead-owner fan-out (`prune_dead_exotic_expando_owners`) already did in the same windows. - `is_regex_pointer` / `is_valid_regex_ptr` / `is_registered_regex` answer from the header (GC_TYPE_REGEXP + size + REGEXP_MAGIC), which they already checked first. Every `registered_owner` reader was a membership check, so no other semantics are lost. - GC_TYPE_REGEXP uses the shared ExoticExpandoOwner move hook and no finalize hook. The RegExpSideTables hook kinds, the copied-minor regex finalizer, the sweep's dead_regexps list, the REGEX_EVER_REGISTERED latch, and the now-unused prefetch_gc_owner_headers / exotic_expando_owner_clear_dead helpers are removed. - Gates: drop the REGEX_SOURCE_TABLE entry from gc_runtime_root_holders.json; shape_descriptor_census.py now pins RegExp to ExoticExpandoOwner + GcFinalizeHookKind::None. Tests cover header-only identity, the hook wiring, expando pruning for a dead RegExp on a full GC and on a copying minor (asserting the minor ran), and expando migration for a live RegExp that moves. Closes #11503
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (19)
💤 Files with no reviewable changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change removes the RegExp address registry and its GC rekeying and finalization paths. RegExp identity checks use GC header metadata, while expando cleanup uses shared dead-owner handling. Tests and GC metadata checks are updated. The GC owner-header prefetch helper is also removed. ChangesRegExp identity and GC lifecycle
GC owner-header prefetch removal
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Possibly related PRs
Suggested labels: Merge Risk: ⚪ Minimal · up to No merge-blocking issue was established; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change affects how the runtime recognizes and cleans up RegExp objects across garbage collections. The inspected paths support the replacement design, and no new security failure was established, but incomplete lifecycle coverage leaves some uncertainty in a memory-sensitive component. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The implementation satisfies the main [ Resolution Run the required regex gap tests and runtime regex tests with default settings and with Full details: Docstring CoverageExplanation Docstring coverage is 77.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 15 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
b471ff8 to
440ccf0
Compare
2 participants
Summary
This deletes
REGEX_SOURCE_TABLE. It was an address-keyed thread-localPtrHashMap<usize, RegexMetadata>, and its only payload wasregistered_owner: bool. Even so, every RegExp construction inserted into it, every copying minor rekeyed it, and every collection walked it twice just to find dead RegExps and clear their expandos. The shared dead-owner fan-out (prune_dead_exotic_expando_owners) already does that clearing in the same windows. A RegExp is now identified by its own GC header.Changes
regex.rs):is_regex_pointer,is_valid_regex_ptrandis_registered_regexnow answer from the header alone (GC_TYPE_REGEXP+ size +REGEXP_MAGIC). They already checked the header first; the table fallback only changed the answer for a stale entry. I checked every reader ofregistered_owner: the probe fallback, the rekey merge and the two death walks. All of them are "is this a regex we allocated" membership checks, with no other meaning.gc/types.rs):GC_TYPE_REGEXPnow uses the sharedGcMoveHookKind::ExoticExpandoOwnermove hook andGcFinalizeHookKind::None. The following are removed:GcMoveHookKind::RegExpSideTablesandGcFinalizeHookKind::RegExpSideTablesregex_header_moved_for_gc,_clear_dead_for_gcand_finalize_for_gcfinalize_dead_copied_minor_from_space_regexpspass and its+regex:diag fielddead_regexpssubphase (collect_dead_registered_regexps_post_trace/finalize_collected_dead_regexp)REGEX_EVER_REGISTEREDlatchprefetch_gc_owner_headersandexotic_expando_owner_clear_dead, whose only callers were the removed codeexpando_clear_on_allocat construction still covers an address that gets reused. The sweeper keeps pinned objects live, so no path frees a RegExp without the fan-out seeing it first.block_skipmay now reclaim whole dead blocks holding RegExps without visiting them.REGEX_SOURCE_TABLEentry fromscripts/gc_runtime_root_holders.json.scripts/shape_descriptor_census.pynow requires RegExp's type metadata to carryExoticExpandoOwnerandGcFinalizeHookKind::None.gc_rekeyed_key_tables.jsonandDEAD_KEY_PRUNEShad no entry for this table: it was rekeyed by a move hook, not avisit_metadata_*site, so nothing needed deleting there.dead_owner.rs,json_tape_store.rs,hot_diag.rs(the regex side-table counters are now documented as zeroed after-controls) andexotic_expando.rs.Cargo.toml/CLAUDE.md, andCargo.lock.Related issue
Closes #11503 (part of #9908).
Test plan
New and updated tests:
regex::tests::regexp_identity_is_the_header_not_an_address_registry: a header registered nowhere is identified by all three probes, and clearing its magic makes all three say no.regex::tests::regexp_gc_type_needs_no_bespoke_side_table_hooksgc::tests::dead_owner_side_tables::regexp_expandos::test_dead_regexp_expando_pruned_on_full_gc(usesfull_gc_with_no_block_persistence, so the owner is really dead) andtest_live_regexp_expando_survives_full_gcnursery_regexp_that_dies_young_is_finalized_by_the_copied_minor: sets expandos through the production[[Set]]path. It checks that a copying minor ran, that the dead RegExp's entry is gone, and that the live one's value moved to its new address.test_movable_regexp_evacuation_migrates_all_address_owned_state: now checks the copying minor ran and the expando migrated, instead of reading the deleted table.Sabotage checks. For each one I applied the sabotage, rebuilt and saw the tests go red, then restored the code:
prune_dead_exotic_expando_ownersmade to skip RegExp owners →test_dead_regexp_expando_pruned_on_full_gcandnursery_regexp_that_dies_young…fail at their "expando must be pruned" assertions.GcMoveHookKind::None→test_movable_regexp_evacuation…,nursery_regexp_that_dies_young…andregexp_gc_type_needs_no_bespoke_side_table_hooksfail.regex_header_has_magicmade to ignore the magic word →regexp_identity_is_the_header_not_an_address_registryfails.Results:
RUST_TEST_THREADS=1 cargo test --lib -p perry-runtime: 4646 passed, 0 failed, 4 ignored.PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1, filtersregex perex regexp_expando: 210 passed, 0 failed.copying dead_ownerunder the same knobs, 4 copying tests fail:set_index::{old_identity_keys_are_skipped_by_a_minor, young_identity_half_prunes_dead_keys_and_follows_moved_ones}andtest_copying_minor_rewrites_exact_{closure_pointer_capture,object_pointer_slot}_only. They fail identically on pristinemainunder these knobs, because they assert exact slot-read counts. Not caused by this PR.RUSTFLAGS="-D warnings" cargo check -p perry-runtime -p perry-stdlib --all-targets, plus-p perry-runtime --no-default-features: clean.cargo check --locked -p perry-stdlib --no-default-features(as CI runs it): clean.cargo clippy -p perry-runtime --all-targets: no new findings in any touched file compared withmain.SKIP_COMPILE_GATES=1 scripts/run_lint_gates.sh: 91/94 pass, including every GC inventory gate,cargo fmt --checkandcheck_file_size.sh. The 3 that fail do so for reasons outside this PR: no Bun on the host, nocargo-xwin, andci_public_baseline_check.py, which fails identically on pristinemainin this container.cargo build --releaseand the regex gap tests: not run locally.perryneeds LLVM 22, and this container's network policy blocksapt.llvm.org. The PR-tier gap-suite shards cover the default run. I have not run the gap tests underPERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1; that part of the issue's Verify list is still open.#[test]s in the affected crate.docs/src/(no API change).Checklist
perf:prefix conventionGenerated by Claude Code
Summary by CodeRabbit
Updates
Tests