-
Notifications
You must be signed in to change notification settings - Fork 275
[MTP] Improve performance of validating command line options #5655
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
if (invalidReservedOptions.Any()) | ||
// Create a HashSet of all system option names for faster lookup | ||
HashSet<string> systemOptionNames = new(); | ||
foreach (var provider in systemOptionsByProvider) |
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.
Use explicit type instead of 'var'
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.
Changed var
to explicit type KeyValuePair<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>>
in 86c987c.
{ | ||
var stringBuilder = new StringBuilder(); | ||
foreach (string reservedOption in invalidReservedOptions) | ||
foreach (var option in provider.Value) |
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.
Use explicit type instead of 'var'
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.
Changed var
to explicit type CommandLineOption
in 2e2ee02.
|
||
return ValidationResult.Invalid(stringBuilder.ToTrimmedString()); | ||
StringBuilder? stringBuilder = null; | ||
foreach (var provider in extensionOptionsByProvider) |
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.
Use explicit type instead of 'var'
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.
Changed var
to explicit type KeyValuePair<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>>
in 2e2ee02.
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Show resolved
Hide resolved
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
Outdated
Show resolved
Hide resolved
…tation Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. |
…thod Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5655 +/- ##
==========================================
- Coverage 76.37% 76.32% -0.05%
==========================================
Files 602 602
Lines 36759 36813 +54
==========================================
+ Hits 28075 28099 +24
- Misses 8684 8714 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This PR improves the performance of
CommandLineOptionsValidator
which was consuming an unnecessarily large amount of CPU time as identified in a trace involving multiple test processes.Performance improvements:
Algorithm Optimizations
ValidateNoUnknownOptions
by using a HashSet for O(1) lookups instead of nested LINQ operationsMemory Allocations
ToTrimmedString
to avoid unnecessary string allocationsData Structure Improvements
Union()
andIntersect()
operations with direct dictionary operationsDocumentation
PerformanceSensitive
attribute to document performance-critical code pathsBefore Optimization
Fixes #5651.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.