Skip to content

Allow Regex engine to be trimmed #46813

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.

Sign up for GitHub

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

Jump to bottom
Merged
eerhardt merged 3 commits into from
Feb 23, 2023
Merged

Allow Regex engine to be trimmed #46813

eerhardt merged 3 commits into from
Feb 23, 2023

Conversation

@eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 22, 2023

Move the lazy Regex creation code to a delegate that is only set with the RegexRouteConstraint constructor that takes a string regexPattern. This allows for the Regex engine to be trimmed when the regexPattern constructor is trimmed.

Fix #46142

Publishing dotnet new api -aot app on win-x64:

main: 12.1 MB (12,725,760 bytes)
PR: 11.3 MB (11,909,120 bytes)

@eerhardt
Allow Regex engine to be trimmed
10e292f 
Move the lazy Regex creation code to a delegate that is only set with the RegexRouteConstraint constructor that takes a string regexPattern. This allows for the Regex engine to be trimmed when the regexPattern constructor is trimmed.

Fix dotnet#46142
@eerhardt eerhardt requested a review from javiercn as a code owner February 22, 2023 18:41
@ghost ghost added the area-runtime label Feb 22, 2023
JamesNK
JamesNK approved these changes Feb 22, 2023
View reviewed changes
@eerhardt eerhardt enabled auto-merge (squash) February 22, 2023 22:44
halter73
halter73 approved these changes Feb 23, 2023
View reviewed changes
eerhardt
eerhardt commented Feb 23, 2023
View reviewed changes
@davidfowl
Copy link
Member

davidfowl commented Feb 23, 2023

I do not understand.

@build-analysis build-analysis bot mentioned this pull request Feb 23, 2023
Open
5 tasks
@eerhardt eerhardt merged commit ce8e0ce into dotnet:main Feb 23, 2023
@ghost ghost added this to the 8.0-preview2 milestone Feb 23, 2023
@JamesNK
Copy link
Member

JamesNK commented Feb 23, 2023 •
edited
Loading

I do not understand.

We want to trim away new Regex(string, ...). We can't have it lazily called in the regex property because the property is always used by Match. Moving lazily creating the regex to the new RegexRouteConstraint(string) constructor means it's trimmed away along with the constructor.

FYI the alpha constraint still inherits from RegexRouteConstraint, but it doesn't use the base string ctor:

aspnetcore/src/Http/Routing/src/Constraints/AlphaRouteConstraint.cs

Lines 12 to 23 in ce8e0ce

public partial class AlphaRouteConstraint : RegexRouteConstraint, ICachableParameterPolicy
{
/// <summary>
/// Initializes a new instance of the <see cref="AlphaRouteConstraint" /> class.
/// </summary>
public AlphaRouteConstraint() : base(GetAlphaRouteRegex())
{
}
[GeneratedRegex(@"^[A-Za-z]*$")]
private static partial Regex GetAlphaRouteRegex();
}

@eerhardt eerhardt deleted the TrimRegex branch February 23, 2023 14:13
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
@eerhardt eerhardt added the Perf label Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@JamesNK JamesNK JamesNK approved these changes

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@captainsafia captainsafia Awaiting requested review from captainsafia

@javiercn javiercn Awaiting requested review from javiercn

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Perf

Projects

None yet

Milestone

8.0-preview2

Development

Successfully merging this pull request may close these issues.

Resolve routing RegEx constraint dependency size issue

5 participants

@eerhardt @davidfowl @JamesNK @halter73 @amcasey