Avoid Regex in DscClassCache.CreateKeywordFromCimClass#26306
Conversation
| static bool IsMagicProperty(string propertyName) => | ||
| string.Equals(propertyName, "ResourceId", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(propertyName, "SourceInfo", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(propertyName, "ModuleName", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(propertyName, "ModuleVersion", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(propertyName, "ConfigurationName", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
If this change is just to get rid of the Regex, then it doesn't make sense.
If it's about performance, then I suspect that Regex makes better optimizations than this change. And we need measurements to prove that there is a significant increase in parser performance.
There was a problem hiding this comment.
Avoiding Regex can make significant improvement:
| Method | propertyName | Mean | Error | Median | Ratio |
|---|---|---|---|---|---|
| IsMagicProperty_GeneratedRegex | ConfigurationName | 17.068 ns | 0.0866 ns | 17.056 ns | 9.66 |
| IsMagicProperty_StringEquals | ConfigurationName | 1.768 ns | 0.0382 ns | 1.772 ns | 1.00 |
| IsMagicProperty_CompiledRegex | ConfigurationName | 66.315 ns | 1.3416 ns | 65.370 ns | 37.52 |
| IsMagicProperty_RegexIsMatch | ConfigurationName | 127.821 ns | 1.7693 ns | 127.007 ns | 72.32 |
| IsMagicProperty_Regex | ConfigurationName | 157.995 ns | 3.1062 ns | 156.249 ns | 89.40 |
|
I don't think we have any tests for DSC. This means a high risk of regression. So we can't accept changes to this code. |
The only potential regression risk here relates to culture-specific behavior. Since Regex operations use the current culture by default, using However, the existing logic already applies |
daxian-dbw
added
the
CL-CodeCleanup
|
@xtqqczze Can you please update the PR description to briefly summarize the change? It should not be left blank. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CreateKeywordFromCimClass method to replace inefficient regular expression matching with direct string comparison methods. The changes improve performance by using StringComparison.OrdinalIgnoreCase for case-insensitive string matching instead of regex patterns.
Key changes:
- Replaced regex-based property and keyword validation with direct string equality checks
- Converted helper methods to local static functions with expression-bodied syntax
- Removed obsolete regex pattern constants that are no longer needed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
📣 Hey @@xtqqczze, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
Refactor several string comparisons that previously relied on regular expressions to match exact words in a case-insensitive manner. In these cases, regular expressions were unnecessary and less efficient than direct string comparison methods.