Avoid Regex in DscClassCache.CreateKeywordFromCimClass by xtqqczze · Pull Request #26306 · PowerShell/PowerShell
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.
Comment on lines +1535 to +1540
| 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); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 StringComparison.OrdinalIgnoreCase could be a change in behavior.
However, the existing logic already applies StringComparison.OrdinalIgnoreCase to the same string values, meaning these changes actually bring consistency.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The change looks low risk to me. Most of those regex expressions are only to match a string that is exactly one of some words ignoring the case, which should not use regex in the first place in my opinion.
@xtqqczze Can you please update the PR description to briefly summarize the change? It should not be left blank.
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 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.
