Skip to content

Commit 3d151d8

Browse files
CopilotdanmoseleyCopilothoyosjsnoahfalk
authored
Enhance heap progress PR: -noprogress flag, deterministic timing tests, literal output assertions (dotnet#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 dotnet#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 dotnet#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>
1 parent a4a7a3b commit 3d151d8

7 files changed

Lines changed: 295 additions & 4 deletions

File tree

‎src/Microsoft.Diagnostics.ExtensionCommands/DumpHeapCommand.cs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,9 @@ public class DumpHeapCommand : ClrRuntimeCommandBase
6868
[Option(Name = "-ignoreGCState", Help = "Ignore the GC's marker that the heap is not walkable (will generate lots of false positive errors).")]
6969
public bool IgnoreGCState { get; set; }
7070

71+
[Option(Name = "-noprogress", Help = "Suppress periodic progress output during heap scanning.")]
72+
public bool NoProgress { get; set; }
73+
7174
[Argument(Help = "Optional memory ranges in the form of: [Start [End]]")]
7275
public string[] MemoryRange { get; set; }
7376

@@ -77,7 +80,14 @@ public override void Invoke()
7780
{
7881
ParseArguments();
7982

80-
IEnumerable<ClrObject> objectsToPrint = FilteredHeap.EnumerateFilteredObjects(Console.CancellationToken);
83+
Action<long, long> progressCallback = null;
84+
if (StatOnly && !NoProgress)
85+
{
86+
ProgressReporter reporter = new(Console.WriteLine);
87+
progressCallback = reporter.Report;
88+
}
89+
90+
IEnumerable<ClrObject> objectsToPrint = FilteredHeap.EnumerateFilteredObjects(Console.CancellationToken, progressCallback);
8191

8292
bool? liveObjectWarning = null;
8393
if ((Live || Dead) && Short)

‎src/Microsoft.Diagnostics.ExtensionCommands/HeapWithFilters.cs‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,10 +209,26 @@ public IEnumerable<ClrSegment> EnumerateFilteredSegments(ClrSubHeap subheap)
209209
return segments;
210210
}
211211

212-
public IEnumerable<ClrObject> EnumerateFilteredObjects(CancellationToken cancellation)
212+
public IEnumerable<ClrObject> EnumerateFilteredObjects(CancellationToken cancellation, Action<long, long> progressCallback = null)
213213
{
214-
foreach (ClrSegment segment in EnumerateFilteredSegments())
214+
IEnumerable<ClrSegment> segments = EnumerateFilteredSegments();
215+
216+
long totalBytes = 0;
217+
if (progressCallback != null)
215218
{
219+
// Materialize segments to compute totalBytes before enumeration begins.
220+
List<ClrSegment> segmentList = segments.ToList();
221+
totalBytes = segmentList.Sum(s => (long)s.CommittedMemory.Length);
222+
segments = segmentList;
223+
}
224+
225+
// Tracks the sum of CommittedMemory.Length for all completed segments.
226+
long pastSegmentBytes = 0;
227+
228+
foreach (ClrSegment segment in segments)
229+
{
230+
long lastReportedBytesInSegment = 0;
231+
216232
IEnumerable<ClrObject> objs;
217233
if (MemoryRange is MemoryRange range)
218234
{
@@ -232,9 +248,21 @@ public IEnumerable<ClrObject> EnumerateFilteredObjects(CancellationToken cancell
232248
{
233249
cancellation.ThrowIfCancellationRequested();
234250

251+
// Report every ProgressStepBytes of progress within the segment, measured by object address.
252+
if (progressCallback != null)
253+
{
254+
long bytesInSegment = (long)(obj.Address - segment.Start);
255+
if (bytesInSegment - lastReportedBytesInSegment >= ProgressStepBytes)
256+
{
257+
progressCallback(pastSegmentBytes + bytesInSegment, totalBytes);
258+
lastReportedBytesInSegment = bytesInSegment;
259+
}
260+
}
261+
235262
if (obj.IsValid)
236263
{
237264
ulong size = obj.Size;
265+
238266
if (MinimumObjectSize != 0 && size < MinimumObjectSize)
239267
{
240268
continue;
@@ -248,7 +276,14 @@ public IEnumerable<ClrObject> EnumerateFilteredObjects(CancellationToken cancell
248276

249277
yield return obj;
250278
}
279+
280+
// Advance the past-segment accumulator and report at the end of each segment.
281+
pastSegmentBytes += (long)segment.CommittedMemory.Length;
282+
progressCallback?.Invoke(pastSegmentBytes, totalBytes);
251283
}
252284
}
285+
286+
/// <summary>Minimum byte advancement within a segment that triggers an in-segment progress callback.</summary>
287+
private const int ProgressStepBytes = 16 * 1024;
253288
}
254289
}

‎src/Microsoft.Diagnostics.ExtensionCommands/Microsoft.Diagnostics.ExtensionCommands.csproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@
1818
<PackageReference Include="Microsoft.Diagnostics.Runtime" Version="$(MicrosoftDiagnosticsRuntimeVersion)" />
1919
</ItemGroup>
2020

21+
<ItemGroup>
22+
<InternalsVisibleTo Include="Microsoft.Diagnostics.ExtensionCommands.UnitTests" />
23+
</ItemGroup>
24+
2125
<ItemGroup>
2226
<ProjectReference Include="$(MSBuildThisFileDirectory)..\Microsoft.Diagnostics.DebugServices\Microsoft.Diagnostics.DebugServices.csproj" />
2327
<ProjectReference Include="$(MSBuildThisFileDirectory)..\Microsoft.SymbolStore\Microsoft.SymbolStore.csproj" />

‎src/Microsoft.Diagnostics.ExtensionCommands/ProgressReporter.cs‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Diagnostics;
6+
7+
namespace Microsoft.Diagnostics.ExtensionCommands
8+
{
9+
/// <summary>
10+
/// Receives byte-progress callbacks from heap enumeration and writes a formatted
11+
/// progress message to the console whenever enough time has elapsed.
12+
/// </summary>
13+
internal sealed class ProgressReporter
14+
{
15+
/// <summary>
16+
/// Default minimum interval in milliseconds between progress writes.
17+
/// </summary>
18+
public const long DefaultReportingInterval = 5000;
19+
20+
private readonly Action<string> _writeMessage;
21+
private readonly long _intervalMs;
22+
private readonly Func<long> _getElapsedMs;
23+
private long _lastReportMs;
24+
25+
/// <summary>
26+
/// Creates a new ProgressReporter using the system clock.
27+
/// </summary>
28+
/// <param name="writeMessage">Called with the formatted progress string to display.</param>
29+
/// <param name="intervalMs">Minimum interval in milliseconds between writes.</param>
30+
public ProgressReporter(Action<string> writeMessage, long intervalMs = DefaultReportingInterval)
31+
: this(writeMessage, intervalMs, getElapsedMs: null)
32+
{
33+
}
34+
35+
/// <summary>
36+
/// Creates a new ProgressReporter with an injectable time source for testing.
37+
/// </summary>
38+
/// <param name="writeMessage">Called with the formatted progress string to display.</param>
39+
/// <param name="intervalMs">Minimum interval in milliseconds between writes.</param>
40+
/// <param name="getElapsedMs">
41+
/// Returns the current elapsed time in milliseconds. When <see langword="null"/>,
42+
/// a real <see cref="Stopwatch"/> is used.
43+
/// </param>
44+
internal ProgressReporter(Action<string> writeMessage, long intervalMs, Func<long> getElapsedMs)
45+
{
46+
_writeMessage = writeMessage ?? throw new ArgumentNullException(nameof(writeMessage));
47+
_intervalMs = intervalMs;
48+
if (getElapsedMs is not null)
49+
{
50+
_getElapsedMs = getElapsedMs;
51+
}
52+
else
53+
{
54+
Stopwatch sw = Stopwatch.StartNew();
55+
_getElapsedMs = () => sw.ElapsedMilliseconds;
56+
}
57+
}
58+
59+
/// <summary>
60+
/// Called by the heap enumerator with the current scan position.
61+
/// Writes a progress message if enough time has elapsed since the last write.
62+
/// </summary>
63+
public void Report(long scannedBytes, long totalBytes)
64+
{
65+
long elapsedMs = _getElapsedMs();
66+
if (elapsedMs - _lastReportMs >= _intervalMs)
67+
{
68+
_lastReportMs = elapsedMs;
69+
_writeMessage(FormatProgressMessage(scannedBytes, totalBytes));
70+
}
71+
}
72+
73+
/// <summary>
74+
/// Formats a progress message suitable for display during heap scanning.
75+
/// </summary>
76+
public static string FormatProgressMessage(long scannedBytes, long totalBytes)
77+
{
78+
double pct = totalBytes > 0 ? 100.0 * scannedBytes / totalBytes : 0;
79+
return FormattableString.Invariant(
80+
$"Scanning heap: {scannedBytes / (1024 * 1024):n0} MB / {totalBytes / (1024 * 1024):n0} MB ({pct:f0}%)...");
81+
}
82+
}
83+
}

‎src/Microsoft.Diagnostics.ExtensionCommands/VerifyHeapCommand.cs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
3+
using System;
34
using System.Collections.Generic;
45
using System.Diagnostics;
56
using Microsoft.Diagnostics.DebugServices;
@@ -28,6 +29,9 @@ public class VerifyHeapCommand : ClrRuntimeCommandBase
2829
[Option(Name = "-ignoreGCState", Help = "Ignore the GC's marker that the heap is not walkable (will generate lots of false positive errors).")]
2930
public bool IgnoreGCState { get; set; }
3031

32+
[Option(Name = "-noprogress", Help = "Suppress periodic progress output during heap scanning.")]
33+
public bool NoProgress { get; set; }
34+
3135
[Argument(Help ="Optional memory ranges in the form of: [Start [End]]")]
3236
public string[] MemoryRange { get; set; }
3337

@@ -58,7 +62,14 @@ public override void Invoke()
5862
throw new DiagnosticsException("The GC heap is not in a valid state for traversal. (Use -ignoreGCState to override.)");
5963
}
6064

61-
VerifyHeap(filteredHeap.EnumerateFilteredObjects(Console.CancellationToken), verifySyncTable: filteredHeap.HasFilters);
65+
Action<long, long> progressCallback = null;
66+
if (!NoProgress)
67+
{
68+
ProgressReporter reporter = new(Console.WriteLine);
69+
progressCallback = reporter.Report;
70+
}
71+
72+
VerifyHeap(filteredHeap.EnumerateFilteredObjects(Console.CancellationToken, progressCallback), verifySyncTable: filteredHeap.HasFilters);
6273
}
6374

6475
[HelpInvoke]

‎src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/Microsoft.Diagnostics.ExtensionCommands.UnitTests.csproj‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
3+
<PropertyGroup>
4+
<TargetFrameworks>$(SupportedXUnitTestTargetFrameworks)</TargetFrameworks>
5+
<IsPackable>false</IsPackable>
6+
</PropertyGroup>
7+
8+
<ItemGroup>
9+
<PackageReference Include="xunit" Version="$(XUnitVersion)" />
10+
</ItemGroup>
11+
12+
<ItemGroup>
13+
<ProjectReference Include="$(SrcDir)Microsoft.Diagnostics.ExtensionCommands\Microsoft.Diagnostics.ExtensionCommands.csproj" />
14+
</ItemGroup>
15+
16+
</Project>

‎src/tests/Microsoft.Diagnostics.ExtensionCommands.UnitTests/ProgressReporterTests.cs‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Collections.Generic;
5+
using Microsoft.Diagnostics.ExtensionCommands;
6+
using Xunit;
7+
8+
namespace Microsoft.Diagnostics.ExtensionCommands.UnitTests
9+
{
10+
public class ProgressReporterTests
11+
{
12+
// ─── FormatProgressMessage ───────────────────────────────────────────────
13+
14+
[Fact]
15+
public void FormatProgressMessage_FormatsCorrectly()
16+
{
17+
string msg = ProgressReporter.FormatProgressMessage(
18+
scannedBytes: 5L * 1024 * 1024 * 1024, // 5 GB → 5,120 MB
19+
totalBytes: 16L * 1024 * 1024 * 1024); // 16 GB → 16,384 MB (31%)
20+
21+
Assert.Equal("Scanning heap: 5,120 MB / 16,384 MB (31%)...", msg);
22+
}
23+
24+
[Fact]
25+
public void FormatProgressMessage_HandlesZeroTotal()
26+
{
27+
string msg = ProgressReporter.FormatProgressMessage(0, 0);
28+
Assert.Equal("Scanning heap: 0 MB / 0 MB (0%)...", msg);
29+
}
30+
31+
[Fact]
32+
public void FormatProgressMessage_Handles100Percent()
33+
{
34+
string msg = ProgressReporter.FormatProgressMessage(1024 * 1024, 1024 * 1024); // 1 MB / 1 MB
35+
Assert.Equal("Scanning heap: 1 MB / 1 MB (100%)...", msg);
36+
}
37+
38+
// ─── Timing-based write tests (deterministic via fake clock) ──────────
39+
40+
[Fact]
41+
public void Report_WithZeroInterval_WritesEveryTime()
42+
{
43+
int writeCount = 0;
44+
long fakeMs = 0;
45+
46+
ProgressReporter reporter = new(
47+
_ => writeCount++,
48+
intervalMs: 0,
49+
getElapsedMs: () => fakeMs);
50+
51+
reporter.Report(100, 1000);
52+
reporter.Report(300, 1000);
53+
reporter.Report(600, 1000);
54+
55+
Assert.Equal(3, writeCount);
56+
}
57+
58+
[Fact]
59+
public void Report_DoesNotWriteBeforeInterval()
60+
{
61+
int writeCount = 0;
62+
long fakeMs = 0;
63+
64+
ProgressReporter reporter = new(
65+
_ => writeCount++,
66+
intervalMs: 10_000,
67+
getElapsedMs: () => fakeMs);
68+
69+
// First call at t=0ms: elapsed (0) - lastReport (0) = 0, not >= 10_000, no write
70+
reporter.Report(1, 1000);
71+
Assert.Equal(0, writeCount);
72+
73+
// Advance to just before the interval boundary; still no write
74+
fakeMs = 9_999;
75+
reporter.Report(2, 1000);
76+
Assert.Equal(0, writeCount);
77+
}
78+
79+
[Fact]
80+
public void Report_WritesAfterInterval()
81+
{
82+
int writeCount = 0;
83+
long fakeMs = 0;
84+
85+
ProgressReporter reporter = new(
86+
_ => writeCount++,
87+
intervalMs: 10_000,
88+
getElapsedMs: () => fakeMs);
89+
90+
// t=0ms: 0 - 0 = 0, not >= 10_000 → no write
91+
reporter.Report(1, 1000);
92+
Assert.Equal(0, writeCount);
93+
94+
// t=10_000ms: 10_000 - 0 = 10_000, >= 10_000 → writes; lastReport becomes 10_000
95+
fakeMs = 10_000;
96+
reporter.Report(2, 1000);
97+
Assert.Equal(1, writeCount);
98+
99+
// t=15_000ms: 15_000 - 10_000 = 5_000, not >= 10_000 → no write
100+
fakeMs = 15_000;
101+
reporter.Report(3, 1000);
102+
Assert.Equal(1, writeCount);
103+
104+
// t=20_000ms: 20_000 - 10_000 = 10_000, >= 10_000 → writes; lastReport becomes 20_000
105+
fakeMs = 20_000;
106+
reporter.Report(4, 1000);
107+
Assert.Equal(2, writeCount);
108+
}
109+
110+
[Fact]
111+
public void Report_WritesFormattedMessageWithCorrectBytes()
112+
{
113+
List<string> messages = new();
114+
long fakeMs = 0;
115+
116+
ProgressReporter reporter = new(
117+
messages.Add,
118+
intervalMs: 10_000,
119+
getElapsedMs: () => fakeMs);
120+
121+
// t=0ms: no write yet
122+
reporter.Report(500 * 1024 * 1024, 2000 * 1024 * 1024);
123+
Assert.Empty(messages);
124+
125+
// t=10_000ms: writes with the current scanned/total values
126+
fakeMs = 10_000;
127+
reporter.Report(800 * 1024 * 1024, 2000 * 1024 * 1024);
128+
Assert.Single(messages);
129+
Assert.Equal("Scanning heap: 800 MB / 2,000 MB (40%)...", messages[0]);
130+
}
131+
}
132+
}

0 commit comments

Comments
 (0)