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.

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

Apply Regex starting loop optimization to non-atomic loops as well #35936

Merged
merged 2 commits into from
May 9, 2020
Merged

Apply Regex starting loop optimization to non-atomic loops as well #35936

merged 2 commits into from
May 9, 2020

Conversation

stephentoub
Copy link
Member

@danmosemsft, @eerhardt, @pgovind, this appears sound, but it'd be great if you could double-check me on it.

@stephentoub stephentoub added this to the 5.0 milestone May 7, 2020
@ghost
Copy link

ghost commented May 7, 2020

Tagging subscribers to this area: @eerhardt
Notify danmosemsft if you want to be subscribed.

@pgovind
Copy link

pgovind commented May 8, 2020

I missed this PR somehow. I'll go over the logic and comment/approve tomorrow

stephentoub added 2 commits May 8, 2020 11:21
@stephentoub
Apply Regex starting loop optimization to non-atomic loops as well
6b72b4d
@stephentoub
Remove min iteration restriction
f359c8a 
The node.N > 0 restriction isn't necessary, and prevents this optimization from being used with * loops.    Worst case, the loop doesn't match anything, and we pay to overwrite the starting position with itself.  Best case, we eliminate a ton of cost.
@jaredpar jaredpar mentioned this pull request May 8, 2020
Closed
This was referenced May 8, 2020
Closed
Open
eerhardt
eerhardt approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

I can't think of a reason why this would break. If you are convinced it is correct, I think that's good enough for me. 😉

@stephentoub stephentoub closed this May 8, 2020
@stephentoub stephentoub reopened this May 8, 2020
pgovind
pgovind approved these changes May 8, 2020
View reviewed changes
Copy link

@pgovind pgovind left a comment

Choose a reason for hiding this comment

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

Alright, this looks good to me. I spent some time going over the parsing logic , ran some patterns locally and am reasonably convinced that this is a safe change.

@stephentoub stephentoub merged commit 386ee46 into dotnet:master May 9, 2020
@stephentoub stephentoub deleted the regexatomic branch May 9, 2020 10:12
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eerhardt eerhardt eerhardt approved these changes

@pgovind pgovind pgovind approved these changes

Assignees
No one assigned
Labels
area-System.Text.RegularExpressions tenet-performance Performance related issue
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@stephentoub @pgovind @eerhardt