Skip to content

fix(gc): report actual restore coverage inputs and slot work - #10237

Closed
proggeramlug wants to merge 2 commits into
mainfrom
codex/9877-restore-coverage-metrics
Closed

proggeramlug wants to merge 2 commits into
mainfrom
codex/9877-restore-coverage-metrics

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #9877.

The restore diagnostic can report three dirty pages while its old-arena walk visits only one: dirty_pages is the union of old and external pages. Report the actual dirty_old_pages input, raw external_entries, and covered skip-set size separately.

Keep the existing candidate/skipped counts and add admitted parent visits, all enumerated slots (including weak targets and primitive values), and strong slots whose children still require tracking. Productivity counts edges, even when the corresponding page was already dirty; pages_added continues to measure newly restored entries. Parent counts are visits, not distinct objects.

A const-generic diagnostics branch removes the new accounting from the diagnostics-off traversal. The existing child-tracking decision supplies the productive-slot result, so children are not decoded twice. Collection policy, slot traversal, and remembered-set repair stay the same. This fixes the diagnostic defect; it makes no performance claim based on the timing interpretation withdrawn in the issue's September 6 update.

Validation:

  • The new isolated-process regression fails before the change (dirty_pages=3 for one old page) and passes afterward. It verifies diagnostics on/off, duplicate stale external owners, covered parents, primitive/old/young slots, and repeated repair: two productive slots in both passes, but one newly dirty page followed by zero.
  • Single-threaded runtime suite: 3,786 passed, 4 ignored.
  • Coherent perry-dev build of the compiler, runtime/stdlib static wrappers, and net archive passes; both runtime archive timestamps postdate the edit.
  • Native method-receiver GC fixture matches Node 26.5.1 under ordinary scheduling. A two-iteration stress reduction matches Node with diagnostics off and on; the enabled run proves 1,206 copying minors, 4,827 moved objects, 451,826 visited slots, and 2,859 tracking slots. The original 400-iteration fixture exceeded its 45-second forced-scheduling limit, so it is not claimed as a stress pass.
  • Node version consistency and git diff --check pass. pre-tag-check.sh --quick passes every check except public benchmark evidence freshness. Its artifact, verifier, and all 66 relevant input paths are unchanged from base d8bfa28a38; the freshness failure is inherited.

No version bump.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected [gc-restore-coverage] diagnostics to accurately report restore-coverage activity.
    • Added counters for visited parents, inspected slots, tracked slots, and coverage input details.
    • Ensured diagnostics remain silent when GC diagnostics are disabled.
    • Preserved existing garbage-collection restore behavior and collection results.
  • Tests

    • Added regression coverage for varied page, ownership, parent, slot, and repair scenarios.
    • Verified diagnostic values match the restore-coverage work performed.

@coderabbitai

coderabbitai Bot commented Sep 13, 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: afbd466a-2b9e-406b-a75a-1e30ed25c73e

📥 Commits

Reviewing files that changed from the base of the PR and between d8bfa28 and 7f18753.

📒 Files selected for processing (4)
  • changelog.d/10237-restore-coverage-metrics.md
  • crates/perry-runtime/src/gc/tests/mod.rs
  • crates/perry-runtime/src/gc/tests/restore_coverage.rs
  • crates/perry-runtime/src/gc/verify.rs

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


📝 Walkthrough

Walkthrough

The GC restore-coverage diagnostic now reports the snapshot inputs and slot-level traversal counters. The repair walk returns whether slots require tracking. New subprocess tests verify enabled, disabled, and repeated-repair behavior.

Changes

Restore coverage diagnostics

Layer / File(s) Summary
Track restore walk counters
crates/perry-runtime/src/gc/verify.rs
remember_evacuated_old_to_young_slot returns a tracking result. The restore walk records visited parents, visited slots, and slots requiring tracking when diagnostics are enabled.
Report corrected diagnostic fields
crates/perry-runtime/src/gc/verify.rs
The diagnostic reports dirty_old_pages, external_entries, covered, traversal counters, and pages_added.
Validate diagnostic and disabled paths
crates/perry-runtime/src/gc/tests/..., changelog.d/10237-restore-coverage.md
The registered subprocess tests validate diagnostic silence, reported counter values, repeated repairs, and unchanged restoration behavior. The changelog documents the corrected metrics.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant TestProcess
  participant restore_surviving_dirty_coverage
  participant GCRepairWalk
  TestProcess->>restore_surviving_dirty_coverage: run with PERRY_GC_DIAG=0 or 1
  restore_surviving_dirty_coverage->>GCRepairWalk: walk dirty pages, external entries, and covered objects
  GCRepairWalk-->>restore_surviving_dirty_coverage: restore pages and collect counters
  restore_surviving_dirty_coverage-->>TestProcess: emit diagnostics when enabled
Loading

Merge Risk: ⚪ Minimal · up to 7f187

The diagnostics and their enabled and disabled paths are covered without an identified behavior regression, so the change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1… 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: correcting GC restore-coverage inputs and reporting slot work.
Description check ✅ Passed The description provides the change summary, linked issue, detailed changes, validation results, known test limitation, and confirms no version bump. It omits the template headings and checklist items…
Linked Issues check ✅ Passed Issue #9877 requires diagnostics for the actual dirty_old_pages walk, separate external_entries and covered counts, parent and slot cost counts, productive tracking-slot counts, and retained `ob…
Out of Scope Changes check ✅ Passed The changed runtime code implements the #9877 diagnostic counters and preserves the repair behavior. The new test constructs varied walk inputs and validates the required diagnostics. The test-module …
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1 unsupported.)

  • 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 codex/9877-restore-coverage-metrics

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

Landed via merge train 186r (#10247) at eb13fa1 on main.

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.

[gc-restore-coverage] reports dirty_pages while the walk iterates dirty_old_pages, and counts objects where the cost is per slot

1 participant