Skip to content

Skip RegexOptions.NonBacktracking phase 3 for some patterns - #65531

Merged
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:fixnbperf
Feb 18, 2022
Merged

Skip RegexOptions.NonBacktracking phase 3 for some patterns#65531
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:fixnbperf

Conversation

@stephentoub

@stephentoubstephentoub commented Feb 17, 2022

Copy link
Copy Markdown
Member

If a pattern doesn't have any captures and if any match of that pattern will always be the same length, we can skip the Phase 3 computation as, given the computed starting position of the match, we know exactly where it's going to end.

Also took the opportunity to add some comments.

Fixes#65383 (mostly... I think the rest is #65532)

MethodToolchainPatternOptionsMeanRatio
Count\main\corerun.exe(?i)HolmesNonBacktracking675.67 us1.00
Count\pr\corerun.exe(?i)HolmesNonBacktracking645.59 us0.96
Count\main\corerun.exe(?i)SherlockNonBacktracking150.84 us1.00
Count\pr\corerun.exe(?i)SherlockNonBacktracking141.54 us0.94
Count\main\corerun.exe(?i)Sherlock HolmesNonBacktracking160.33 us1.00
Count\pr\corerun.exe(?i)Sherlock HolmesNonBacktracking148.33 us0.93
Count\main\corerun.exeHolmesNonBacktracking131.74 us1.00
Count\pr\corerun.exeHolmesNonBacktracking110.33 us0.84
Count\main\corerun.exeSherlockNonBacktracking63.59 us1.00
Count\pr\corerun.exeSherlockNonBacktracking55.97 us0.88
Count\main\corerun.exeSherlock HolmesNonBacktracking76.50 us1.00
Count\pr\corerun.exeSherlock HolmesNonBacktracking65.42 us0.86

If a pattern doesn't have any captures and if any match of that pattern will always be the same length, we can skip the Phase 3 computation as, given the computed starting position of the match, we know exactly where it's going to end.
@ghost

Copy link
Copy Markdown

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

Issue Details

If a pattern doesn't have any captures and if any match of that pattern will always be the same length, we can skip the Phase 3 computation as, given the computed starting position of the match, we know exactly where it's going to end.

Also took the opportunity to add some comments.

Fixes #65383 (mostly)

Author:stephentoub
Assignees:-
Labels:

area-System.Text.RegularExpressions

Milestone:7.0.0

Comment on lines 515 to 522
if (startat == input.Length)
{
// Covers the special-case of an empty match at the end of the input.
uint prevKind = GetCharKind(input, startat - 1);
uint nextKind = GetCharKind(input, startat);

bool emptyMatchExists = _pattern.IsNullableFor(CharKind.Context(prevKind, nextKind));
return emptyMatchExists ?
return _pattern.IsNullableFor(CharKind.Context(prevKind, nextKind)) ?
new SymbolicMatch(startat, 0) :
SymbolicMatch.NoMatch;
}

@olsaarikolsaarikFeb 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at the new comment I realized this optimization should additionally handle the case where there are capture groups and do the same ApplyEffects thing FindEndPositionCapturing is doing. The difference will be visible for some patterns with nullable capture groups that have anchors in them.

Edit: Oh actually any nullable patterns with nullable capture groups.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks, @olsaarik. That's pre-existing this PR, yes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, shouldn't block this PR, just something I noticed.

@kunalspathak

kunalspathak commented Feb 22, 2022

Copy link
Copy Markdown
Contributor

@ghostghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for freeto 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.

Regressions in System.Text.RegularExpressions.Tests.Perf_Regex_Industry_RustLang_Sherlock

4 participants

@stephentoub@kunalspathak@joperezr@olsaarik