Removed WMI due to performance#13610
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the Windows WMI-based implementation used to retrieve a process command line (to address significant performance regressions during node reuse scanning), and updates unit tests to reflect that command-line retrieval is not supported on Windows.
Changes:
- Removed the Windows WMI/COM-based command-line retrieval path from
ProcessExtensions.TryGetCommandLine. - Treated Windows as unsupported for command-line retrieval (returns
nullcommand line) and updated unit tests accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Framework/Utilities/ProcessExtensions.cs | Removes WMI-based Windows command-line retrieval to avoid the perf hit. |
| src/Utilities.UnitTests/ProcessExtensions_Tests.cs | Updates expectations on Windows to assert commandLine is null. |
| try | ||
| { | ||
| #if NET | ||
| if (NativeMethods.IsWindows) | ||
| { | ||
| commandLine = Windows.GetCommandLine(process.Id); | ||
| return true; | ||
| } | ||
| else if (NativeMethods.IsOSX || NativeMethods.IsBSD) | ||
| if (NativeMethods.IsOSX || NativeMethods.IsBSD) | ||
| { | ||
| commandLine = BSD.GetCommandLine(process.Id); | ||
| return true; |
There was a problem hiding this comment.
TryGetCommandLine now treats Windows/other platforms as unsupported (returns true with commandLine == null). However, the method's XML doc says it returns false when the platform doesn't support command-line retrieval, and the unsupported-OS branch comment says "return false" even though it returns true. Please align the docs/comments with the actual contract (or adjust the return value/contract consistently across callers).
There was a problem hiding this comment.
Correctness & Edge Cases — one issue found
Comment/code mismatch (line 78): The comment says "return false" but the code returns true. This is not a runtime bug — the true + null return is correct for the callers — but the comment directly contradicts the code it describes.
Everything else checks out:
- Callers handle
(true, null)correctly. Both call sites inNodeProviderOutOfProcBase.csexplicitly guardif (commandLine is null)and include the process, which is the right fallback when command-line retrieval is unsupported. ExtractFromCommandLineis null-safe. It checksstring.IsNullOrWhiteSpaceup front, so even if a BSD/Linux helper returnednull, callers would not crash (though they'd getNodeMode? == nulland exclude the process — thenullguard in callers fires first anyway).- XML doc
<returns>is slightly misleading (says "true if successfully retrieved" buttruealso means "unsupported platform"), however this is a pre-existing issue — the .NET Framework path already returned(true, null)before this PR.
Generated by Expert Code Review (on open) for issue #13610 · ● 11.7M
🔍 Skill Validator Results✅ All checks passed
Summary
Full validator output```text Found 7 skill(s) [assessing-breaking-changes] 📊 assessing-breaking-changes: 992 BPE tokens [chars/4: 1,154] (detailed ✓), 8 sections, 2 code blocks [maintaining-binary-log-compatibility] 📊 maintaining-binary-log-compatibility: 1,271 BPE tokens [chars/4: 1,509] (detailed ✓), 12 sections, 2 code blocks [authoring-errors-and-warnings] 📊 authoring-errors-and-warnings: 1,287 BPE tokens [chars/4: 1,393] (detailed ✓), 13 sections, 4 code blocks [changewaves] 📊 changewaves: 901 BPE tokens [chars/4: 1,007] (detailed ✓), 9 sections, 1 code blocks [running-unit-tests] 📊 running-unit-tests: 2,376 BPE tokens [chars/4: 2,310] (detailed ✓), 10 sections, 6 code blocks [optimizing-msbuild-performance] 📊 optimizing-msbuild-performance: 1,182 BPE tokens [chars/4: 1,342] (detailed ✓), 10 sections, 1 code blocks [integrating-sdk-and-msbuild] 📊 integrating-sdk-and-msbuild: 1,250 BPE tokens [chars/4: 1,440] (detailed ✓), 14 sections, 2 code blocks ✅ All checks passed (7 skill(s)) Found 1 agent(s) Validated 1 agent(s)✅ All checks passed (1 agent(s)) |
bd3d68b to
c7c5f5e
Compare
Fixes #13550
Context
WMI is causing issues, so we need to remove it now and find a better solution.
Changes Made
WMI code was removed and Windows is treated as unsupported OS.
Testing
Manual testing with running MSBuild worker nodes.
Notes