-
-
Notifications
You must be signed in to change notification settings - Fork 18
Faster way for performance monitor to get child processes #7574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Typically takes a fraction of a second vs 5-10s for the old approach.
There was a problem hiding this 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 optimizes the performance monitor's child process enumeration by replacing a slow WMI-based approach with a faster Windows Toolhelp32 API-based solution. The old implementation made multiple WMI queries per generation of child processes, taking 5-10 seconds, whereas the new approach takes a snapshot of all processes once and walks the process tree in memory.
Changes:
- Replaced iterative, generation-by-generation WMI queries with a single snapshot-based approach using Windows Toolhelp32 API
- Added fallback to WMI-based approach for non-Windows platforms or when the fast method fails
- Refactored descendant process discovery to use breadth-first search with a parent-to-children map
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); | ||
| if (snapshot == IntPtr.Zero || snapshot == INVALID_HANDLE_VALUE) | ||
| return map; |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When CreateToolhelp32Snapshot fails, the function silently returns an empty map, which will result in no child processes being found. Consider logging this failure for diagnostics, as it could indicate a system issue or permission problem that might affect performance monitoring accuracy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new or changed code. Don't want to mess with it.
| catch | ||
| { | ||
| // If the fast Windows approach fails for any reason, fall back to WMI below. |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch block silently swallows all exceptions when the Windows approach fails. Consider logging the exception for diagnostics to help identify why the optimized path failed and whether the fallback is being triggered frequently.
| catch | |
| { | |
| // If the fast Windows approach fails for any reason, fall back to WMI below. | |
| catch (Exception ex) | |
| { | |
| // If the fast Windows approach fails for any reason, fall back to WMI below. | |
| Debug.WriteLine($"GetAllDescendantProcessesWindows failed; falling back to WMI. Exception: {ex}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| foreach (var childPid in children) | ||
| { | ||
| if (descendantPids.Add(childPid)) | ||
| queue.Enqueue(childPid); | ||
| } | ||
| } | ||
|
|
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var childPid in children) | |
| { | |
| if (descendantPids.Add(childPid)) | |
| queue.Enqueue(childPid); | |
| } | |
| } | |
| foreach (var childPid in children.Where(childPid => descendantPids.Add(childPid))) | |
| { | |
| queue.Enqueue(childPid); | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to risk changing this. The current code is not difficult to understand, and I'm not 100% sure that there is no difference between checking when the pid is enqueued and checking when it is dequeued.
| foreach (var childPid in children) | ||
| { | ||
| if (descendantPids.Add(childPid)) | ||
| queue.Enqueue(childPid); | ||
| } |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk reviewed 1 file and all commit messages, made 1 comment, resolved 9 discussions, and dismissed @copilot[bot] from 9 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @JohnThomson).
src/BloomExe/Utils/PerformanceMeasurement.cs line 385 at r1 (raw file):
} return GetAllDescendantProcessesWmi(parent.Id);
Seems like we should wrap this in a try with empty catch. We definitely don't want performance monitor causing user crashes (not that users are likely to have it on... but still seems worth it).
|
@andrew-polk I've opened a new pull request, #7583, to work on those changes. Once the pull request is ready, I'll request review from you. |
JohnThomson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have taken some of the suggestions, not going to use the AI PR.
@JohnThomson made 7 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk).
src/BloomExe/Utils/PerformanceMeasurement.cs line 385 at r1 (raw file):
Previously, andrew-polk wrote…
Seems like we should wrap this in a try with empty catch. We definitely don't want performance monitor causing user crashes (not that users are likely to have it on... but still seems worth it).
Done.
| catch | ||
| { | ||
| // If the fast Windows approach fails for any reason, fall back to WMI below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| foreach (var childPid in children) | ||
| { | ||
| if (descendantPids.Add(childPid)) | ||
| queue.Enqueue(childPid); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to risk changing this. The current code is not difficult to understand, and I'm not 100% sure that there is no difference between checking when the pid is enqueued and checking when it is dequeued.
| var snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); | ||
| if (snapshot == IntPtr.Zero || snapshot == INVALID_HANDLE_VALUE) | ||
| return map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new or changed code. Don't want to mess with it.
| listMOs.AddRange( | ||
| new ManagementObjectSearcher( | ||
| "Select ProcessId, ParentProcessId From Win32_Process" | ||
| ) | ||
| .Get() | ||
| .Cast<ManagementObject>() | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| foreach (var childPid in children) | ||
| { | ||
| if (descendantPids.Add(childPid)) | ||
| queue.Enqueue(childPid); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JohnThomson).
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk dismissed @copilot[bot] from 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrew-polk).
|
@andrew-polk I've opened a new pull request, #7593, to work on those changes. Once the pull request is ready, I'll request review from you. |
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @JohnThomson).
Typically takes a fraction of a second vs 5-10s for the old approach.
This change is