Add progress reporting during heap enumeration#5763
Closed
danmoseley wants to merge 3 commits intodotnet:mainfrom
Closed
Add progress reporting during heap enumeration#5763danmoseley wants to merge 3 commits intodotnet:mainfrom
danmoseley wants to merge 3 commits intodotnet:mainfrom
Conversation
Commands like verifyheap and dumpheap -stat walk the entire managed heap but produce zero output during the walk, which on large dumps (e.g., 19 GB, 248M objects) can mean 7+ minutes of silence indistinguishable from a hang. Add a ProgressCallback property to HeapWithFilters that fires periodically (default every 10 seconds) during EnumerateFilteredObjects(), reporting bytes scanned vs. total heap size as a percentage. Enable it in: - VerifyHeapCommand (always, since it never streams output) - DumpHeapCommand (when -stat is used, since output is deferred) Extract timing logic into a ProgressReporter helper class with unit tests. Fixes dotnet#5760 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
One thing not mentioned above -- while a user may know to wait without feedback, the Copilot CLI (at least for me) did not. So this may be more intersting to fix than before. I'm assuming that changing the console output here is not considered a breaking change... |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds periodic progress reporting during managed heap enumeration to avoid long “silent” periods in verifyheap and dumpheap -stat, using a time-gated reporter and a callback hook in the heap enumeration pipeline.
Changes:
- Added
ProgressReporterhelper and integrated it intoHeapWithFilters.EnumerateFilteredObjects()via an optionalProgressCallback. - Enabled progress reporting for
verifyheap(always) anddumpheap -stat(when-statis used). - Added a new unit test project with
ProgressReporterTests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Diagnostics.ExtensionCommands/HeapWithFilters.cs | Adds progress callback plumbing and reporting during object enumeration. |
| src/Microsoft.Diagnostics.ExtensionCommands/ProgressReporter.cs | Introduces time-gated progress reporting + message formatting. |
| src/Microsoft.Diagnostics.ExtensionCommands/VerifyHeapCommand.cs | Enables heap-scan progress output for verifyheap. |
| src/Microsoft.Diagnostics.ExtensionCommands/DumpHeapCommand.cs | Enables heap-scan progress output for dumpheap -stat. |
| src/Microsoft.Diagnostics.ExtensionCommands/Microsoft.Diagnostics.ExtensionCommands.csproj | Grants unit tests access to internals via InternalsVisibleTo. |
| src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/Microsoft.Diagnostics.ExtensionCommands.UnitTests.csproj | Adds new unit test project for progress reporting. |
| src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/ProgressReporterTests.cs | Adds unit tests for reporting behavior and formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...nostics.ExtensionCommands.UnitTests/Microsoft.Diagnostics.ExtensionCommands.UnitTests.csproj
Outdated
Show resolved
Hide resolved
src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/ProgressReporterTests.cs
Outdated
Show resolved
Hide resolved
...nostics.ExtensionCommands.UnitTests/Microsoft.Diagnostics.ExtensionCommands.UnitTests.csproj
Outdated
Show resolved
Hide resolved
…d using - Use $(SupportedXUnitTestTargetFrameworks) instead of hard-coded net8.0 - Use $(XUnitVersion) instead of hard-coded xunit 2.9.3 - Remove unused using System.Threading Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
flaky tests-
|
This was referenced Mar 20, 2026
Member
|
@hoyosjs and I made some edits with copilot which created its own alternate PR: #5773 I expect we'll wind up merging that one rather than this one but I think it still has the progress behavior you care about. Thanks for getting it started @danmoseley 👍 |
Member
Author
|
Sounds good to me, thanks! (Are there any other diagnostic tools that might need such a feature too..) |
hoyosjs
added a commit
that referenced
this pull request
Mar 27, 2026
…s, literal output assertions (#5773) - [x] Address review comment 1: refactor `ProgressCallback` as optional method parameter on `EnumerateFilteredObjects` (remove from `HeapWithFilters` class property) - [x] Address review comment 2: rewrite byte-tracking in `EnumerateFilteredObjects` to use segment position (`obj.Address - segment.Start`, 16KB threshold, segment start/end callbacks) - [x] Refactor `ProgressReporter` to be the timed console writer registered as callback (takes `Action<string>` writeMessage, receives `(scannedBytes, totalBytes)` via `Report` method) - [x] Update `DumpHeapCommand` and `VerifyHeapCommand` to use new API - [x] Update `ProgressReporterTests` to match new `ProgressReporter` API (7/7 tests pass) <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > Modify Dan Moseley's PR #5763 (branch: danmoseley:heap-progress) to do the following: > > 1. Add a flag to opt out of progress reporting for heap enumeration. > 2. Update the unit test cases so that they validate literal output from progress lines, rather than simply inferring correctness using text.Contains("5"). > 3. Refactor progress timing tests to use TimeProvider, so that the test does not rely on sleep() or time delays, and instead deterministically validates timing. > > Requirements: > - Show the full `git diff` of changes before pushing to the PR branch. > - Do not push any changes until the user explicitly approves after seeing the diff. > > The following is the prior conversation context from the user's chat exploration (may be truncated): > > User: Can you push commits to this PR? > > Assistant: **Yes, you should be able to push commits to this PR.** Here's why: > > 1. **`maintainer_can_modify` is `true`** — The PR author (@danmoseley) enabled "Allow edits from maintainers" when creating this PR from their fork (`danmoseley/diagnostics` → `dotnet/diagnostics`). > > 2. **You have write access** — As long as you have push/write permissions to the base repository (`dotnet/diagnostics`), GitHub allows you to push commits directly to the PR's head branch (`heap-progress` on `danmoseley/diagnostics`). > > You can push to the branch by adding the fork as a remote and pushing to the `heap-progress` branch: > > ```bash > git remote add danmoseley https://github.com/danmoseley/diagnostics.git > git push danmoseley HEAD:heap-progress > ``` > > This is documented in GitHub's guide on [Committing changes to a pull request branch created from a fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork). > > User: Do you have permissions to modify this PR? > > Assistant: I'm waiting for your confirmation to proceed with the tool call that was just proposed. > > User: @Copilot Accepted Confirmation: Confirm agent session > > Assistant: I don't have the ability to directly check my own permissions on a repository. However, I've kicked off a task that will attempt to interact with the PR, which will effectively test whether I have write access: > > ````list type="task" > data: > - title: '[WIP] Check push permissions to PR #5763' > repository: dotnet/diagnostics > url: https://github.com/copilot/tasks/pull/PR_kwDOB4aLFs7MAicv?session_id=88a58d80-0bba-4b9d-bf31-fcfdb05e5cf0 > state: queued > > ```` > > That said, to answer your question more directly: **I don't know for certain whether I (Copilot) have write permissions to `dotnet/diagnostics`**. That depends on: > > 1. **Whether GitHub Copilot is installed** on the `dotnet/diagnostics` repository (or the `dotnet` org) with write permissions. > 2. **Whether the org/repo settings** allow Copilot to push to forks via `maintainer_can_modify`. > > If the task above fails with a permissions error, that will confirm I don't have the necessary access. In that case, you (hoyosjs) would need to push the commits yourself, assuming you have write access to `dotnet/diagnostics`. > > User: Modify Dan Moseleys pr to: > > - add a flag to opt out of progress report > -we actually see some literal output in the test cases rather than just infering the whole thing is good from text.Contains("5") > - use TimeProvider to validate the progress part without relying on sleep() in the test (or the current state that doesn't actually validate progress timing) > > Show me the diff before pushing to the PR > > Assistant: I'll prepare a diff that modifies Dan Moseley's PR as you requested—adding a flag to opt out of progress reporting, improving test assertions to check the actual output, and refactoring timing verification to use TimeProvider instead of sleep. > > Once the changes are ready, I will show you the complete diff so you can review it before anything is pushed. > > </details> <!-- START COPILOT CODING AGENT SUFFIX --> *This pull request was created from Copilot chat.* > <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Dan Moseley <danmose@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com> Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add progress reporting during heap enumeration
Commands like
verifyheapanddumpheap -statwalk the entire managed heap but produce zero output during the walk. On large dumps (e.g., 19 GB / 248M objects), this means minutes of complete silence on both stdout and stderr, indistinguishable from a hang.This PR adds periodic progress output (every 10 seconds) during heap enumeration for the two worst offenders:
verifyheap-- 7+ minutes of silence before printing a single linedumpheap -stat-- ~80 seconds of silence before printing the statistics tableExample output
Without the fix, there would be no output at all before the
Statistics:line.Design choices
Output format: Progress lines are written via
Console.WriteLine(), producing one line every ~10 seconds that scrolls up. Two other formats were considered:. . . . .) -- minimal but gives no indication of how far along or how much is left\roverwrite -- cleanest UX in interactive terminals, but produces a single growing line when stdout is redirected to a file, and depends on terminal\rsupportWriteLineis the most portable choice; forverifyheapon this dump it produces ~43 progress lines over 7 minutes, which seems an acceptable trade-off for the information provided.Scope: Only
verifyheap(always) anddumpheap -stat(when-statis used) enable progress. Commands that already stream output continuously (dumpheapwithout-stat,finalizequeue,gchandles) don't need it.gcheapstatanddumpasyncalso have silent walks but use different enumeration paths; they can be addressed in a follow-up.Performance
The progress mechanism adds negligible overhead:
Stopwatch.ElapsedMillisecondsread per valid object (~5ns), compared to the per-object work of reading method tables, sizes, and verifying references (orders of magnitude more expensive)Console.WriteLinecall happens only once every 10 secondsList<ClrSegment>upfront (typically a handful of segments) to avoid double-enumerating the filtered segment listChanges
HeapWithFilters.cs-- AddedProgressCallbackproperty and aProgressIntervalMsconstant. When set,EnumerateFilteredObjects()computes total heap size from segment metadata and periodically invokes the callback.ProgressReporter.cs-- New helper class encapsulating the time-gated progress logic, with a staticFormatProgressMessagemethod.VerifyHeapCommand.cs-- Sets progress callback before heap walk.DumpHeapCommand.cs-- Sets progress callback when-statis used.ProgressReporterTestscovering callback timing, byte tracking, and message formatting.Test data (19.3 GB dump, 248M objects, warm cache)
verifyheapdumpheap -statFixes #5760