Skip to content

Enhance heap progress PR: -noprogress flag, deterministic timing tests, literal output assertions#5773

Open
Copilot wants to merge 11 commits intomainfrom
copilot/danmoseley-add-heap-progress-flag
Open

Enhance heap progress PR: -noprogress flag, deterministic timing tests, literal output assertions#5773
Copilot wants to merge 11 commits intomainfrom
copilot/danmoseley-add-heap-progress-flag

Conversation

Copy link
Contributor

Copilot AI commented Mar 20, 2026

  • Address review comment 1: refactor ProgressCallback as optional method parameter on EnumerateFilteredObjects (remove from HeapWithFilters class property)
  • Address review comment 2: rewrite byte-tracking in EnumerateFilteredObjects to use segment position (obj.Address - segment.Start, 16KB threshold, segment start/end callbacks)
  • Refactor ProgressReporter to be the timed console writer registered as callback (takes Action<string> writeMessage, receives (scannedBytes, totalBytes) via Report method)
  • Update DumpHeapCommand and VerifyHeapCommand to use new API
  • Update ProgressReporterTests to match new ProgressReporter API (7/7 tests pass)
Original prompt

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/diagnosticsdotnet/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:

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.

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:

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.

This pull request was created from Copilot chat.


💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

danmoseley and others added 4 commits March 20, 2026 03:33
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 #5760

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…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>
…s, literal output assertions

Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>
Copilot AI changed the title [WIP] Add flag to opt out of heap enumeration progress reporting Enhance heap progress PR: -noprogress flag, deterministic timing tests, literal output assertions Mar 20, 2026
Copilot AI requested a review from hoyosjs March 20, 2026 03:43
- Move ProgressCallback from HeapWithFilters class property to optional
  parameter on EnumerateFilteredObjects
- Rewrite byte tracking: use obj.Address - segment.Start (16KB threshold,
  segment start/end callbacks) instead of summing object sizes
- Refactor ProgressReporter to be the timed console writer registered as
  the callback handler (takes Action<string>, exposes Report(long, long))
- Update DumpHeapCommand and VerifyHeapCommand to use new API
- Update ProgressReporterTests for new API (7/7 pass)

Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
…e add

Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
…nal in ProgressReporter

Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
Copilot AI requested a review from noahfalk March 20, 2026 05:44
noahfalk
noahfalk previously approved these changes Mar 20, 2026
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

made some changes, looks good to me now

@noahfalk noahfalk marked this pull request as ready for review March 20, 2026 06:09
@noahfalk noahfalk requested a review from a team as a code owner March 20, 2026 06:09
Copilot AI review requested due to automatic review settings March 20, 2026 06:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances heap-enumeration progress reporting in Microsoft.Diagnostics.ExtensionCommands by making it optional via a new command flag, refactoring enumeration progress plumbing, and strengthening unit test assertions for progress output/timing.

Changes:

  • Add -noprogress option to dumpheap and verifyheap to suppress periodic heap-scan progress output.
  • Refactor HeapWithFilters.EnumerateFilteredObjects to accept an optional progress callback and track progress using segment-relative address advancement plus end-of-segment reporting.
  • Introduce ProgressReporter and add new unit tests validating literal progress output and deterministic timing behavior.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Microsoft.Diagnostics.ExtensionCommands/HeapWithFilters.cs Adds optional progress callback parameter and new byte-progress tracking logic during enumeration.
src/Microsoft.Diagnostics.ExtensionCommands/ProgressReporter.cs New helper that time-gates formatted progress output.
src/Microsoft.Diagnostics.ExtensionCommands/DumpHeapCommand.cs Adds -noprogress and wires progress reporter for dumpheap -stat.
src/Microsoft.Diagnostics.ExtensionCommands/VerifyHeapCommand.cs Adds -noprogress and wires progress reporter for verifyheap.
src/Microsoft.Diagnostics.ExtensionCommands/Microsoft.Diagnostics.ExtensionCommands.csproj Exposes internals to the new unit test assembly.
src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/Microsoft.Diagnostics.ExtensionCommands.UnitTests.csproj New unit test project for progress reporter tests.
src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/ProgressReporterTests.cs New tests with literal output assertions and deterministic timing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@noahfalk
Copy link
Member

@copilot - please address the code review feedback

…re progress tracking

Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/diagnostics/sessions/883cf807-18c2-4166-a1a6-b7053a396eda
Copy link
Contributor Author

Copilot AI commented Mar 21, 2026

@copilot - please address the code review feedback

Done in 045404d:

  • Added using Microsoft.Diagnostics.ExtensionCommands; to ProgressReporterTests.cs
  • Extracted 16 * 1024 to private const int ProgressStepBytes = 16 * 1024 in HeapWithFilters
  • Fixed progress accuracy under MemoryRange filtering: totalBytes is now computed as the sum of overlaps between each segment's CommittedMemory and the filter range (via a new OverlapLength helper), and per-object bytesInSegment is measured from Math.Max(segment.Start, range.Start) so bytes before the range start are not counted

Copilot AI requested a review from noahfalk March 21, 2026 00:45
@noahfalk
Copy link
Member

@copilot - please remove the change you made for memory range filtering. It adds unnecessary complexity and the progress tracking was sufficient for our needs prior to your change.

Copy link
Contributor Author

Copilot AI commented Mar 21, 2026

@copilot - please remove the change you made for memory range filtering. It adds unnecessary complexity and the progress tracking was sufficient for our needs prior to your change.

Done in 944eca0 — removed the OverlapLength helper, effectiveSegStart/effectiveSegLength variables, and the overlap-based totalBytes computation. EnumerateFilteredObjects is back to using segment.Start and segment.CommittedMemory.Length directly, with only the ProgressStepBytes constant kept.

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.

5 participants