Skip to content

Use static Regex.IsMatch in ValueMatchFilter to take advantage of caching #4055

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

Closed
stevenaw opened this issue Mar 8, 2022 · 3 comments · Fixed by #4056
Closed

Use static Regex.IsMatch in ValueMatchFilter to take advantage of caching #4055

stevenaw opened this issue Mar 8, 2022 · 3 comments · Fixed by #4056
Assignees
Labels
Milestone

Comments

@stevenaw
Copy link
Member

stevenaw commented Mar 8, 2022

The static Regex functions internally cache parsed regex instances based on the provided pattern on .NET 5+. ValueMatchFilter can be used to filter tests based on a regex and will apply the same regex to all potential candidates to see if it matches. This means that on a large test suite there could be measurable savings by using an overload which caches the regex.

Something like this:
https://github.com/nunit/nunit/blob/master/src%2FNUnitFramework%2Fframework%2FInternal%2FFilters%2FValueMatchFilter.cs#L43

-return input != null && new Regex(ExpectedValue).IsMatch(input);
+return input != null && Regex.IsMatch(input, ExpectedValue);
@stevenaw
Copy link
Member Author

stevenaw commented Mar 8, 2022

Alternately, we could consider implementing our own caching mechanism so that all runtimes can benefit

@manfred-brands
Copy link
Member

manfred-brands commented Mar 9, 2022

I think it is much simpler than that. ExpectedValue is a constant initialized in the constructor.
Instead of using it as a string, we an create a (compiled) RegEx instance here and called IsMatch on that instance.
I find the IsRegEx property a bit weird. The pattern can't change type halfway through. Looking at its usages it is mainly used as an init property. In my opinion it should be changed to a constructor parameter.

This matches the pattern that is already used in StackFilter and RegexConstaint.

I will create a PR.

@stevenaw
Copy link
Member Author

stevenaw commented Mar 9, 2022

Thanks @manfred-brands ! That was exactly what I had been thinking too, right down to setting IsRegEx as a ctor param. Thanks for offering to take this on 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants