-
-
Notifications
You must be signed in to change notification settings - Fork 161
perf(gc): pre-size the per-minor dirty-scan covered set instead of rebuilding it from empty #9835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2e99865
4a58e85
9606554
1e6fa10
4d23fac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| **The GC trigger path and the dirty-page barrier stop paying `_tlv_get_addr` | ||
| per read, and the policy gate that let them stop paying it now counts the | ||
| thing it is bounding.** | ||
|
|
||
| `gc_check_trigger` runs on every `gc_malloc`, and its predicate | ||
| (`gc_budgeted_due_trigger`) resolved eleven raw `thread_local!` declarations | ||
| one out-of-line call at a time. Measured with `sample` on the compiled | ||
| claude-code TUI streaming a 3300-char reply (14,578 active main-thread | ||
| samples, callers resolved by an explicit ancestor walk rather than | ||
| nearest-symbol labels): `_tlv_get_addr` was 380 main-thread leaf samples, | ||
| **71 of them with `gc_budgeted_due_trigger` as the immediate caller**, 36 in | ||
| `old_page_account_dirty_slots`, 31 in `scan_dirty_object_slots`, 27 in | ||
| `gc_malloc_header_is_tracked`. `crates/perry-runtime/src/tls_hot.rs` has | ||
| existed to abolish exactly this since #7469; the allocation path's *fields* | ||
| were covered and the trigger path never was. | ||
|
|
||
| Sixty-seven declarations across `gc/policy.rs`, `gc/malloc.rs`, `gc/old_free.rs`, | ||
| `gc/tenuring.rs`, `gc/trace.rs`, `gc/barrier/mod.rs`, `arena/block.rs` and | ||
| `arena/page_meta.rs` move to `crate::perry_thread_local!` — same syntax, same | ||
| `.with()` at every call site, the address served from this thread's hot cache | ||
| instead of a libdyld call. | ||
|
|
||
| **Why they were still cold is a measurement bug in the gate, not an oversight | ||
| anyone could have noticed.** `scripts/check_thread_locals.py` ratchets on the | ||
| number of raw `thread_local!` **blocks** per file, while `thread_local! { … }` | ||
| holds any number of declarations — so `gc/policy.rs` counted as **6** while | ||
| declaring **28**, and adding a `static` to an already-recorded block passed | ||
| the gate silently. Counted in the same unit as the hot side, `main` was **318 | ||
| hot declarations against 339 cold ones** — cold was the majority, reported as | ||
| a 2.6:1 minority. The gate now ratchets on declarations (`385 hot / 272 | ||
| cold`), and `--self-test` grew a seventh direction that fails when a `static` | ||
| is added to a recorded block; restoring the block count makes that case, and | ||
| only that case, fail. | ||
|
|
||
| Three declarations stay deliberately raw and say so at their declaration: | ||
| `ARENA_TOTAL_BYTES`, `BLOCK_POOL` and `BLOCK_POOL_BYTES` are read from | ||
| `Arena::new`, which runs as `tls_hot::fill`'s **first** provider, so a | ||
| `HotKey` there re-enters `fill` — which by design has not yet written the | ||
| `temp_roots` field it gates on — and re-runs `ARENA`'s initializer without | ||
| bound. It is a stack overflow at thread start, not a slow path, and it is the | ||
| first documented instance of the rule that a declaration read from inside a | ||
| `fill` provider cannot use the macro. `gc::tests::tls_fill_reentrancy` is the | ||
| standing guard, and it is sabotage-proved: moving `ARENA_TOTAL_BYTES` alone | ||
| into the neighbouring hot block aborts that test with `fatal runtime error: | ||
| stack overflow`. | ||
|
|
||
| `gc::tests::trigger_path_tls` is the runtime half of the gate: it drives | ||
| `gc_check_trigger` on a fresh thread and asserts every trigger-path | ||
| declaration owns a hot slot and that the path publishes slots at all. | ||
| Reverting any one of them to a raw `thread_local!` removes `slot_index` and | ||
| breaks the build at that declaration's own name. It is a test that can fail | ||
| and did: the first run rejected `GC_DEFERRED_REQUEST` with `index 4294967295`, | ||
| correctly — `defer_gc_request` reads it only while a root lock is held, so it | ||
| is not a fast-path read and never claims a slot. The list is what the fast | ||
| path reads, not what the module declares. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| **The per-minor dirty-scan covered set is pre-sized instead of being rebuilt | ||
| from empty**, removing the hashbrown growth ladder every copying minor walked. | ||
|
|
||
| `dirty_scan_covered` is created with `new_ptr_hash_set()` at the top of every | ||
| copying minor and filled during the dirty-slot scan. Measured with | ||
| `[gc-dirty-covered]` (added here), it reaches **~119,000 entries** on a | ||
| 3300-character claude-code reply — not the ~1,000 the `[gc-restore-coverage]` | ||
| `objects_skipped` figure suggested — so it walked hashbrown's capacity ladder | ||
| (1,792 → 14,336 → 57,344 → 114,688 → 229,376) and paid a | ||
| `RawTable::reserve_rehash` at each boundary, re-hashing and re-copying the whole | ||
| table. `reserve_rehash` was **217 leaf samples, 1.49 % of the turn**, 111 of | ||
| them under `PtrHashSet::insert` and the rest under `run_copied_minor_attempt` | ||
| and `restore_surviving_dirty_coverage`. | ||
|
|
||
| The set is now pre-sized from the previous minor's count, the same treatment and | ||
| the same justification as `PREVIOUS_SURVIVOR_ESTIMATE` immediately above it: the | ||
| count is autocorrelated between adjacent cycles, over-estimating costs only | ||
| untouched reserved bytes, under-estimating falls back to ordinary growth, and | ||
| the estimate shares that constant's cap so one huge cycle cannot make every | ||
| later cycle reserve unboundedly. | ||
|
|
||
| `reserve_rehash` falls **217 → 167 leaf samples (1.49 % → 1.24 % of the turn)**. | ||
| The rig is flat, as expected of a 1.5 % item — 400-character turn CPU 4.05 min | ||
| against 4.14, 3300 17.86 against 17.71 — with settled footprint and peak RSS | ||
| improving at 400 (557 → 457 MB, 604 → 563 MB) and flat at 3300. The ground | ||
| claimed is **work permanently removed**, counted rather than inferred: | ||
| `[gc-dirty-covered]` reports `len`, `capacity` and `presized_to` per minor, so | ||
| the pre-size can be seen tracking rather than assumed to. | ||
|
|
||
| **A high-water estimate was tried and rejected.** It is better on the mechanism | ||
| — under-shoots fall from 57 of 96 minors to 21 of 97 — but reserving the peak on | ||
| every minor cost settled footprint 763 → 1165 MB and peak RSS 974 → 1250 MB at | ||
| 3300 characters for no measurable time difference (167 vs 182 leaf samples, | ||
| inside run-to-run noise). Trading footprint for CPU is rejected, and here it did | ||
| not even buy CPU. The rejection is recorded at the function so the next person | ||
| does not re-derive it. |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -215,6 +215,37 @@ static PREVIOUS_SURVIVOR_ESTIMATE: std::sync::atomic::AtomicUsize = | |||||||||
| /// reserve 100 MB of pointers. | ||||||||||
| const SURVIVOR_ESTIMATE_CAP: usize = 1 << 21; | ||||||||||
|
|
||||||||||
| /// Previous minor's dirty-scan covered-set size, for pre-sizing the next one. | ||||||||||
| /// Capped for the same reason as the survivor estimate: a one-off huge cycle | ||||||||||
| /// must not make every later cycle reserve unboundedly. | ||||||||||
| static PREVIOUS_DIRTY_COVERED_ESTIMATE: std::sync::atomic::AtomicUsize = | ||||||||||
| std::sync::atomic::AtomicUsize::new(0); | ||||||||||
|
|
||||||||||
| pub(super) fn previous_dirty_covered_estimate() -> usize { | ||||||||||
| PREVIOUS_DIRTY_COVERED_ESTIMATE.load(std::sync::atomic::Ordering::Relaxed) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// LAST-VALUE. **Do not "just reserve the peak" — that was tried and it cost | ||||||||||
| /// 400 MB of settled footprint for no time gain.** | ||||||||||
| /// | ||||||||||
| /// LAST-VALUE, and a high-water mark was tried and REJECTED. | ||||||||||
| /// | ||||||||||
| /// `[gc-dirty-covered]` shows this set is far more volatile than the survivor | ||||||||||
| /// count this pattern was copied from: it ramps 1,028 -> ~119,000 over a turn | ||||||||||
| /// and swings between adjacent minors, so a last-value estimate under-shoots on | ||||||||||
| /// 57 of 96 minors. A high-water mark fixes that on the mechanism — under-shoots | ||||||||||
| /// fall to 21 of 97 — and was still rejected: reserving the peak on EVERY minor | ||||||||||
| /// cost settled footprint 763 -> 1165 MB and peak RSS 974 -> 1250 MB at 3300 | ||||||||||
| /// characters, for no measurable time difference (`reserve_rehash` 167 vs 182 | ||||||||||
| /// leaf samples, inside run-to-run noise). Trading footprint for CPU is | ||||||||||
| /// rejected, and here it did not even buy CPU. | ||||||||||
| pub(super) fn note_dirty_covered_for_presizing(count: usize) { | ||||||||||
| PREVIOUS_DIRTY_COVERED_ESTIMATE.store( | ||||||||||
| count.min(SURVIVOR_ESTIMATE_CAP), | ||||||||||
| std::sync::atomic::Ordering::Relaxed, | ||||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| pub(super) fn note_survivor_count_for_presizing(count: usize) { | ||||||||||
| PREVIOUS_SURVIVOR_ESTIMATE.store( | ||||||||||
| count.min(SURVIVOR_ESTIMATE_CAP), | ||||||||||
|
|
@@ -1356,7 +1387,20 @@ pub(super) fn run_copied_minor_attempt( | |||||||||
| let snapshot = remembered_dirty_snapshot(); | ||||||||||
| // #9754: objects whose every slot the dirty scan visited in-body — the | ||||||||||
| // post-cycle coverage restore skips them (see `scan_dirty_object_slots`). | ||||||||||
| let mut dirty_scan_covered = crate::fast_hash::new_ptr_hash_set(); | ||||||||||
| // #9835: this set is rebuilt from EMPTY on every minor and reaches ~1,000 | ||||||||||
| // entries (`[gc-restore-coverage] objects_skipped=1026..1116`), so it walked | ||||||||||
| // hashbrown's growth ladder and paid a `RawTable::reserve_rehash` at each | ||||||||||
| // power-of-two boundary — measured 217 leaf samples in `reserve_rehash` on a | ||||||||||
| // 3300-char claude-code reply (1.5 % of the turn), 111 of them under | ||||||||||
| // `PtrHashSet::insert` and the rest under this function and | ||||||||||
| // `restore_surviving_dirty_coverage`. | ||||||||||
| // | ||||||||||
| // Same treatment, and the same justification, as `PREVIOUS_SURVIVOR_ESTIMATE` | ||||||||||
| // above: the count is strongly autocorrelated between adjacent cycles (it is | ||||||||||
| // the same program in the same phase), over-estimating costs only untouched | ||||||||||
| // reserved bytes, and under-estimating falls back to ordinary growth. | ||||||||||
| let mut dirty_scan_covered = | ||||||||||
| crate::fast_hash::new_ptr_hash_set_with_capacity(previous_dirty_covered_estimate()); | ||||||||||
| if !untraced { | ||||||||||
| let _phase = super::pin::CopyingWalkPhaseGuard::enter("remembered_set"); | ||||||||||
| let remembered_stats = scan_remembered_dirty_slots_copying( | ||||||||||
|
|
@@ -1617,6 +1661,21 @@ pub(super) fn run_copied_minor_attempt( | |||||||||
| if !collector.skip_remembering { | ||||||||||
| restore_surviving_dirty_coverage(&snapshot, &dirty_scan_covered, "copying_minor"); | ||||||||||
| } | ||||||||||
| // The mechanism, counted rather than assumed: with the pre-size working, | ||||||||||
| // `capacity` is already >= `len` on entry and hashbrown never grows the | ||||||||||
| // table, so `reserve_rehash` disappears from this path. A capacity that | ||||||||||
| // keeps climbing across minors would say the estimate is not tracking. | ||||||||||
| if crate::gc::gc_diag_enabled() { | ||||||||||
| eprintln!( | ||||||||||
| "[gc-dirty-covered] len={} capacity={} presized_to={}", | ||||||||||
| dirty_scan_covered.len(), | ||||||||||
| dirty_scan_covered.capacity(), | ||||||||||
| previous_dirty_covered_estimate(), | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| note_dirty_covered_for_presizing(dirty_scan_covered.len()); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win Preserve the last estimate when the dirty scan is skipped. When Proposed fix- note_dirty_covered_for_presizing(dirty_scan_covered.len());
+ if !untraced {
+ note_dirty_covered_for_presizing(dirty_scan_covered.len());
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| { | ||||||||||
| } | ||||||||||
| let malloc_freed_bytes = if malloc_sweep_due { | ||||||||||
| let phase_start = trace_phase_start(trace); | ||||||||||
| let freed = sweep_malloc_objects(); | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Keep
ARENA_TOTAL_BYTESin raw TLS.ARENAis atls_hot::fillprovider. Its initializer callsArena::new, which readsARENA_TOTAL_BYTES. MovingARENA_TOTAL_BYTESintocrate::perry_thread_local!re-enterstls_hot::fillwhiletemp_rootsis unset. A fresh thread will recurse until stack overflow. Keep this declaration in the plainthread_local!block withBLOCK_POOLandBLOCK_POOL_BYTES.🤖 Prompt for AI Agents