Skip to content

perf(gc): batch dead old-object page unregistration per sweep step - #10147

Closed
proggeramlug wants to merge 1 commit into
mainfrom
gc/sweep-batch-old-unregister
Closed

perf(gc): batch dead old-object page unregistration per sweep step#10147
proggeramlug wants to merge 1 commit into
mainfrom
gc/sweep-batch-old-unregister

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

A full collection unregistered every dead old object from the page index one at a time, and that removal is quadratic in objects per page.

The cost

For each dead old object, invalidate_dead_old_arena_headerunregister_old_object_pages did:

  • two Vec allocations (page overlaps, removed_pages)
  • a deferral-buffer flush and a promoted-run materialization
  • both RefCell table borrows and HashMap lookups
  • a linear position through the page's object list before swap_remove
  • a separate page-metadata update

A full collection frees whole pages of small objects, so removing each page's objects one by one is O(k²) in objects per page. Profiled on records_array_8m:scan (~720k dead records per full), invalidate_dead_old_arena_header was 15.8% of all samples, the single largest cost of a full collection.

The registration side already batches for exactly this reason (flush_deferred_old_page_registrations_batch, #7624). This is its mirror.

The change

ArenaSweepObjectsState still invalidates each dead header's fields immediately, so no walker can read it as live. It queues the page-index removal and flushes:

  • once at the end of every sweep step (the queue is empty at every step boundary, so no stale entry is ever visible to the mutator or a minor), and
  • every 4096 headers, so an unbudgeted sweep never stages an unbounded buffer.

unregister_old_objects_batch does one deferral flush, one run materialization per touched page, one retain per page against that page's sorted dead headers (binary search), and one page-meta update per page. Allocated bytes and object counts only fall during removal, so applying a page's decrements together and then resetting/refreshing once reaches the same state as the per-object path. reset_cycle_sweep_accounting and refresh_policy_bits are pure recomputes of the page's own fields. Its scratch buffer is caller-owned and reused, so a warm flush allocates nothing (the #7624 lesson about per-batch staging buffers and peak RSS).

Nothing inside a sweep step reads page-index membership.

Results

Four fulls on records_array_8m:scan: 241 ms → 187 ms (−22%). The batched flush is 6.3% of samples where the per-object remover was 15.8%.

Validation

  • cargo test --release -p perry-runtime --lib: 3698 passed, 0 failed (twice) on this branch's base.
  • New arena::tests_batch_unregister: drives the per-object and batched removers over the same population in one arena (page-spanning objects, a fully emptied page, partial pages) and requires identical page membership and metadata. Sabotage-tested: dropping the object-count decrement fails it with page … metadata diverged.
  • every_page_object_reader_expands_promoted_runs covers the new remover. It touches OLD_GEN_PAGE_OBJECTS and expands runs first, on substance, not just on the pattern the test greps for.
  • Seeded GC stress (PERRY_GC_SCHEDULE_SEED 11–14, rate 0.1, PERRY_GC_PROTECT_FROMSPACE=1, depth 32) over a workload that promotes 40k records per round, frees half, forces fulls and verifies every survivor: 0 bad values, no faults across ~9,100 copying minors per seed. It was confirmed to exercise the path: 30 fulls with reclaim_dead_old_blocks, 582 MB freed, 325 MB of old holes made reusable.
  • GC gap tests (--filter test_gap_gc, Node 26.5.1 oracle): 50/51. The one failure, test_gap_gc_http2_pending_event_callback_rooting, is a 10 s timeout that reproduces identically on the pristine merge base 5603d63d17 (2/2), so it's pre-existing on this host, not caused here.
  • Gates: fmt, file-size, raw-handle ratchet, address-class audit, runtime root holders, store-site inventory, GC env-knob drift — all clean. No env knob added.

Before merging

This changes the full sweep, so the gc-ratchet corpus should run before/after. Draft until it has.

A full collection unregistered each dead old object from the page index on its
own: two `Vec` allocations, a deferral-buffer flush, a promoted-run
materialization, both table borrows, and a linear `position` over the page's
object list before `swap_remove`. Freeing every object on a page that way is
quadratic in objects per page, and a full frees whole pages of small objects.
On records_array_8m:scan (~720k dead records per full)
`invalidate_dead_old_arena_header` was 15.8% of ALL samples -- the single
largest cost of a full collection.

The registration side already batches for exactly this reason
(`flush_deferred_old_page_registrations_batch`, #7624). This is its mirror:
`ArenaSweepObjectsState` still invalidates each dead header's fields
immediately, so no walker can read it as live, but queues the page-index
removal and flushes once per sweep step (and every 4096 headers, so an
unbudgeted sweep never stages an unbounded buffer). The batch does one flush,
one run materialization per touched page, one `retain` per page against that
page's sorted dead headers, and one page-meta update per page -- allocated
bytes and object counts only fall here, so applying a page's decrements
together and resetting/refreshing once is the same state the per-object path
reaches. Nothing inside a sweep step reads page-index membership, and the
queue is empty at every step boundary.

Four fulls on records_array_8m:scan: 241 ms -> 187 ms (-22%). The batched
flush is 6.3% of samples where the per-object remover was 15.8%.

`every_page_object_reader_expands_promoted_runs` covers the new remover (it
touches OLD_GEN_PAGE_OBJECTS and expands first). A new test drives both
removers over the same population in one arena -- page-spanning objects, a
fully emptied page, partial pages -- and requires identical page membership
and metadata; it was sabotage-tested (dropping the object-count decrement
fails it with "page metadata diverged").
@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 95fdbfe1-466f-43a4-a4f7-3ba66e35acab

📥 Commits

Reviewing files that changed from the base of the PR and between 5603d63 and 7f55a7d.

📒 Files selected for processing (6)
  • crates/perry-runtime/src/arena/mod.rs
  • crates/perry-runtime/src/arena/page_meta/mod.rs
  • crates/perry-runtime/src/arena/tests.rs
  • crates/perry-runtime/src/arena/tests_batch_unregister.rs
  • crates/perry-runtime/src/gc/oldgen.rs
  • crates/perry-runtime/src/gc/oldgen/sweep_batch.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The arena now unregisters dead old-generation objects in batches. Old-generation sweeping queues dead headers and flushes them at step boundaries or after 4096 entries. Tests compare batched and per-object page metadata and membership results.

Changes

Old-generation batch unregister

Layer / File(s) Summary
Batch unregister API and validation
crates/perry-runtime/src/arena/page_meta/mod.rs, crates/perry-runtime/src/arena/mod.rs, crates/perry-runtime/src/arena/tests.rs, crates/perry-runtime/src/arena/tests_batch_unregister.rs
The arena adds unregister_old_objects_batch, which groups removals by page, updates page metadata, and refreshes policy state. Tests compare batched removal with per-object removal for mixed object layouts.
Sweep batching integration
crates/perry-runtime/src/gc/oldgen.rs, crates/perry-runtime/src/gc/oldgen/sweep_batch.rs
Old-generation sweeping queues dead headers and flushes pending unregisters at step boundaries or after 4096 entries. Both dead-object paths use the deferred queue.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant ArenaSweepObjectsState
  participant PendingOldUnregister
  participant unregister_old_objects_batch
  participant PageIndex
  participant PageMetadata
  ArenaSweepObjectsState->>PendingOldUnregister: defer dead header and size
  ArenaSweepObjectsState->>PendingOldUnregister: flush at step boundary
  PendingOldUnregister->>unregister_old_objects_batch: submit queued removals
  unregister_old_objects_batch->>PageIndex: remove dead headers by page
  unregister_old_objects_batch->>PageMetadata: update bytes and object counts
Loading

Merge Risk: ⚪ Minimal · up to 7f55a

The batched sweep change has no remaining actionable correctness risk from the reviewed behavior and is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: batching dead old-object page unregistration during GC sweeps.
Description check ✅ Passed The description is detailed and covers the change, motivation, performance results, validation, known baseline failures, and merge dependency. It does not use the template headings or explicitly provi…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gc/sweep-batch-old-unregister

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Validation status for taking this out of draft:

  • CI on the current head matches main's run on the same base: cargo-test fails the same single unrelated test (native_stack::tests::stack_top_respects_custom_thread_stack_sizes), and the gap suite's failing set is identical to main's 10 (empty set difference in both directions). The other red jobs (warnings, lint, check, gc-stress) are the ones red on main.
  • Mergeable against current main (9b911855f8) with no conflicts.
  • gc-ratchet corpus (14 probes × 7 repeats, plain archives) run on the combined branch json/parity-combined (this PR together with perf(gc,codegen): collect dead lazy JSON arrays and stop re-classifying indexed reads #10136, perf(gc): keep wide JSON document storage in the nursery (#10123) #10145, perf(gc): batch dead old-object page unregistration per sweep step #10147, perf(json): parse eagerly when this thread's lazy arrays keep being traversed #10150) against its base fd4bcbe647: every gated counter is bit-identical across all 14 probes (minor_cycles, step_cycles, copied_objects/bytes, promoted_objects/bytes, heap_used_bytes); peak RSS within ±0.6 %; all 14 correctness checks pass on both arms. Wall time is not gated in the shared_ci profile and was uniformly higher in the combined arm (1.06–2.36×, including on probes whose GC counters are identical) — the two arms ran ~25 minutes apart on a shared host whose load varied between 8 and 40 during the day, and I am reporting it rather than attributing it. check --profile shared_ci fails identically for the untouched base build (pre-existing drift of the pinned baseline: 01_nursery_churn heap_used +107 %, 02_survivor_promotion copied +8 %, 04_dead_after_deep_stack copied −26 %), so that red predates these PRs.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train #10188 (rebase-merged; main 5cec2fbbc9, tree identical to the train), cherry-picked onto 6874a9eb73 with the version bump to 0.5.1549. Validation and the CI attribution against main are in #10188.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant