diff --git a/CLAUDE.md b/CLAUDE.md index f38f97c480..068651578d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Perry is a native TypeScript compiler written in Rust that compiles TypeScript source code directly to native executables. It uses SWC for TypeScript parsing and LLVM for code generation. -**Current Version:** 0.5.1359 +**Current Version:** 0.5.1360 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index 1a506dc0ff..14da3dc9fb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5547,7 +5547,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "base64", @@ -5607,14 +5607,14 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "serde", ] [[package]] name = "perry-audio-miniaudio" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "cc", "libc", @@ -5622,7 +5622,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "inkwell", @@ -5639,7 +5639,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-hir", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-hir", @@ -5655,7 +5655,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-dispatch", @@ -5664,7 +5664,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-hir", @@ -5672,7 +5672,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "base64", @@ -5684,7 +5684,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-hir", @@ -5692,7 +5692,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "async-trait", @@ -5721,14 +5721,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "serde", "serde_json", @@ -5736,7 +5736,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1359" +version = "0.5.1360" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5747,7 +5747,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "clap", @@ -5762,7 +5762,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "block2", "objc2", @@ -5772,7 +5772,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "argon2", "perry-ffi", @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "perry-ext-axios" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "reqwest", @@ -5789,7 +5789,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "bcrypt", "perry-ffi", @@ -5797,7 +5797,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "rusqlite", @@ -5805,7 +5805,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "scraper", @@ -5813,7 +5813,7 @@ dependencies = [ [[package]] name = "perry-ext-commander" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "perry-runtime", @@ -5821,7 +5821,7 @@ dependencies = [ [[package]] name = "perry-ext-cron" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "chrono", "cron", @@ -5831,7 +5831,7 @@ dependencies = [ [[package]] name = "perry-ext-dayjs" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "chrono", "perry-ffi", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "rust_decimal", @@ -5847,7 +5847,7 @@ dependencies = [ [[package]] name = "perry-ext-dotenv" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "serde_json", @@ -5855,7 +5855,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "rand 0.10.1", @@ -5863,7 +5863,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "perry-runtime", @@ -5871,14 +5871,14 @@ dependencies = [ [[package]] name = "perry-ext-exponential-backoff" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-fastify" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "bytes", "http-body-util", @@ -5896,7 +5896,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "bytes", "lazy_static", @@ -5909,7 +5909,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "bytes", "h2", @@ -5933,7 +5933,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "lazy_static", "perry-ffi", @@ -5943,7 +5943,7 @@ dependencies = [ [[package]] name = "perry-ext-jsonwebtoken" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "jsonwebtoken", @@ -5954,7 +5954,7 @@ dependencies = [ [[package]] name = "perry-ext-lru-cache" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "lru", "perry-ffi", @@ -5963,7 +5963,7 @@ dependencies = [ [[package]] name = "perry-ext-moment" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "chrono", "perry-ffi", @@ -5971,7 +5971,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "bson", "futures-util", @@ -5983,7 +5983,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "chrono", "perry-ffi", @@ -5993,7 +5993,7 @@ dependencies = [ [[package]] name = "perry-ext-nanoid" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "nanoid", "perry-ffi", @@ -6002,7 +6002,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "bytes", "perry-ffi", @@ -6015,7 +6015,7 @@ dependencies = [ [[package]] name = "perry-ext-node-forge" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "const-oid 0.9.6", "der 0.7.10", @@ -6034,7 +6034,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "lettre", "perry-ffi", @@ -6044,7 +6044,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "printpdf", @@ -6052,7 +6052,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "sqlx", @@ -6061,7 +6061,7 @@ dependencies = [ [[package]] name = "perry-ext-ratelimit" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "governor", "perry-ffi", @@ -6069,7 +6069,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "fast_image_resize", "image", @@ -6079,14 +6079,14 @@ dependencies = [ [[package]] name = "perry-ext-slugify" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-streams" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "lazy_static", "perry-ffi", @@ -6095,7 +6095,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "perry-runtime", @@ -6104,7 +6104,7 @@ dependencies = [ [[package]] name = "perry-ext-uuid" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "uuid", @@ -6112,7 +6112,7 @@ dependencies = [ [[package]] name = "perry-ext-validator" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ffi", "regex", @@ -6122,7 +6122,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "futures-util", "lazy_static", @@ -6135,7 +6135,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "brotli", "flate2", @@ -6145,7 +6145,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "dashmap", "once_cell", @@ -6154,7 +6154,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-api-manifest", @@ -6172,7 +6172,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-diagnostics", @@ -6184,7 +6184,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "base64", @@ -6226,14 +6226,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6328,14 +6328,14 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "perry-hir", @@ -6344,14 +6344,14 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ui-model", ] [[package]] name = "perry-ui-android" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "itoa", @@ -6368,7 +6368,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "rand 0.10.1", "serde", @@ -6378,7 +6378,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "cairo-rs 0.22.0", @@ -6401,7 +6401,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "block2", @@ -6417,7 +6417,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "block2", @@ -6432,7 +6432,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1359" +version = "0.5.1360" [[package]] name = "perry-ui-test" @@ -6443,11 +6443,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1359" +version = "0.5.1360" [[package]] name = "perry-ui-tvos" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "block2", @@ -6463,7 +6463,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "block2", @@ -6479,7 +6479,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "block2", "libc", @@ -6492,7 +6492,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "base64", "libc", @@ -6509,14 +6509,14 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "perry-ui-windows", ] [[package]] name = "perry-updater" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "anyhow", "base64", @@ -6532,7 +6532,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1359" +version = "0.5.1360" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index cc5d306f7d..4aecd2ec52 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -315,7 +315,7 @@ codegen-units = 16 codegen-units = 16 [workspace.package] -version = "0.5.1359" +version = "0.5.1360" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/7624-defer-old-page-registration.md b/changelog.d/7624-defer-old-page-registration.md new file mode 100644 index 0000000000..ec54d56042 --- /dev/null +++ b/changelog.d/7624-defer-old-page-registration.md @@ -0,0 +1,224 @@ +### Old-object page registration is deferred off the promote path (#7624) + +Extracted from #7623 per its audit: that PR's static-pretenure half was a +measurement confound and is not merging, but the `register_old_object_pages` +finding inside it stands alone — and pays on current `main`, with no codegen +change and no allocator-policy change. + +**The cost.** `register_old_object_pages` was written for the occasional +old-gen birth. Per object it pays two `RefCell` borrows, two `Vec` allocations, +a hash lookup, and a **linear `contains` scan of that page's object list** — +which grows as the page fills, so a burst of births into one 4 KiB page is +quadratic in the objects it lands there. Since **#7613's promote-on-first-copy** +that is no longer an occasional path: a copying minor promotes straight into +old-gen (`gc/copying.rs`'s `move_young` → `arena_alloc_gc_old`), so +json_pipeline pushes ~113 MB of promotions per run through it. + +**The change.** `arena_alloc_gc_old` records `(header_addr, total_size)` in a +thread-local buffer (`arena/page_meta.rs`); one batched flush folds the burst +in, holding a single borrow of each table, allocating no per-object `Vec`, and +scanning only the portion of a page's object list that **predates the batch**. +A bump-allocated promotion burst fills fresh pages, where that prefix is empty +and the dedup scan disappears. Skipping in-batch entries is sound because they +are pairwise distinct — an address cannot be handed out twice without an +intervening free, and no free happens without a flush; hole reuse, the reason +the dedup exists, hands back an address registered *before* the batch and is +still covered. + +Allocation policy is deliberately unchanged: the `old_free_take_exact` hole +probe stays. (#7623 also dropped it on its pretenure allocator; that is a +separate change with its own RSS consequences and is not here.) + +**Caller disposition.** + +| caller | disposition | why | +|---|---|---| +| `gc/copying.rs:614` promote (`arena_alloc_gc_old`) | **defer** | the target: per-object, ~113 MB/run since #7613 | +| `gc/oldgen.rs:1735` evacuate-tenured-nursery (`arena_alloc_gc_old`) | **defer** | per-object, same function | +| `typedarray`, `buffer`, `native_arena`, `json_tape` (via `arena_alloc_gc_old_born_tenured`), `arena_alloc_gc` large-object arm | **defer** | inherited; rare/large, so neither helped nor harmed, and one code path is easier to reason about than two | +| `gc/oldgen.rs:1843` defrag relocation (`arena_alloc_gc_old_excluding_pages`) | **eager** | rare; per-object cost dominated by the `copy_nonoverlapping` beside it; runs inside `old_arena_walk_objects_on_pages`' callback. Keeping it eager narrows the proof obligation | + +**Soundness — one rule.** Every reader **and every remover** of +`OLD_GEN_PAGE_OBJECTS` / `OLD_GEN_PAGE_META` flushes first. Both tables are +thread-locals private to `arena/page_meta.rs`, so the toucher set is closed and +the rule is checkable. + +Removers matter as much as readers, and that is the part that is easy to get +wrong: a removal that runs while an entry is still deferred is a **no-op**, and +the later flush then puts the dead object back — a resurrected index entry +pointing into swept or recycled memory. + +| flush site | kind | why it cannot rely on cycle start | +|---|---|---| +| `old_pages_begin_gc_cycle` | cycle start | all three constructors route through it (`gc/mod.rs` minor, `gc/cycle.rs` `new_full`, `gc/policy.rs` budgeted) | +| `old_arena_walk_objects_on_pages` | reader | a copying minor's root scan promotes **before** the remembered-set walk reads the index | +| `OldArenaPageObjectCursor::new` | reader | same index, incremental (budgeted) reader | +| `old_page_summary` | reader (`META`) | a deferred entry also owes `allocated_bytes`/`object_count` | +| `old_page_meta_snapshot` | reader (`META`) | drives `gc/oldgen_defrag.rs` page selection — real policy | +| `old_pages_reset_sweep_accounting` | reader (`META`) | closes the promote→sweep window inside a full cycle | +| `old_page_meta_for_tests` | reader (`META`) | keeps existing allocate-then-inspect tests honest | +| `unregister_old_object_pages` | remover | resurrection | +| `old_arena_page_index_remove_object` | remover | resurrection | +| `unregister_old_block_pages` | remover | resurrection into a recycled block | +| size cap (8k entries, 128 KB) | bound | the buffer cannot grow without a collection | +| `old_arena_page_index_clear_for_tests` | **discards** | a caller asking for an empty index must not get a repopulated one | + +Two consequences worth recording: + +- `classify_heap_generation` — every barrier remember-decision — reads the + **block-level** `PAGE_GENERATIONS` map, populated by `register_old_block_pages` + when a block is created. It never consults the object index, so it is + unchanged. (The #7623 audit reached the same conclusion for its shape; this + was re-verified for this caller set.) +- The per-object `META` writers (`old_page_account_swept_object`, + `old_page_account_promoted_object`) call `refresh_policy_bits()`, which reads + `allocated_bytes`. They can run while a registration is pending and therefore + recompute a bit from a stale count — but the flush itself calls + `refresh_policy_bits()` for every page it touches, and every reader flushes + first, so no reader can observe a stale bit. They stay flush-free so the + per-object sweep path pays nothing. + +**Tests.** Seven per-obligation unit tests in `arena/tests.rs`, all +**sabotage-verified**: a harness removes one flush site at a time and requires +the matching test to go red — 9 cases, 9 caught. That includes "revert the +promote path to eager registration", which turns +`old_gen_birth_defers_its_page_registration` red, so a later refactor cannot +silently make this inert (the #7024/#6942 "gate whose subject never ran" +failure mode). `every_cycle_constructor_routes_through_the_flush_point` is the +second half of the cycle-start claim: one test proves +`old_pages_begin_gc_cycle` flushes, that one proves all three constructors +still call it. `every_old_gen_birth_path_sets_tenured` stays green. + +Those seven pin the flush sites that exist *today*; they are blind to one added +later. `deferred_registration_flush_sites` closes that — it enumerates every +function in `page_meta.rs` touching either table and requires a flush or a +written exemption, and fails on a **stale** exemption too so the list cannot rot +into suppression. It is not hypothetical: on its first run it caught +`OldArenaPageObjectCursor::next` (deliberately flush-free, now exempt with the +argument attached). Both of its arms are sabotage-verified — a bogus exemption +and a newly added unflushed toucher each turn it red. + +## Measured — pinned quiet host (`perry-macos`, M1 mini) + +> An earlier revision of this description carried a wall/user/RSS table taken +> while a second `run_public_baseline` was concurrently executing on the mini. +> **That table is superseded by this one.** The GC census rows were never +> affected — they are load-independent — and are unchanged. + +Both arms `perry-dev`, identical package set, one target dir each. Workloads +compiled on the dev Mac with `PERRY_NO_AUTO_OPTIMIZE=1` and the **prebuilt +executables shipped** to the mini, so nothing was rebuilt on the measurement +host. 5 rounds, base/fix interleaved within each round, every row hash-verified. + +**Idleness was gated, not assumed.** The run waits for all three of: no +`run_public_baseline` process, a `SCRIPT REAL EXIT=` marker in +`/tmp/baseline_mini.log`, and 1-min load < 2.0 — then settles 60 s, measures, +and **re-checks all three afterwards**. Load recorded by the A/B itself at its +own start: `1.62 2.04 2.45`. + +Deltas are **medians of per-round paired deltas**, which is the statistic the +interleaving exists to support; see the `cycles` note below for why +median-of-medians is not safe here. + +### json_pipeline + +| | base | fix | Δ (paired) | +|---|--:|--:|--:| +| 200k wall | 1.53 s | **1.47 s** | **−3.9%** | +| 200k user CPU | 1.48 s | **1.42 s** | **−4.1%** | +| 200k peak RSS | 489.0 MB | **486.8 MB** | −0.5% | +| 500k wall | 4.09 s | **3.94 s** | **−3.7%** | +| 500k user CPU | 3.95 s | **3.80 s** | **−3.8%** | +| 500k peak RSS | 1,110.4 MB | 1,114.7 MB | +0.4% | + +**All 20 paired json deltas are negative** — 200k wall −4.5/−3.9/−4.6/−3.9/−3.9%, +500k wall −3.9/−3.7/−3.4/−3.7/−3.9%. Output hashes identical at both sizes. + +``` +200k base real 1.54 1.53 1.53 1.53 1.53 fix 1.47 1.47 1.46 1.47 1.47 +500k base real 4.10 4.09 4.09 4.09 4.11 fix 3.94 3.94 3.95 3.94 3.95 +``` + +**The clean host both shrank the effect and shrank the noise.** Base 200k wall +now spans 1.53–1.54 s (0.7%) where under the concurrent baseline it spanned +1.63–1.75 s (7%). The honest win is **smaller** than the superseded table +claimed (−3.9%/−3.7% vs −4.9%/−4.1%), and that table's 200k RSS "win" (−3.7%) +was noise — it is −0.5% here. + +### gc bench set (`gc-handoff/bench`) + +| workload | wall Δ (paired) | RSS Δ | output | +|---|--:|--:|---| +| retain1 | **−4.6%** | **−6.6%** | identical | +| retain | **−4.5%** | −0.6% | identical | +| churn_alloc | −2.5% | +0.3% | identical | +| deeplist | −2.4% | +1.6% | identical | +| churn / churn_read / churn_num / push_cls / push_num / cycles | 0.0% | +0.0…+1.3% | identical | +| tree | +0.8% | −2.2% | identical | + +All eleven produce byte-identical stdout. The wins land where the mechanism +predicts — `retain`/`retain1`/`deeplist` are the promote-heavy ones. + +> **`cycles` is reported at +0.0%, not +18.5%.** Median-of-medians says +18.5%; +> that is an artifact. The workload is **bimodal in both arms** (rounds 1–3 +> ≈ 0.79 s, rounds 4–5 ≈ 0.96 s), so the two arms' medians land on different +> modes. The paired deltas are 0.00 in three of five rounds and the run-1 +> outlier is the whole difference. This is exactly what interleaving is for, and +> it is why every number above is a paired statistic. + +### GC census — identical, and load-independent + +`CENSUS 200k IDENTICAL`, `CENSUS 500k IDENTICAL`: same cycle sequence, same +`promoted_objects`/`promoted_bytes`, same sweep and reclaim. 200k promotes +**1,657,962 objects / 113,226,896 bytes**; 500k promotes **4,117,011 / +280,996,760** — all through the path this PR touches, none of it moving. + +### gc-ratchet (the #7609 baseline), both arms, clean host + +Both arms measured back-to-back in the same session on the clean host, +`measure --repeats 7`, then `check` on both profiles. 144 cells per arm. + +| | base (`origin/main`) | fix | +|---|---|---| +| `shared_ci` (what CI gates on) | **OK** | **OK** | +| `pinned_host` | **FAILED**, 10 regression rows | FAILED, 15 rows | + +**Read the base column first.** Pure `origin/main` fails `pinned_host` on this +host with ten RSS rows of its own (`03_cross_gen_writes` +3.83%, +`08_map_set_sidetables` +4.20%, `04_dead_after_deep_stack` +3.72%, …). The +pinned artifact was captured at `main 26b9c9d59` (0.5.1346) and we are at +0.5.1355, so the profile's RSS bands no longer describe this host/version. +**"fix fails `pinned_host`" is therefore not a statement about this PR** — the +only sound comparison is base vs fix in the same session, which is what follows. + +**fix vs base, all 144 cells:** + +- **GC semantics: 107 of 108 cells byte-identical.** The single exception is + `12_large_live_set.heap_used_bytes` (59,946,104 → 59,944,160, −1,944 B) — the + one cell the harness explicitly de-gates by probe override because it is + conservative-stack-scan sample-dependent, with a documented spread of 9,072 B + over 36 runs. The difference is under a quarter of that spread. Every + `copied_*`, `promoted_*`, `freed_bytes`, `minor_cycles`, `step_cycles` and + `heap_total_bytes` cell is identical. +- **Memory: 24 cells, median fix-vs-base +0.23%**, range −0.30% to +1.46% + (largest: `07_array_grow_evacuate.peak_rss_bytes` +1.46%). +- **Wall: 12 cells, median fix-vs-base +0.0%.** These probes are microbenchmarks + where the deferral has almost nothing to do; the promote-heavy work is + json_pipeline's. + +**And this retires the open question from the earlier revision.** I had flagged +`11_collect_at_depth.rss_bytes` as an unexplained ~+1.07 MB, with "allocator +segment granularity" as an untested hypothesis. Measuring **base on the same +clean host** answers it: + +| | `11_collect_at_depth.rss_bytes` | vs pinned artifact | +|---|--:|--:| +| pinned baseline (0.5.1346) | 34,652,160 | — | +| **base arm = pure `origin/main`** | 35,651,584 | **+2.88%** (ok — just under the 1,039,565 band) | +| fix arm | 35,749,888 | +3.17% (REGRESSION — just over) | + +**fix is +98,304 B (+0.28%) above base, not +1.07 MB.** Base already sat at 96% +of the allowance, so the cell tips over on a rounding-scale difference. The row +is ~91% pre-existing drift in `origin/main` and ~9% this PR. No allocator-granularity +story is needed, and the one I floated should be disregarded. + diff --git a/crates/perry-runtime/src/arena/allocators.rs b/crates/perry-runtime/src/arena/allocators.rs index 1270929140..f20ff58989 100644 --- a/crates/perry-runtime/src/arena/allocators.rs +++ b/crates/perry-runtime/src/arena/allocators.rs @@ -116,6 +116,16 @@ pub(crate) fn arena_alloc_old_excluding_pages( /// GcHeader-prefixed counterpart of `arena_alloc_old`. See /// `arena_alloc_gc_longlived` for the same shape on the longlived /// arena — only the backing region differs. +/// +/// #7624: page registration is DEFERRED here (`defer_old_object_page_registration` +/// rather than `register_old_object_pages`). This is the per-object old-gen +/// birth path — since #7613's promote-on-first-copy it carries every promotion +/// a copying minor makes, ~113 MB per json_pipeline run — and eager +/// registration costs two `RefCell` borrows, two `Vec` allocations, and a +/// linear dedup scan that grows as the page fills. Allocation policy is +/// deliberately UNCHANGED: the `old_free_take_exact` hole probe below stays, +/// so this is a bookkeeping change only. See the flush discipline in +/// `arena/page_meta.rs`. pub fn arena_alloc_gc_old(size: usize, align: usize, obj_type: u8) -> *mut u8 { use crate::gc::{GcHeader, GC_FLAG_ARENA, GC_HEADER_SIZE}; @@ -136,7 +146,7 @@ pub fn arena_alloc_gc_old(size: usize, align: usize, obj_type: u8) -> *mut u8 { (*header)._reserved = 0; (*header).size = total as u32; } - register_old_object_pages(raw as usize, total); + defer_old_object_page_registration(raw as usize, total); return user_ptr as *mut u8; } let raw = arena_alloc_old(total, align); @@ -149,7 +159,7 @@ pub fn arena_alloc_gc_old(size: usize, align: usize, obj_type: u8) -> *mut u8 { (*header)._reserved = 0; (*header).size = total as u32; } - register_old_object_pages(raw as usize, total); + defer_old_object_page_registration(raw as usize, total); unsafe { raw.add(GC_HEADER_SIZE) } } @@ -174,6 +184,16 @@ pub(crate) fn arena_alloc_gc_old_born_tenured(size: usize, align: usize, obj_typ user_ptr } +/// #7624: registration stays EAGER here, unlike `arena_alloc_gc_old`. This is +/// old-page defrag's relocation allocator (`gc/oldgen.rs`'s +/// `evacuate_selected_old_pages_collecting`), which runs from INSIDE +/// `old_arena_walk_objects_on_pages`' callback — i.e. downstream of that +/// reader's own flush. Deferring would be sound (the walk snapshots its header +/// list before invoking the callback, and the flush discipline covers the +/// rest), but it buys nothing: defrag is a rare, per-cycle pass whose +/// per-object cost is dominated by the `copy_nonoverlapping` beside it, and +/// keeping it eager keeps the deferral's proof obligation to the one path that +/// measurably needs it. pub(crate) fn arena_alloc_gc_old_excluding_pages( size: usize, align: usize, diff --git a/crates/perry-runtime/src/arena/mod.rs b/crates/perry-runtime/src/arena/mod.rs index 04027c5551..9015ba1120 100644 --- a/crates/perry-runtime/src/arena/mod.rs +++ b/crates/perry-runtime/src/arena/mod.rs @@ -43,9 +43,9 @@ pub(crate) use block::{ gc_trigger_arena_calls, reset_gc_trigger_arena_probe, }; pub(crate) use page_meta::{ - address_span_overlaps_pages, register_block_space, register_old_object_pages, - unregister_block_generation, unregister_old_block_pages, OLD_GEN_RECLAIM_RETURNED_BYTES, - OLD_GEN_RECLAIM_REUSABLE_BYTES, + address_span_overlaps_pages, defer_old_object_page_registration, register_block_space, + register_old_object_pages, unregister_block_generation, unregister_old_block_pages, + OLD_GEN_RECLAIM_RETURNED_BYTES, OLD_GEN_RECLAIM_REUSABLE_BYTES, }; pub(crate) use page_meta::{page_generation_cache_hot_addr, page_generations_hot_addr}; @@ -122,6 +122,7 @@ pub(crate) use page_meta::{ #[cfg(test)] pub(crate) use page_meta::{ - generation_page_base, old_arena_page_index_clear_for_tests, old_page_meta_for_tests, - GENERATION_CLASS_SHIFT, GENERATION_PAGE_SIZE, + deferred_old_page_registrations_len, generation_page_base, + old_arena_page_index_clear_for_tests, old_page_meta_for_tests, + DEFERRED_OLD_PAGE_REGISTRATION_CAP, GENERATION_CLASS_SHIFT, GENERATION_PAGE_SIZE, }; diff --git a/crates/perry-runtime/src/arena/page_meta.rs b/crates/perry-runtime/src/arena/page_meta.rs index 0b4653b17e..ae9c7cee9f 100644 --- a/crates/perry-runtime/src/arena/page_meta.rs +++ b/crates/perry-runtime/src/arena/page_meta.rs @@ -400,6 +400,10 @@ pub(crate) fn unregister_old_block_pages(pages: &[usize]) { if pages.is_empty() { return; } + // #7624 REMOVER: a deferred entry for one of these pages must be folded in + // BEFORE the page is dropped, or the flush would re-add it afterwards and + // hand a later walk a header inside a recycled block. + flush_deferred_old_page_registrations(); OLD_GEN_PAGE_META.with(|meta| { let mut meta = meta.borrow_mut(); for &page in pages { @@ -656,11 +660,194 @@ pub(crate) fn register_old_object_pages(header_addr: usize, total_size: usize) { update_old_page_meta_for_object(&added_pages, true); } +// --------------------------------------------------------------------------- +// #7624: deferred old-object page registration. +// +// `register_old_object_pages` above is written for the OCCASIONAL old-gen +// birth it was introduced for. Per call it pays two `RefCell` borrows, two +// `Vec` allocations, a hash lookup, and a **linear `contains` scan of the +// page's object list** — and that scan grows as the page fills, so a burst of +// births into one 4 KiB page is quadratic in the objects it lands there. +// +// #7613's promote-on-first-copy made that path hot on ordinary workloads: a +// copying minor promotes straight into old-gen (`gc/copying.rs`'s `move_young` +// → `arena_alloc_gc_old`), so json_pipeline pushes ~113 MB of promotions per +// run through it. Deferring lets the whole burst be registered in ONE batch, +// where the per-page list length is captured once and the dedup scan only has +// to cover the entries that predate the batch — zero comparisons for the fresh +// pages a bump-allocated promotion burst actually fills. +// +// SOUNDNESS. The deferral is invisible because **every reader and every +// remover of `OLD_GEN_PAGE_OBJECTS`/`OLD_GEN_PAGE_META` flushes first**, so +// the index is exactly what eager registration would have left at each +// observation point. Ordering matters in both directions: a remover that ran +// before the flush would be a no-op and the flush would then RESURRECT a dead +// entry, which is why removers flush too and not only readers. The flush sites +// are enumerated in `deferred_registration_flush_sites` in `arena/tests.rs`, +// which fails if a new toucher of either table appears without one. +// --------------------------------------------------------------------------- + +thread_local! { + /// Old-object page registrations not yet folded into `OLD_GEN_PAGE_OBJECTS`. + /// Entries are `(header_addr, total_size)`; nothing here is dereferenced, so + /// a deferred entry never keeps an object alive and is not a GC root — and + /// the flush discipline above means the buffer is provably EMPTY at every + /// point a collector could observe it (asserted by + /// `deferred_buffer_is_empty_after_every_cycle_constructor`). + static DEFERRED_OLD_PAGE_REGISTRATIONS: RefCell> = + const { RefCell::new(Vec::new()) }; +} + +/// Bound on the buffer between flushes, and therefore on its resident +/// footprint: 16 B/entry × 8k = **128 KB**. +/// +/// This started at 64k entries (1 MB), inherited from #7623 where the buffer +/// backed a different shape. Nothing here wanted 64k: the cap exists to amortise +/// the per-batch loop, 8k already does that ~8,000×, and since the flush is +/// allocation-free the extra batches cost only the loop entry. So it is sized +/// for the footprint it has to justify. +/// +/// It was reduced while chasing a `gc-ratchet` `pinned_host` row — +/// `11_collect_at_depth.rss_bytes`, ~+1.07 MB above the pinned artifact — on the +/// theory that the row WAS this buffer. Two measurements later that theory is +/// dead twice over, and the cap had nothing to do with it: +/// +/// 1. Shrinking the cap 8× (1 MB → 128 KB) moved the cell +16 KB in the WRONG +/// direction when it should have shed ~0.9 MB. The probe also promotes +/// **zero** objects, so this path is inert on it. +/// 2. Measuring **`origin/main` itself** on the same idle host settled it: +/// base reads 35,651,584 on that cell (+2.88% over the pinned artifact, just +/// under the band) against fix's 35,749,888. **fix is +98 KB over base, not +/// +1.07 MB** — the row is ~91% pre-existing drift between the artifact +/// (pinned at 0.5.1346) and current `main`, and base fails ten other +/// `pinned_host` RSS cells on its own. +/// +/// The smaller cap is kept because 128 KB beats 1 MB on its own terms, not +/// because it fixed anything. +pub(crate) const DEFERRED_OLD_PAGE_REGISTRATION_CAP: usize = 8_192; + +/// Record `header_addr`'s page registration for the next flush instead of +/// performing it now. Callers must be old-gen births; see the module comment +/// for why this is invisible to every consumer of the index. +#[inline] +pub(crate) fn defer_old_object_page_registration(header_addr: usize, total_size: usize) { + if header_addr == 0 || total_size == 0 { + return; + } + let full = DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| { + let mut buf = buf.borrow_mut(); + buf.push((header_addr, total_size)); + buf.len() >= DEFERRED_OLD_PAGE_REGISTRATION_CAP + }); + if full { + flush_deferred_old_page_registrations_batch(); + } +} + +/// Make the page-objects index complete. Cheap (one thread-local read) when +/// nothing is pending, which is the case at all but a handful of GC-time calls. +#[inline] +pub(crate) fn flush_deferred_old_page_registrations() { + if DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| buf.borrow().is_empty()) { + return; + } + flush_deferred_old_page_registrations_batch(); +} + +/// The batched drain. Equivalent to `register_old_object_pages` per entry, but +/// holding one borrow of each table for the whole batch and — the part that +/// removes the quadratic term — scanning only the portion of a page's object +/// list that PREDATES this batch. +/// +/// Skipping the in-batch entries is sound because they are pairwise distinct: +/// each comes from a live allocation, and an address cannot be handed out twice +/// without an intervening free, which cannot happen without a flush (every +/// remover flushes). Hole reuse — the reason the dedup exists at all — hands +/// back an address registered BEFORE the batch, so it is still covered. +/// The batch also holds BOTH table borrows at once and applies the +/// `OLD_GEN_PAGE_META` update inline rather than staging it in a `Vec`. The two +/// thread-locals are distinct cells, so there is no aliasing; the payoff is that +/// a flush allocates nothing at all. That is not a micro-optimisation: measured +/// on the pinned host, staging the updates and re-growing the pending buffer +/// once per batch cost **+31 MB peak RSS** on json_pipeline 500k (63 flushes, +/// each re-growing a ~1 MB `Vec` from empty and freeing a ~1 MB staging `Vec`), +/// which is a regression the deferral does not need to pay. +#[cold] +fn flush_deferred_old_page_registrations_batch() { + let mut pending = + DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| std::mem::take(&mut *buf.borrow_mut())); + if pending.is_empty() { + return; + } + OLD_GEN_PAGE_OBJECTS.with(|index| { + let mut index = index.borrow_mut(); + OLD_GEN_PAGE_META.with(|meta| { + let mut meta = meta.borrow_mut(); + // Entries arrive in allocation order, so consecutive ones share a + // page; cache that page's pre-batch length across the run. + let mut run_page: Option = None; + let mut run_base_len: usize = 0; + for &(header_addr, total_size) in &pending { + let object_end = header_addr + total_size; + let first_page = generation_page_for_addr(header_addr); + let last_page = generation_page_for_addr(object_end - 1); + for page in first_page..=last_page { + let page_base = generation_page_base(page); + let page_end = page_base + GENERATION_PAGE_SIZE; + let overlap_start = header_addr.max(page_base); + let overlap_end = object_end.min(page_end); + if overlap_start >= overlap_end { + continue; + } + let headers = index.entry(page).or_insert_with(Vec::new); + if run_page != Some(page) { + run_page = Some(page); + run_base_len = headers.len(); + } + if headers[..run_base_len.min(headers.len())].contains(&header_addr) { + continue; + } + headers.push(header_addr); + // Identical to `update_old_page_meta_for_object(.., true)` + // for this one page, applied here so no staging Vec exists. + let page_meta = meta + .entry(page) + .or_insert_with(|| OldPageMeta::zero_for_page(page)); + page_meta.allocated_bytes = page_meta + .allocated_bytes + .saturating_add(overlap_end - overlap_start); + page_meta.object_count = page_meta.object_count.saturating_add(1); + page_meta.refresh_policy_bits(); + } + } + }); + }); + // Hand the allocation back rather than dropping it, so the next burst + // refills a buffer that is already 64k entries wide. + pending.clear(); + DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| { + let mut buf = buf.borrow_mut(); + if buf.capacity() < pending.capacity() { + *buf = pending; + } + }); +} + +/// Entries awaiting a flush. Tests only — the buffer is an implementation +/// detail everywhere else. +#[cfg(test)] +pub(crate) fn deferred_old_page_registrations_len() -> usize { + DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| buf.borrow().len()) +} + #[allow(dead_code)] pub(crate) fn unregister_old_object_pages(header_addr: usize, total_size: usize) { if header_addr == 0 || total_size == 0 { return; } + // #7624 REMOVER: see `unregister_old_block_pages`. Removing before the + // flush would leave the flush to resurrect this object. + flush_deferred_old_page_registrations(); let overlaps = old_object_page_overlaps(header_addr, total_size); let mut removed_pages = Vec::with_capacity(overlaps.len()); OLD_GEN_PAGE_OBJECTS.with(|index| { @@ -683,6 +870,11 @@ pub(crate) fn unregister_old_object_pages(header_addr: usize, total_size: usize) } pub(crate) fn old_pages_begin_gc_cycle() { + // #7624 CYCLE START: all three cycle constructors route through here + // (`gc/mod.rs`'s minor, `gc/cycle.rs`'s `new_full`, `gc/policy.rs`'s + // budgeted), so every collection begins with a complete page-objects index + // and an EMPTY deferral buffer. + flush_deferred_old_page_registrations(); // #6181: the per-page `dirty_slots` reset used to iterate every old page // here (O(old pages) on every minor, growing with old-gen size). It is now // a single epoch bump — a page whose `dirty_slots_epoch` predates the new @@ -694,6 +886,12 @@ pub(crate) fn old_pages_begin_gc_cycle() { } pub(crate) fn old_pages_reset_sweep_accounting() { + // #7624 READER (`OLD_GEN_PAGE_META`): closes the promote → sweep window + // inside a full cycle. The per-object accounting that follows calls + // `refresh_policy_bits`, which reads `allocated_bytes`; flushing here means + // it never recomputes a page's bits from a count that is missing this + // cycle's promotions. + flush_deferred_old_page_registrations(); OLD_GEN_PAGE_META.with(|meta| { for page_meta in meta.borrow_mut().values_mut() { page_meta.reset_cycle_sweep_accounting(); @@ -787,6 +985,10 @@ pub(crate) fn old_page_account_dirty_slot(slot_addr: usize) { } pub(crate) fn old_page_summary() -> OldPageSummary { + // #7624 READER (`OLD_GEN_PAGE_META`): a deferred registration also owes + // this table an `allocated_bytes`/`object_count` update, so the summary + // would under-report a mid-cycle promotion burst without the flush. + flush_deferred_old_page_registrations(); let current_epoch = old_gen_page_dirty_epoch(); OLD_GEN_PAGE_META.with(|meta| { let meta = meta.borrow(); @@ -831,6 +1033,9 @@ pub(crate) fn old_page_summary() -> OldPageSummary { } pub(crate) fn old_page_meta_snapshot() -> Vec { + // #7624 READER (`OLD_GEN_PAGE_META`): this one drives real policy — + // `gc/oldgen_defrag.rs` selects evacuation pages from it. + flush_deferred_old_page_registrations(); let current_epoch = old_gen_page_dirty_epoch(); OLD_GEN_PAGE_META.with(|meta| { let mut snapshot = meta @@ -893,6 +1098,11 @@ pub(crate) fn old_arena_walk_objects_on_pages( return 0; } + // #7624 READER: promotions land in old-gen mid-cycle (a copying minor's + // root scan runs before the remembered-set walk), so this cannot rely on + // the cycle-start flush alone. + flush_deferred_old_page_registrations(); + let mut headers = Vec::new(); let mut seen = crate::fast_hash::new_ptr_hash_set(); OLD_GEN_PAGE_OBJECTS.with(|index| { @@ -923,6 +1133,12 @@ pub(crate) struct OldArenaPageObjectCursor { impl OldArenaPageObjectCursor { pub(crate) fn new(pages: &crate::fast_hash::PtrHashSet) -> Self { + // #7624 READER: same obligation as `old_arena_walk_objects_on_pages`. + // The cursor is stepped incrementally by the budgeted cycle, which + // marks but never allocates into old-gen, so nothing can accumulate + // between `new` and the last `next`; `next` debug-asserts that rather + // than paying a thread-local check per object. + flush_deferred_old_page_registrations(); Self { pages: pages.iter().copied().collect(), page_cursor: 0, @@ -931,6 +1147,14 @@ impl OldArenaPageObjectCursor { } pub(crate) fn next(&mut self) -> Option { + // #7624: `new` flushed; the stepping window must not re-dirty the + // buffer, or this reader would be walking a stale index. Debug-only so + // the per-object read costs nothing in a shipped collector. + debug_assert!( + DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| buf.borrow().is_empty()), + "an old-gen birth happened while a page-object cursor was stepping; \ + this reader is now walking a stale index (#7624)" + ); loop { let page = *self.pages.get(self.page_cursor)?; let header = OLD_GEN_PAGE_OBJECTS.with(|index| { @@ -957,6 +1181,8 @@ pub(crate) fn old_arena_page_index_remove_object(header_addr: usize, total_size: if header_addr == 0 || total_size == 0 { return; } + // #7624 REMOVER: see `unregister_old_block_pages`. + flush_deferred_old_page_registrations(); let overlaps = old_object_page_overlaps(header_addr, total_size); if overlaps.is_empty() { return; @@ -1008,11 +1234,18 @@ pub(crate) fn old_arena_page_index_clear_for_tests() { // Wiping page metadata makes real old-arena objects unclassifiable — // stand the #6179 differential verifier down for this thread's test. crate::gc::CLASSIFIER_VERIFY_SUPPRESSED.with(|c| c.set(true)); + // #7624: DISCARD rather than flush — a caller asking for an empty index + // would get a repopulated one if the pending burst were folded in first. + DEFERRED_OLD_PAGE_REGISTRATIONS.with(|buf| buf.borrow_mut().clear()); OLD_GEN_PAGE_OBJECTS.with(|index| index.borrow_mut().clear()); } #[cfg(test)] pub(crate) fn old_page_meta_for_tests(page: usize) -> Option { + // #7624 READER: same rule as `old_page_summary`/`old_page_meta_snapshot`, + // so a test that allocates and then inspects a page sees what eager + // registration would have left. + flush_deferred_old_page_registrations(); let current_epoch = old_gen_page_dirty_epoch(); OLD_GEN_PAGE_META.with(|meta| { meta.borrow() diff --git a/crates/perry-runtime/src/arena/tests.rs b/crates/perry-runtime/src/arena/tests.rs index d68af2088e..acc46fa762 100644 --- a/crates/perry-runtime/src/arena/tests.rs +++ b/crates/perry-runtime/src/arena/tests.rs @@ -1168,3 +1168,426 @@ fn block_pool_is_per_thread_and_drops_with_its_thread() { // The other thread's pool never touched ours. assert_eq!(block_pool_bytes_for_test(), before); } + +// --------------------------------------------------------------------------- +// #7624: deferred old-object page registration. +// +// `arena_alloc_gc_old` records its page registration in a thread-local buffer +// instead of folding it into `OLD_GEN_PAGE_OBJECTS`/`OLD_GEN_PAGE_META` on the +// spot. The deferral is only invisible if EVERY reader and EVERY remover of +// those two tables flushes first, so that is what these pin — one test per +// obligation, each written so that deleting the corresponding +// `flush_deferred_old_page_registrations()` call turns it red. +// --------------------------------------------------------------------------- + +/// The rule this whole family enforces, made checkable rather than remembered. +/// +/// The per-obligation tests below each pin ONE flush site, which is the right +/// shape for the sites that exist today — but they are blind to a site that +/// does not exist yet. A future edit that adds a function touching either table +/// gets no test, and the deferral silently starts being visible to it. This +/// closes that: both tables are thread-locals private to `page_meta.rs`, so the +/// toucher set is enumerable from the source, and every toucher must either +/// flush or appear below with a reason. +/// +/// A name in `EXEMPT` that no longer touches either table also fails, so a +/// removed function cannot leave a stale exemption behind (the shape +/// `gc_root_dominance_allowlist.json` uses). +#[test] +fn deferred_registration_flush_sites() { + // Every exemption is a claim about why the deferral cannot be observed. + const EXEMPT: &[(&str, &str)] = &[ + ( + "register_old_block_pages", + "creates zeroed per-page META entries when a BLOCK is registered; \ + reads no counter the deferral owes", + ), + ( + "update_old_page_meta_for_object", + "the flush's own target — it is what applies the batch", + ), + ( + "register_old_object_pages", + "the eager path itself; the flush calls its logic, and \ + arena_alloc_gc_old_excluding_pages still calls it directly", + ), + ( + "old_page_account_swept_object", + "per-object sweep writer. It calls refresh_policy_bits, which reads \ + allocated_bytes, but the flush refreshes every page it touches and \ + every READER flushes first, so no reader can observe a stale bit. \ + Kept flush-free so the sweep path pays nothing", + ), + ( + "old_page_account_promoted_object", + "as old_page_account_swept_object — per-object, same argument", + ), + ( + "old_page_account_dirty_slot", + "touches only dirty_slots/epoch, which no registration contributes to", + ), + ( + "old_page_mark_dirty", + "per-store barrier path; asks only whether a META entry exists, and \ + entries are created per page at BLOCK registration, not per object", + ), + ( + "old_page_clear_dirty", + "as old_page_mark_dirty — the dirty bit only", + ), + ( + "next", + "OldArenaPageObjectCursor::next. `new` flushes and the budgeted \ + stepping window marks without allocating into old-gen, so the \ + buffer cannot re-fill mid-walk; `next` debug-asserts exactly that \ + rather than paying a thread-local read per object", + ), + ( + "old_arena_page_index_clear_for_tests", + "DISCARDS the buffer instead: a caller asking for an empty index \ + must not get a repopulated one", + ), + ("defer_old_object_page_registration", "the producer"), + ( + "flush_deferred_old_page_registrations", + "the flush entry point", + ), + ( + "flush_deferred_old_page_registrations_batch", + "the flush body", + ), + ( + "deferred_old_page_registrations_len", + "test-only observer of the buffer, not of either table", + ), + ]; + + let src = std::fs::read_to_string( + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/arena/page_meta.rs"), + ) + .expect("page_meta.rs must be readable"); + + // Split into function bodies by tracking `fn ` headers at any indent. + let mut current: Option = None; + let mut bodies: Vec<(String, String)> = Vec::new(); + for line in src.lines() { + let trimmed = line.trim_start(); + if let Some(rest) = trimmed + .strip_prefix("pub(crate) fn ") + .or_else(|| trimmed.strip_prefix("pub fn ")) + .or_else(|| trimmed.strip_prefix("fn ")) + { + let name: String = rest + .chars() + .take_while(|c| c.is_alphanumeric() || *c == '_') + .collect(); + current = Some(name.clone()); + bodies.push((name, String::new())); + } + if current.is_some() { + if let Some(last) = bodies.last_mut() { + last.1.push_str(line); + last.1.push('\n'); + } + } + } + + let touches = |body: &str| { + body.contains("OLD_GEN_PAGE_OBJECTS.with") || body.contains("OLD_GEN_PAGE_META.with") + }; + let exempt_names: Vec<&str> = EXEMPT.iter().map(|(n, _)| *n).collect(); + + let mut offenders = Vec::new(); + let mut touching = std::collections::BTreeSet::new(); + for (name, body) in &bodies { + if !touches(body) { + continue; + } + touching.insert(name.clone()); + if body.contains("flush_deferred_old_page_registrations()") { + continue; + } + if exempt_names.contains(&name.as_str()) { + continue; + } + offenders.push(name.clone()); + } + + assert!( + offenders.is_empty(), + "these functions in arena/page_meta.rs read or mutate OLD_GEN_PAGE_OBJECTS / \ + OLD_GEN_PAGE_META without first calling flush_deferred_old_page_registrations(), \ + and are not listed as exempt: {offenders:?}.\n\ + A deferred registration is invisible to a reader that does not flush, and a \ + REMOVER that does not flush is worse — the removal no-ops and the later flush \ + resurrects the dead entry. Add the flush, or add the function to EXEMPT with \ + the argument for why the deferral cannot be observed there (#7624)." + ); + + // Stale exemptions fail too, so this list cannot rot into suppression. + let stale: Vec<&str> = exempt_names + .iter() + .copied() + .filter(|n| { + !touching.contains(*n) + && !matches!( + *n, + "defer_old_object_page_registration" + | "flush_deferred_old_page_registrations" + | "flush_deferred_old_page_registrations_batch" + | "deferred_old_page_registrations_len" + | "old_arena_page_index_clear_for_tests" + ) + }) + .collect(); + assert!( + stale.is_empty(), + "EXEMPT names nothing that touches either table any more: {stale:?}. \ + Delete the entry (#7624)." + ); + + // And the gate must be looking at something. + assert!( + touching.len() >= 10, + "only found {} functions touching the page tables — the parser above has \ + probably stopped matching, which would make this gate vacuous", + touching.len() + ); +} + +/// A synthetic old-gen block plus `count` distinct in-range header addresses. +/// Registration never dereferences a header, so fabricated addresses exercise +/// the bookkeeping exactly as real ones do — and keep the test independent of +/// how many objects an allocator happens to fit in a page. +fn synthetic_old_headers(count: usize) -> Vec { + let (base, min_size) = synthetic_old_block_range(); + let size = (count * 64) + .next_multiple_of(GENERATION_PAGE_SIZE) + .max(min_size); + register_block_space(base, size, HeapGeneration::Old, HeapSpace::Old); + (0..count).map(|i| base + i * 64).collect() +} + +fn page_object_count(page: usize) -> usize { + old_page_meta_for_tests(page) + .map(|meta| meta.object_count) + .unwrap_or(0) +} + +#[test] +fn old_gen_birth_defers_its_page_registration() { + run_with_fresh_arenas(|| { + assert_eq!(deferred_old_page_registrations_len(), 0); + let _old_ptr = arena_alloc_gc_old(40, 8, GC_TYPE_STRING) as usize; + assert!( + deferred_old_page_registrations_len() > 0, + "arena_alloc_gc_old must defer, not register eagerly — otherwise \ + the change is inert and every measurement of it is vacuous" + ); + }); +} + +#[test] +fn cycle_start_flushes_deferred_registrations() { + run_with_fresh_arenas(|| { + let old_ptr = arena_alloc_gc_old(40, 8, GC_TYPE_STRING) as usize; + let (header_addr, total_size) = old_header_and_size(old_ptr); + assert!(deferred_old_page_registrations_len() > 0); + + // The single flush point all three cycle constructors route through. + old_pages_begin_gc_cycle(); + + assert_eq!( + deferred_old_page_registrations_len(), + 0, + "old_pages_begin_gc_cycle must leave the deferral buffer empty" + ); + let mut pages = crate::fast_hash::new_ptr_hash_set(); + for (page, _) in old_object_page_overlaps(header_addr, total_size) { + pages.insert(page); + } + let mut visited = Vec::new(); + old_arena_walk_objects_on_pages(&pages, |header| visited.push(header as usize)); + assert_seen_headers("post-cycle-start walk", &visited, &[header_addr]); + }); +} + +/// The other half of the cycle-constructor claim. `cycle_start_flushes_...` +/// proves `old_pages_begin_gc_cycle` flushes; this proves each of the three +/// constructors actually calls it, which is what makes "every collection begins +/// with a complete index" true rather than merely asserted in a comment. +#[test] +fn every_cycle_constructor_routes_through_the_flush_point() { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/gc"); + for (file, what) in [ + ("mod.rs", "non-moving / copying minor"), + ("cycle.rs", "full mark-sweep (GcCycleState::new_full)"), + ("policy.rs", "budgeted minor"), + ] { + let src = std::fs::read_to_string(root.join(file)) + .unwrap_or_else(|e| panic!("cannot read gc/{file}: {e}")); + assert!( + src.contains("old_pages_begin_gc_cycle()"), + "the {what} constructor in gc/{file} no longer calls \ + old_pages_begin_gc_cycle(); deferred old-page registrations would \ + survive into the cycle unflushed (#7624)" + ); + } +} + +#[test] +fn deferral_buffer_flushes_at_its_size_cap() { + run_with_fresh_arenas(|| { + let headers = synthetic_old_headers(DEFERRED_OLD_PAGE_REGISTRATION_CAP); + for &header in &headers { + defer_old_object_page_registration(header, 64); + } + assert_eq!( + deferred_old_page_registrations_len(), + 0, + "the buffer must self-flush at DEFERRED_OLD_PAGE_REGISTRATION_CAP \ + so it cannot grow without bound between collections" + ); + // And the cap flush is a real registration, not a discard. + assert!(page_object_count(generation_page_for_addr(headers[0])) > 0); + }); +} + +/// Each reader of the two tables, one obligation per assertion. Delete any one +/// `flush_deferred_old_page_registrations()` in `page_meta.rs` and exactly one +/// of these goes red. +#[test] +fn every_index_reader_flushes_before_reading() { + run_with_fresh_arenas(|| { + let headers = synthetic_old_headers(4); + let page = generation_page_for_addr(headers[0]); + let mut pages = crate::fast_hash::new_ptr_hash_set(); + pages.insert(page); + + // 1. old_arena_walk_objects_on_pages + defer_old_object_page_registration(headers[0], 64); + let mut visited = Vec::new(); + old_arena_walk_objects_on_pages(&pages, |h| visited.push(h as usize)); + assert_seen_headers("old_arena_walk_objects_on_pages", &visited, &[headers[0]]); + assert_eq!(deferred_old_page_registrations_len(), 0); + + // 2. OldArenaPageObjectCursor — same index, incremental reader. + defer_old_object_page_registration(headers[1], 64); + let mut cursor = OldArenaPageObjectCursor::new(&pages); + assert_eq!( + deferred_old_page_registrations_len(), + 0, + "OldArenaPageObjectCursor::new must flush before it starts stepping" + ); + let mut seen = Vec::new(); + while let Some(h) = cursor.next() { + seen.push(h); + } + assert_seen_headers("OldArenaPageObjectCursor", &seen, &headers[..2]); + + // 3. old_page_summary (OLD_GEN_PAGE_META) + let before = old_page_summary().object_count; + defer_old_object_page_registration(headers[2], 64); + assert_eq!( + old_page_summary().object_count, + before + 1, + "old_page_summary must flush; a mid-cycle promotion burst would \ + otherwise be missing from allocated_bytes/object_count" + ); + + // 4. old_page_meta_snapshot — drives defrag page selection. + defer_old_object_page_registration(headers[3], 64); + let snapshot = old_page_meta_snapshot(); + assert_eq!(deferred_old_page_registrations_len(), 0); + let page_base = generation_page_base(page); + let meta = snapshot + .iter() + .find(|m| m.page_base == page_base) + .expect("snapshot should carry the page"); + assert_eq!(meta.object_count, 4); + }); +} + +/// The remover obligation, and the one that is easiest to get wrong: a removal +/// that runs while the object is still only DEFERRED is a no-op, and the later +/// flush then puts the dead object back. Registration ORDER, not just eventual +/// visibility, is what the flush-before-remove rule buys. +#[test] +fn removing_a_deferred_object_does_not_resurrect_it() { + run_with_fresh_arenas(|| { + let headers = synthetic_old_headers(3); + let mut pages = crate::fast_hash::new_ptr_hash_set(); + pages.insert(generation_page_for_addr(headers[0])); + + let visited_now = |pages: &crate::fast_hash::PtrHashSet| { + let mut visited = Vec::new(); + old_arena_walk_objects_on_pages(pages, |h| visited.push(h as usize)); + visited + }; + + // 1. unregister_old_object_pages + defer_old_object_page_registration(headers[0], 64); + unregister_old_object_pages(headers[0], 64); + assert!( + !visited_now(&pages).contains(&headers[0]), + "a deferred entry removed before its flush was resurrected by the \ + flush — unregister_old_object_pages must flush first (#7624)" + ); + + // 2. old_arena_page_index_remove_object + defer_old_object_page_registration(headers[1], 64); + old_arena_page_index_remove_object(headers[1], 64); + assert!( + !visited_now(&pages).contains(&headers[1]), + "old_arena_page_index_remove_object must flush first (#7624)" + ); + + // 3. unregister_old_block_pages — the whole page goes away, and a + // later flush must not recreate it pointing into a recycled block. + defer_old_object_page_registration(headers[2], 64); + unregister_old_block_pages(&[generation_page_for_addr(headers[2])]); + assert!( + !visited_now(&pages).contains(&headers[2]), + "unregister_old_block_pages must flush first (#7624)" + ); + }); +} + +/// The batched flush skips the dedup scan over entries added within the same +/// batch. That is only sound if it still catches the case the dedup exists for: +/// hole reuse handing back an address registered BEFORE the batch. +#[test] +fn batched_flush_matches_eager_registration() { + run_with_fresh_arenas(|| { + let headers = synthetic_old_headers(64); + let page = generation_page_for_addr(headers[0]); + + // A pre-existing (pre-batch) registration, as hole reuse would leave. + register_old_object_pages(headers[0], 64); + assert_eq!(page_object_count(page), 1); + + // Now defer the whole set INCLUDING the already-registered address. + for &header in &headers { + defer_old_object_page_registration(header, 64); + } + let mut pages = crate::fast_hash::new_ptr_hash_set(); + pages.insert(page); + let mut visited = Vec::new(); + old_arena_walk_objects_on_pages(&pages, |h| visited.push(h as usize)); + + visited.sort_unstable(); + let mut expected = headers.clone(); + expected.sort_unstable(); + assert_eq!( + visited, expected, + "batched flush must produce exactly the eager index — no duplicate \ + for the re-registered address, no dropped entry" + ); + assert_eq!( + page_object_count(page), + headers.len(), + "page object_count must match the eager path's, counting the \ + re-registered address exactly once" + ); + }); +}