-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
changing expander regex #11210
changing expander regex #11210
Conversation
…iled one where applicable
Contributes to #7598 |
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.
Thank you!
If you have any measurements you run in your env - feel free to attach here and brag more
It looks good! Do you plan to adapt them too to the new practice? |
Thanks. I noticed this one as it was visible in profiler, that being said, unifying the rest is a good idea - I will see what I can do. so the impact in context of MSBUild is in the range of ~0.1%, but still visible.
Now my test strings for matching are not all that representative so this targeted illustration isn't pefect:
Namely in Expander, we're already doing some fail-soon pre-scanning so some of the edge cases might be slightly different. I'm pondering how much more we could gain by tweaking the Regex.Replace itself, but that is in the realm of speculation. |
to use code generated version instead of compiled one where applicable according to this .NET article.
This nets a small but visible performance gain with only a minor code update.