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

[release/6.0-preview7] Eliminate backtracking in the interpreter for patterns with .* #55960

Merged
merged 17 commits into from
Jul 20, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 19, 2021

Backport of #51508 to release/6.0-preview7

/cc @pgovind

Customer Impact

  1. Optimize regex patterns containing .* in them. In my performance tests (part of the original PR), I get a speed up of 6x

Testing

  1. Added ~25 new unit tests with .* and various patterns (strings/numbers/case sensitivity/RightToLeft)
  2. We also already have ~130 unit tests with the .* pattern

Risk

  1. Minimal. The changes in this PR are pure optimizations and don't change the current behavior. All this PR does is short-circuit a number of inefficient backtracking steps.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@ghost
Copy link

ghost commented Jul 19, 2021

Tagging subscribers to this area: @eerhardt, @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #51508 to release/6.0-preview7

/cc @pgovind

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Text.RegularExpressions, community-pr

Milestone: -

@terrajobst terrajobst removed the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley jeffhandley added the Servicing-consider Issue for next servicing release review label Jul 19, 2021
@danmoseley
Copy link
Member

This is approved to merge. The test failure is unrelated.

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.

matches main.

@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 20, 2021
@ghost
Copy link

ghost commented Jul 20, 2021

Hello @danmoseley!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@BrennanConroy
Copy link
Member

FYI, this change (in RC1) likely broke some tests in ASP.NET Core dotnet/aspnetcore#34491

Looking into the previous and current behavior, will post in a couple minutes the results.

@safern safern merged commit d885650 into release/6.0-preview7 Jul 20, 2021
@safern safern deleted the backport/pr-51508-to-release/6.0-preview7 branch July 20, 2021 18:06
@BrennanConroy
Copy link
Member

Rule: (.*)/(.*).aspx
Input: /pages/homepage.aspx

Expected backreferences:

  1. /pages/homepage.aspx
  2. /pages
  3. homepage

Actual backreferences: None, didn't match

@danmoseley @safern @pgovind I don't think this was intended to be a breaking change

@stephentoub
Copy link
Member

stephentoub commented Jul 20, 2021

I don't think this was intended to be a breaking change

It was not. This should be reverted.

On .NET 5, Console.WriteLine(Regex.IsMatch("/pages/homepage.aspx", "(.*)/(.*).aspx")); prints true.
With this change, Console.WriteLine(Regex.IsMatch("/pages/homepage.aspx", "(.*)/(.*).aspx")); prints false.

@pgovind
Copy link
Contributor

pgovind commented Jul 20, 2021

I don't think this was intended to be a breaking change

It was not. I think I see the problem. Let's revert this change for now

stephentoub added a commit that referenced this pull request Jul 20, 2021
stephentoub added a commit that referenced this pull request Jul 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants