-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Improve DFA matcher builder startup perf with many constraints #46323
Conversation
Numbers: Main (interpreted regex)
Main (compiled regex)
PR
|
@@ -1,8 +1,6 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. | |||
// The .NET Foundation licenses this file to you under the MIT license. | |||
|
|||
#nullable disable |
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.
Note: Nothing has functionally changed in this class. I made changes to it (enable nullable, split up methods) intending to use it, but found I didn't need to. I left the changes here as they're all positive.
{ | ||
// Create regex instance lazily to avoid compiling regexes at app startup. Delay creation until constraint is first evaluation. | ||
// This is not thread safe. No side effect but multiple instances of a regex instance could be created from a burst of requests. | ||
_constraint ??= new Regex( |
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.
I guess if a constraint is evaluated once, it's likely to be hit an arbitrary number of times over the life of the app?
If that wasn't the case one could imagine lazily instantiating as interpreted first, then counting until a threshold and swapping for compiled.
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.
This is similar to what DI does, but it could be something we do after this change. We would need to make sure this happened in a place that could properly measure the "hit count". I'm not sure where that is.
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.
looks good; one question
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.
Very clever
@JamesNK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
@JamesNK sorry this missed branching for preview1 by just a few minutes. If you want it in that release, please do the needful. |
Waiting for preview 2 is fine. Thanks for the heads up though. |
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.
Looks great @JamesNK
Fixes #46154
RegexRouteConstraint
creates itsRegex
instance lazily. Still important in situations where the web app has many non-duplicate regexes.