Skip to content

fix(gc): subtract live from-space bytes, not the block high-water, in a copied minor (#7901) - #7943

Merged
proggeramlug merged 2 commits into
mainfrom
gc/live-census-7901
Aug 12, 2026
Merged

fix(gc): subtract live from-space bytes, not the block high-water, in a copied minor (#7901)#7943
proggeramlug merged 2 commits into
mainfrom
gc/live-census-7901

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Closes #7901.

A full or non-moving sweep publishes sweep.arena_live_bytes, an object-walk
census that excludes the dead objects left as holes beside a survivor. It
cannot reset those survivors' blocks, so the holes stay inside the block offsets
copying_from_space_in_use_bytes() sums. The next copied minor computed

pre_collection_live_bytes − from_space_high_water + copied + promoted

which removes those holes a second time. saturating_sub then hides the
arithmetic failure: when the young high-water exceeds unrelated old live bytes it
erases old-generation occupancy from heapUsed and from major-GC pacing
altogether, and the error persists into the next census.

Fix

The sweep publishes the live from-space share alongside the total
(SweepTraceStats::arena_live_from_space_bytes, accumulated in
ArenaSweepObjectsState::keep_live_object — the one funnel the
census-publishing sweep routes live objects through, gated on a block-index test
against Eden plus the active survivor). record_arena_live_census stores it with
the from-space high-water at the same instant, and
arena::arena_live_from_space_bytes() derives the current from-space
contribution as census.from_space_live + (high_water_now − census.high_water),
clamped to the current high-water. The copied minor subtracts that.

A copied minor publishes None — "from-space is compacted by construction":
after the flip Eden is empty and the new active survivor holds only the copies,
so live equals high-water and the old and new derivations agree. That is why
copying_minors_preserve_prior_promotions_in_the_live_census is unchanged.

Two debug_asserts guard the subtraction: live from-space bytes cannot exceed
the from-space high-water, nor the whole live census. A high-water subtraction
can no longer silently consume unrelated generations.

Test

heap_accounting::copied_minor_after_a_non_moving_sweep_does_not_subtract_dead_holes_twice
seeds an old live cohort, fills Eden with garbage plus one rooted survivor, runs
a non-moving full sweep, then a copied minor. It asserts

  • the setup is non-degenerate — the from-space high-water must exceed the
    live from-space bytes by at least half a block, or the two formulas coincide
    and a green run would prove nothing;
  • the old high-water derivation would have produced a strictly smaller figure on
    this exact state;
  • the old cohort is still present in heapUsed, and pacing reads the same
    corrected number.

Collateral

gc/cycle.rs and gc/oldgen.rs were both within 6 lines of the 2000-line gate.
Reclaimed by collapsing an allocate-black paragraph duplicated verbatim in
GcCycleState::new_full and new_minor_fallback, and by deleting an 18-line
description in the legacy sweep_arena_objects of a "two-phase probe-then-track"
strategy the code no longer implements — the very next line already contradicted
it.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected memory accounting after cleanup and copying cycles, especially when partially occupied memory blocks contain reclaimed gaps.
    • Improved collection pacing and occupancy reporting by distinguishing live memory from high-water usage.
    • Preserved accurate survivor and older-generation tracking during garbage collection.
  • Tests

    • Added regression coverage verifying accurate accounting and continued validity of retained objects.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

GC live from-space accounting

Layer / File(s) Summary
Census state and accessors
crates/perry-runtime/src/arena/stats.rs, crates/perry-runtime/src/arena/mod.rs
Arena census state stores from-space live bytes and high-water values. Recording and derivation helpers expose the updated accounting.
Sweep from-space census propagation
crates/perry-runtime/src/arena/reset.rs, crates/perry-runtime/src/arena/mod.rs, crates/perry-runtime/src/gc/oldgen.rs, crates/perry-runtime/src/gc/cycle.rs
Sweeps identify copying from-space blocks, count live bytes, propagate the value through sweep statistics, and publish it with the total arena census.
Copied-minor accounting and regression coverage
crates/perry-runtime/src/gc/copying.rs, crates/perry-runtime/src/gc/tests/heap_accounting.rs, changelog.d/7943-live-from-space-census.md
Copied minors subtract pre-collection live from-space bytes instead of high-water usage. Assertions and regression coverage validate survivor, old-generation, and pacing accounting.

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

Sequence Diagram(s)

sequenceDiagram
  participant NonMovingSweep
  participant ArenaCensus
  participant CopiedMinor
  participant HeapAccounting
  NonMovingSweep->>ArenaCensus: record total and live from-space bytes
  CopiedMinor->>ArenaCensus: read live from-space bytes
  CopiedMinor->>HeapAccounting: subtract live from-space and add survivors
  HeapAccounting->>ArenaCensus: record copied-minor census
Loading

Possibly related PRs

  • PerryTS/perry#7886: Added the live arena census and copied-minor accounting extended by this change.
  • PerryTS/perry#7613: Also changed sweep and copying accounting through SweepTraceStats and ArenaSweepObjectsState.
  • PerryTS/perry#7869: Also changed post-collection census publication in publish_reclaim_outcome.

Suggested reviewers: jdalton, thehypnoo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary GC accounting fix.
Description check ✅ Passed The description explains the problem, fix, regression test, and related issue, although it omits the template checklist and screenshots sections.
Linked Issues check ✅ Passed The changes satisfy issue #7901 by tracking live from-space bytes, adding invariants, preserving accounting, and testing the non-moving sweep sequence.
Out of Scope Changes check ✅ Passed The changes remain within the stated GC accounting fix, regression coverage, telemetry separation, and documented code-size reductions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gc/live-census-7901

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 pushed a commit that referenced this pull request Aug 12, 2026
Ralph Küpper added 2 commits August 12, 2026 11:54
… a copied minor

A non-moving sweep publishes an exact aggregate live census that excludes
the dead objects left as holes beside a survivor, but leaves those holes
inside the block offsets copying_from_space_in_use_bytes() sums. The next
copied minor subtracted that high-water from the exact census, charging
the same garbage twice; saturating_sub then hid the negative result by
erasing unrelated old-generation occupancy from heapUsed and major-GC
pacing.

The sweep now publishes the live from-space share alongside the total, the
census stores it with the from-space high-water at the same instant, and
the copied minor subtracts the derived live figure.
@proggeramlug
proggeramlug marked this pull request as ready for review August 12, 2026 10:11

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
changelog.d/7943-live-from-space-census.md (1)

1-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reduce this fragment to one release-note entry.

Keep the shipped behavior: copied minor GC now preserves accurate live-byte accounting after non-moving sweeps.

Remove internal formulas, symbol names, issue history, and unrelated comment-cleanup details. Based on learnings, changelog fragments must describe final shipped behavior as one coherent release-note entry.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@changelog.d/7943-live-from-space-census.md` around lines 1 - 49, Rewrite the
changelog entry as one concise release note focused on the shipped behavior:
copied minor GC now preserves accurate live-byte and heap accounting after
non-moving sweeps. Remove implementation formulas, internal symbol names, issue
references, regression-test details, assertions, and unrelated comment-cleanup
information.

Source: Learnings

🔇 Additional comments (7)
crates/perry-runtime/src/arena/stats.rs (1)

14-20: LGTM!

Also applies to: 31-32, 45-73, 75-103

crates/perry-runtime/src/arena/mod.rs (1)

95-99: LGTM!

Also applies to: 127-127

crates/perry-runtime/src/arena/reset.rs (1)

91-101: LGTM!

crates/perry-runtime/src/gc/oldgen.rs (1)

138-143: LGTM!

Also applies to: 771-786, 1088-1094, 1273-1273, 1339-1341, 1374-1375, 1510-1521

crates/perry-runtime/src/gc/cycle.rs (1)

990-992: LGTM!

Also applies to: 1944-1954

crates/perry-runtime/src/gc/copying.rs (1)

1371-1373: LGTM!

Also applies to: 1789-1792, 1813-1842

crates/perry-runtime/src/gc/tests/heap_accounting.rs (1)

108-216: 🩺 Stability & Availability

⚠️ Unverified finding
Sandbox verification was unavailable.

Run this regression test serially.

Use RUST_TEST_THREADS=1 when validating this test because perry-runtime tests are not parallel-safe.

As per coding guidelines, crates/perry-runtime/**/*.rs: “perry-runtime's tests are not parallel-safe — run them RUST_TEST_THREADS=1.”

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@changelog.d/7943-live-from-space-census.md`:
- Around line 1-49: Rewrite the changelog entry as one concise release note
focused on the shipped behavior: copied minor GC now preserves accurate
live-byte and heap accounting after non-moving sweeps. Remove implementation
formulas, internal symbol names, issue references, regression-test details,
assertions, and unrelated comment-cleanup information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 98c2a7c4-b25f-4daf-a605-e54e29385dbc

📥 Commits

Reviewing files that changed from the base of the PR and between 0809ada and 7a22d3f.

📒 Files selected for processing (8)
  • changelog.d/7943-live-from-space-census.md
  • crates/perry-runtime/src/arena/mod.rs
  • crates/perry-runtime/src/arena/reset.rs
  • crates/perry-runtime/src/arena/stats.rs
  • crates/perry-runtime/src/gc/copying.rs
  • crates/perry-runtime/src/gc/cycle.rs
  • crates/perry-runtime/src/gc/oldgen.rs
  • crates/perry-runtime/src/gc/tests/heap_accounting.rs

@proggeramlug
proggeramlug merged commit e886b56 into main Aug 12, 2026
1 of 19 checks passed
@proggeramlug
proggeramlug deleted the gc/live-census-7901 branch August 12, 2026 10:17
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.

memory(gc): copied minor subtracts from-space high-water from an exact live census

1 participant