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

Add RegexOptions.Compiled option to RegexRouteConstraint #46192

Merged
merged 1 commit into from Jan 25, 2023

Conversation

eugeneogongo
Copy link
Contributor

@eugeneogongo eugeneogongo commented Jan 20, 2023

Add RegexOptions.Compiled option to RegexRouteConstraint

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Add RegexOptions.Compiled option to RegexRouteConstraint

Fixes #46154

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jan 20, 2023
@ghost
Copy link

ghost commented Jan 20, 2023

Thanks for your PR, @eugeneogongo. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@eugeneogongo
Copy link
Contributor Author

@surayya-MS would you go through this PR?
Thank you.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@mkArtakMSFT mkArtakMSFT added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-runtime labels Jan 21, 2023
@mkArtakMSFT
Copy link
Member

@surayya-MS can you please review this? Thanks!

@mkArtakMSFT
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@surayya-MS
Copy link
Member

Thanks for the contribution

@surayya-MS surayya-MS merged commit 8389520 into dotnet:main Jan 25, 2023
@ghost ghost added this to the 8.0-preview1 milestone Jan 25, 2023
@JamesNK
Copy link
Member

JamesNK commented Jan 26, 2023

I looked back in time, and we've never used RegexOptions.Compiled. Not even from the first commit:

Constraint = new Regex(regexPattern, RegexOptions.CultureInvariant | RegexOptions.IgnoreCase);

Maybe Compiled was left out accidentally, but it could have been purposefully removed. I worry about web app startup time. For example, an app could have thousands of routes with regex constraints. Are they all compiled when the app starts up? That could really hurt time to first request.

Edit: I'm going to reopen #46154. Make sure we aren't helping one area by hurting another.

@eugeneogongo
Copy link
Contributor Author

@JamesNK The cost of constructing the Regex object may be higher, but the cost of performing matches with it is likely to be much smaller.

@ghost
Copy link

ghost commented Jan 26, 2023

Hi @eugeneogongo. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@JamesNK
Copy link
Member

JamesNK commented Jan 26, 2023

@eugeneogongo The performance improvement in matching the constraint is great, but I think we should take some time to verify that nothing else (startup time, total app memory usage) has regressed when there are a lot of routes with regex constraints.

If measuring highlights RegexOptions.Compiled is a problem, there might be ways to have the best of both worlds. For example, waiting to create the Regex instance until the first time the route constraint is evaluated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RegexRouteConstraint should use Compiled Regex
6 participants