Skip to content
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

improvement regex #1548

Merged
merged 10 commits into from Apr 29, 2022
Merged

improvement regex #1548

merged 10 commits into from Apr 29, 2022

Conversation

EngRajabi
Copy link
Contributor

improvement regex with compile flag

@JRahnama
Copy link
Member

JRahnama commented Apr 2, 2022

@EngRajabi I am not sure if that makes any good here.

Compiled regular expressions maximize run-time performance at the expense of initialization time.

Can you provide some bench marking to support your idea please?

@EngRajabi
Copy link
Contributor Author

Yes, I did.It is a bit slow to use the first time, but much faster the next time
https://medium.com/@mohsen_rajabi/how-to-write-a-regex-very-fast-in-c-best-practice-875d386c0485

@JRahnama
Copy link
Member

JRahnama commented Apr 6, 2022

Yes, I did.It is a bit slow to use the first time, but much faster the next time

@DavoudEshtehari and @Wraith2 any opinion on this? I think we should not sacrifice initialization time for runtime performance unless we have a good benchmark to show a practical test case.

@EngRajabi can you write a test case with SqlClient and do some benchmarking on it? The one you have provided is merely for RegEx and does not contain any SqlClient related tests.

@Wraith2
Copy link
Contributor

Wraith2 commented Apr 6, 2022

@DavoudEshtehari and @Wraith2 any opinion on this? I think we should not sacrifice initialization time for runtime performance unless we have a good benchmark to show a practical test case.

I looked at this when it was posted and my opinon was that it is not harmful. It's not in a heavily used code path for any performance benchmarks I've seen so it's not going to cause trouble for most people. If it is heavily used for a small number of people then this is probably an improvement for them. Ideally I'd like to see benchmark numbers but it's a single line change in a low traffic area that might benefit users so I'm not against it.

We keep a reference the to the regex so we're not relying on the static regex cache we will do the work only once. On netfx compiled regex output an assembly which is going to cause codegen which is slow but if it's heavily used there may be an overall perf benefit. On netcore <7 compiled flag does nothing, on 7+ it may cause a build time code generator to generate the compiled expression which will be faster for users in all cases.

@DavoudEshtehari
Copy link
Member

Following the Wraith2 comment, it'll improve a small number of customers with high loads.

Best Practices
To maximize the performance of regular expressions in the .NET Framework, we recommend that you do the following when working with regular expression objects:

  • Call static regular expression matching methods instead of repeatedly instantiating the same regular expression object.
  • Use interpreted regular expressions when you expect to call pattern-matching methods a limited number of times to parse text.
  • Use compiled regular expressions to optimize performance for regular expression patterns that are known in advance and when the frequency of calls to the regular expression’s pattern-matching methods can vary extensively.

@EngRajabi
Copy link
Contributor Author

Thanks for the explanation friends
According to Microsoft documentation
https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices

And the test I took
https://medium.com/@mohsen_rajabi/how-to-write-a-regex-very-fast-in-c-best-practice-875d386c0485

The following should be noted in regular expressions
-Use the Regex object statically because they are threadsafe
-Use the compiled flag

@David-Engel David-Engel added this to the 5.0.0-preview3 milestone Apr 12, 2022
@JRahnama JRahnama merged commit 3072fd6 into dotnet:main Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants