Skip to content

Add limited support for backtracking Regex single char loops to simplified code gen - #60385

Merged
stephentoub merged 3 commits into
dotnet:mainfrom
stephentoub:simplebacktrackingloops
Oct 22, 2021
Merged

Add limited support for backtracking Regex single char loops to simplified code gen#60385
stephentoub merged 3 commits into
dotnet:mainfrom
stephentoub:simplebacktrackingloops

Conversation

@stephentoub

Copy link
Copy Markdown
Member

In .NET 5, we added simpler compiled code gen for regexes that didn't entail backtracking (or that had only very constrained backtracking, such as a top-level alternation). In our corpus of ~90K regular expressions, that code generator is employed for ~40% of them. The primary purpose of adding that code generator initially was performance, as it was able to avoid lots of the expense that original code generator had, especially for simple regexes. However, with the source generator, it's much more valuable to use this code gen as the generated code is human-readable and really helps to understand how the regex is operating, is much more easily debugged, etc.

This change allows the simplified code gen to be used even if there are backtracking single-character loops in the regex, as long as those loops are in a top-level concatenation (or a simple grouping structure like a capture). This increases the percentage of expressions in our corpus that will use the simplified code gen to ~65%.

Once we have the simplified loop code gen, it's also a lot easier to add in vectorization of searching for the next location to back off to based on a literal that comes immediately after the loop (e.g. "abc.*def"). This adds support into both RegexOptions.Compiled and the source generator to use LastIndexOf in that case.

The change also entailed adding/updating a few recursive functions. The plan has been to adopt the same model as in System.Linq.Expressions, Roslyn, and elsewhere, where we fork processing to continue on a secondary thread, rather than trying to enforce some max depth or rewrite as iterative, so I've done that as part of this change as well.

As an example, the "email" benchmark from:
https://github.com/mariomka/regex-benchmark/blame/244ca6c0e4bc8dd257904c51c0b5cabba6956dd2/csharp/Benchmark.cs#L19-L20

privatereadonlystaticRegexs_email=newRegex(@"[\w\.+-]+@[\w\.-]+\.[\w\.-]+",RegexOptions.Compiled);[Benchmark]publicintEmail()=>Count(s_email,s_mariomkaInput);privatestaticintCount(Regexr,stringinput){intcount=0;Matchm=r.Match(input);while(m.Success){count++;m=m.NextMatch();}returncount;}
MethodToolchainMeanRatio
Email\main\corerun.exe600.2 us1.00
Email\pr\corerun.exe485.2 us0.81

And here's the generated code for the matching logic before and after...

Before
protectedoverridevoidGo(){stringruntext=base.runtext!;intruntextbeg=base.runtextbeg;intruntextend=base.runtextend;intruntextpos=base.runtextpos;int[]runtrack=base.runtrack!;intruntrackpos=base.runtrackpos;int[]runstack=base.runstack!;intrunstackpos=base.runstackpos;inttmp1,tmp2,ch;// 000000 *Lazybranch addr = 29L0:runtrack[--runtrackpos]=runtextpos;runtrack[--runtrackpos]=0;// 000002 *SetmarkL1:runstack[--runstackpos]=runtextpos;runtrack[--runtrackpos]=1;// 000003 Setrep [+-.\\w], rep = 1L2:if(runtextend-runtextpos<1){gotoBacktrack;}for(inti=0;i<1;i++){if(!((ch=runtext[runtextpos+i])<128?("\0\0栀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0004\n+,-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){gotoBacktrack;}}runtextpos++;// 000006 Setloopatomic [+-.\\w], rep = infL3:tmp1=runtextend-runtextpos;// lengthtmp2=tmp1+1;while(--tmp2>0){if(!((ch=runtext[runtextpos++])<128?("\0\0栀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0004\n+,-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){runtextpos--;break;}}// 000009 UpdateBumpalongL4:runtrack[^1]=runtextpos;// 000010 One '@'L5:if(runtextpos>=runtextend||runtext[runtextpos++]!=64){gotoBacktrack;}// 000012 Setrep [-.\\w], rep = 1L6:if(runtextend-runtextpos<1){gotoBacktrack;}for(inti=0;i<1;i++){if(!((ch=runtext[runtextpos+i])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){gotoBacktrack;}}runtextpos++;// 000015 *Setloop [-.\\w], rep = infL7:tmp1=runtextend-runtextpos;// lengthtmp2=tmp1+1;while(--tmp2>0){if(!((ch=runtext[runtextpos++])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){runtextpos--;break;}}if(tmp2>=tmp1){gotoL8;}runtrack[--runtrackpos]=tmp1-tmp2-1;runtrack[--runtrackpos]=runtextpos-1;runtrack[--runtrackpos]=2;// 000018 One '.'L8:if(runtextpos>=runtextend||runtext[runtextpos++]!=46){gotoBacktrack;}// 000020 Setrep [-.\\w], rep = 1L9:if(runtextend-runtextpos<1){gotoBacktrack;}for(inti=0;i<1;i++){if(!((ch=runtext[runtextpos+i])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){gotoBacktrack;}}runtextpos++;// 000023 Setloopatomic [-.\\w], rep = infL10:tmp1=runtextend-runtextpos;// lengthtmp2=tmp1+1;while(--tmp2>0){if(!((ch=runtext[runtextpos++])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){runtextpos--;break;}}// 000026 *Capturemark index = 0L11:tmp1=runstack[runstackpos++];base.Capture(0,tmp1,runtextpos);runtrack[--runtrackpos]=tmp1;runtrack[--runtrackpos]=3;// 000029 StopL12:base.runtextpos=runtextpos;return;Backtrack:intlimit=base.runtrackcount*4;if(runstackpos<limit){base.runstackpos=runstackpos;base.DoubleStack();// might change runstackpos and runstackrunstackpos=base.runstackpos;runstack=base.runstack!;}if(runtrackpos<limit){base.runtrackpos=runtrackpos;base.DoubleTrack();// might change runtrackpos and runtrackruntrackpos=base.runtrackpos;runtrack=base.runtrack!;}switch(runtrack[runtrackpos++]){case0:{// 000000 *Lazybranch addr = 29runtextpos=runtrack[runtrackpos++];gotoL12;}case1:{// 000002 *Setmarkrunstackpos++;gotoBacktrack;}case2:{// 000015 *Setloop [-.\\w], rep = infruntextpos=runtrack[runtrackpos++];tmp1=runtrack[runtrackpos++];// positionif(tmp1>0){runtrack[--runtrackpos]=tmp1-1;runtrack[--runtrackpos]=runtextpos-1;runtrack[--runtrackpos]=2;}gotoL8;}case3:{// 000026 *Capturemark index = 0runstack[--runstackpos]=runtrack[runtrackpos++];base.Uncapture();gotoBacktrack;}default:{global::System.Diagnostics.Debug.Fail($"Unexpected backtracking state {runtrack[runtrackpos-1]}");break;}}}
After
protectedoverridevoidGo(){stringruntext=base.runtext!;intruntextpos=base.runtextpos;intruntextend=base.runtextend;intoriginalruntextpos=runtextpos;global::System.ReadOnlySpan<byte>byteSpan;charch;global::System.ReadOnlySpan<char>textSpan=global::System.MemoryExtensions.AsSpan(runtext,runtextpos,runtextend-runtextpos);// Concatenate//{// Setloopatomic [+-.\\w]+{inti0=0;while((uint)i0<(uint)textSpan.Length&&((ch=textSpan[i0])<128?("\0\0栀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0004\n+,-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){i0++;}if(i0<1){gotoNoMatch;}textSpan=textSpan.Slice(i0);runtextpos+=i0;}// UpdateBumpalong{base.runtextpos=runtextpos;}// One '@'{if((uint)textSpan.Length<1||textSpan[0]!='@'){gotoNoMatch;}}// Setloop [-.\\w]+//{runtextpos++;textSpan=textSpan.Slice(1);intstartingRunTextPos1=runtextpos;inti4=0;while((uint)i4<(uint)textSpan.Length&&((ch=textSpan[i4])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){i4++;}if(i4<1){gotoNoMatch;}textSpan=textSpan.Slice(i4);runtextpos+=i4;intendingRunTextPos2=runtextpos;intcrawlPos3=base.Crawlpos();startingRunTextPos1+=1;gotoEndLoop1;Backtrack0:if(startingRunTextPos1>=endingRunTextPos2){gotoNoMatch;}endingRunTextPos2=runtext.LastIndexOf('.',endingRunTextPos2-1,endingRunTextPos2-startingRunTextPos1);if(endingRunTextPos2<0){gotoNoMatch;}runtextpos=endingRunTextPos2;textSpan=global::System.MemoryExtensions.AsSpan(runtext,runtextpos,runtextend-runtextpos);EndLoop1://}// One '.'{if((uint)textSpan.Length<1||textSpan[0]!='.'){gotoBacktrack0;}}// Setloopatomic [-.\\w]+{runtextpos++;textSpan=textSpan.Slice(1);inti5=0;while((uint)i5<(uint)textSpan.Length&&((ch=textSpan[i5])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){i5++;}if(i5<1){gotoBacktrack0;}textSpan=textSpan.Slice(i5);runtextpos+=i5;}//}// Matchbase.runtextpos=runtextpos;base.Capture(0,originalruntextpos,runtextpos);return;// No matchNoMatch:return;}

@ghost

Copy link
Copy Markdown

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

In .NET 5, we added simpler compiled code gen for regexes that didn't entail backtracking (or that had only very constrained backtracking, such as a top-level alternation). In our corpus of ~90K regular expressions, that code generator is employed for ~40% of them. The primary purpose of adding that code generator initially was performance, as it was able to avoid lots of the expense that original code generator had, especially for simple regexes. However, with the source generator, it's much more valuable to use this code gen as the generated code is human-readable and really helps to understand how the regex is operating, is much more easily debugged, etc.

This change allows the simplified code gen to be used even if there are backtracking single-character loops in the regex, as long as those loops are in a top-level concatenation (or a simple grouping structure like a capture). This increases the percentage of expressions in our corpus that will use the simplified code gen to ~65%.

Once we have the simplified loop code gen, it's also a lot easier to add in vectorization of searching for the next location to back off to based on a literal that comes immediately after the loop (e.g. "abc.*def"). This adds support into both RegexOptions.Compiled and the source generator to use LastIndexOf in that case.

The change also entailed adding/updating a few recursive functions. The plan has been to adopt the same model as in System.Linq.Expressions, Roslyn, and elsewhere, where we fork processing to continue on a secondary thread, rather than trying to enforce some max depth or rewrite as iterative, so I've done that as part of this change as well.

As an example, the "email" benchmark from:
https://github.com/mariomka/regex-benchmark/blame/244ca6c0e4bc8dd257904c51c0b5cabba6956dd2/csharp/Benchmark.cs#L19-L20

privatereadonlystaticRegexs_email=newRegex(@"[\w\.+-]+@[\w\.-]+\.[\w\.-]+",RegexOptions.Compiled);[Benchmark]publicintEmail()=>Count(s_email,s_mariomkaInput);privatestaticintCount(Regexr,stringinput){intcount=0;Matchm=r.Match(input);while(m.Success){count++;m=m.NextMatch();}returncount;}
MethodToolchainMeanRatio
Email\main\corerun.exe600.2 us1.00
Email\pr\corerun.exe485.2 us0.81

And here's the generated code for the matching logic before and after...

Before
protectedoverridevoidGo(){stringruntext=base.runtext!;intruntextbeg=base.runtextbeg;intruntextend=base.runtextend;intruntextpos=base.runtextpos;int[]runtrack=base.runtrack!;intruntrackpos=base.runtrackpos;int[]runstack=base.runstack!;intrunstackpos=base.runstackpos;inttmp1,tmp2,ch;// 000000 *Lazybranch addr = 29L0:runtrack[--runtrackpos]=runtextpos;runtrack[--runtrackpos]=0;// 000002 *SetmarkL1:runstack[--runstackpos]=runtextpos;runtrack[--runtrackpos]=1;// 000003 Setrep [+-.\\w], rep = 1L2:if(runtextend-runtextpos<1){gotoBacktrack;}for(inti=0;i<1;i++){if(!((ch=runtext[runtextpos+i])<128?("\0\0栀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0004\n+,-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){gotoBacktrack;}}runtextpos++;// 000006 Setloopatomic [+-.\\w], rep = infL3:tmp1=runtextend-runtextpos;// lengthtmp2=tmp1+1;while(--tmp2>0){if(!((ch=runtext[runtextpos++])<128?("\0\0栀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0004\n+,-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){runtextpos--;break;}}// 000009 UpdateBumpalongL4:runtrack[^1]=runtextpos;// 000010 One '@'L5:if(runtextpos>=runtextend||runtext[runtextpos++]!=64){gotoBacktrack;}// 000012 Setrep [-.\\w], rep = 1L6:if(runtextend-runtextpos<1){gotoBacktrack;}for(inti=0;i<1;i++){if(!((ch=runtext[runtextpos+i])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){gotoBacktrack;}}runtextpos++;// 000015 *Setloop [-.\\w], rep = infL7:tmp1=runtextend-runtextpos;// lengthtmp2=tmp1+1;while(--tmp2>0){if(!((ch=runtext[runtextpos++])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){runtextpos--;break;}}if(tmp2>=tmp1){gotoL8;}runtrack[--runtrackpos]=tmp1-tmp2-1;runtrack[--runtrackpos]=runtextpos-1;runtrack[--runtrackpos]=2;// 000018 One '.'L8:if(runtextpos>=runtextend||runtext[runtextpos++]!=46){gotoBacktrack;}// 000020 Setrep [-.\\w], rep = 1L9:if(runtextend-runtextpos<1){gotoBacktrack;}for(inti=0;i<1;i++){if(!((ch=runtext[runtextpos+i])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){gotoBacktrack;}}runtextpos++;// 000023 Setloopatomic [-.\\w], rep = infL10:tmp1=runtextend-runtextpos;// lengthtmp2=tmp1+1;while(--tmp2>0){if(!((ch=runtext[runtextpos++])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){runtextpos--;break;}}// 000026 *Capturemark index = 0L11:tmp1=runstack[runstackpos++];base.Capture(0,tmp1,runtextpos);runtrack[--runtrackpos]=tmp1;runtrack[--runtrackpos]=3;// 000029 StopL12:base.runtextpos=runtextpos;return;Backtrack:intlimit=base.runtrackcount*4;if(runstackpos<limit){base.runstackpos=runstackpos;base.DoubleStack();// might change runstackpos and runstackrunstackpos=base.runstackpos;runstack=base.runstack!;}if(runtrackpos<limit){base.runtrackpos=runtrackpos;base.DoubleTrack();// might change runtrackpos and runtrackruntrackpos=base.runtrackpos;runtrack=base.runtrack!;}switch(runtrack[runtrackpos++]){case0:{// 000000 *Lazybranch addr = 29runtextpos=runtrack[runtrackpos++];gotoL12;}case1:{// 000002 *Setmarkrunstackpos++;gotoBacktrack;}case2:{// 000015 *Setloop [-.\\w], rep = infruntextpos=runtrack[runtrackpos++];tmp1=runtrack[runtrackpos++];// positionif(tmp1>0){runtrack[--runtrackpos]=tmp1-1;runtrack[--runtrackpos]=runtextpos-1;runtrack[--runtrackpos]=2;}gotoL8;}case3:{// 000026 *Capturemark index = 0runstack[--runstackpos]=runtrack[runtrackpos++];base.Uncapture();gotoBacktrack;}default:{global::System.Diagnostics.Debug.Fail($"Unexpected backtracking state {runtrack[runtrackpos-1]}");break;}}}
After
protectedoverridevoidGo(){stringruntext=base.runtext!;intruntextpos=base.runtextpos;intruntextend=base.runtextend;intoriginalruntextpos=runtextpos;global::System.ReadOnlySpan<byte>byteSpan;charch;global::System.ReadOnlySpan<char>textSpan=global::System.MemoryExtensions.AsSpan(runtext,runtextpos,runtextend-runtextpos);// Concatenate//{// Setloopatomic [+-.\\w]+{inti0=0;while((uint)i0<(uint)textSpan.Length&&((ch=textSpan[i0])<128?("\0\0栀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0004\n+,-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){i0++;}if(i0<1){gotoNoMatch;}textSpan=textSpan.Slice(i0);runtextpos+=i0;}// UpdateBumpalong{base.runtextpos=runtextpos;}// One '@'{if((uint)textSpan.Length<1||textSpan[0]!='@'){gotoNoMatch;}}// Setloop [-.\\w]+//{runtextpos++;textSpan=textSpan.Slice(1);intstartingRunTextPos1=runtextpos;inti4=0;while((uint)i4<(uint)textSpan.Length&&((ch=textSpan[i4])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){i4++;}if(i4<1){gotoNoMatch;}textSpan=textSpan.Slice(i4);runtextpos+=i4;intendingRunTextPos2=runtextpos;intcrawlPos3=base.Crawlpos();startingRunTextPos1+=1;gotoEndLoop1;Backtrack0:if(startingRunTextPos1>=endingRunTextPos2){gotoNoMatch;}endingRunTextPos2=runtext.LastIndexOf('.',endingRunTextPos2-1,endingRunTextPos2-startingRunTextPos1);if(endingRunTextPos2<0){gotoNoMatch;}runtextpos=endingRunTextPos2;textSpan=global::System.MemoryExtensions.AsSpan(runtext,runtextpos,runtextend-runtextpos);EndLoop1://}// One '.'{if((uint)textSpan.Length<1||textSpan[0]!='.'){gotoBacktrack0;}}// Setloopatomic [-.\\w]+{runtextpos++;textSpan=textSpan.Slice(1);inti5=0;while((uint)i5<(uint)textSpan.Length&&((ch=textSpan[i5])<128?("\0\0怀Ͽ\ufffe\ufffe\u07ff"[ch>>4]&(1<<(ch&0xF)))!=0:CharInClass((char)ch,"\0\u0002\n-/\0\u0002\u0004\u0005\u0003\u0001\u0006\t\u0013\0"))){i5++;}if(i5<1){gotoBacktrack0;}textSpan=textSpan.Slice(i5);runtextpos+=i5;}//}// Matchbase.runtextpos=runtextpos;base.Capture(0,originalruntextpos,runtextpos);return;// No matchNoMatch:return;}
Author:stephentoub
Assignees:-
Labels:

area-System.Text.RegularExpressions

Milestone:7.0.0

@danmoseley

Copy link
Copy Markdown
Contributor

Just curious, what does the generated code look like for this example? could you share?

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Just curious, what does the generated code look like for this example? could you share?

It's already in the PR description. Expand the before/ after nodes.

@danmoseley

Copy link
Copy Markdown
Contributor

It's surprisingly readable!

@stephentoub

Copy link
Copy Markdown
MemberAuthor

It's surprisingly readable!

😯 Oh ye of little faith 😄

@jeffhandley

Copy link
Copy Markdown
Member

😯 Oh ye of little faith 😄

There's a joke here somewhere about the readability of goto statements (that is still respectful), but I don't know what it is. 😼

@stephentoub

Copy link
Copy Markdown
MemberAuthor

@BrzVlad, any idea why the Libraries Test Run release mono_interpreter Linux x64 Debug failed leg is failing here? It looks like it's getting a seg fault.

@BrzVlad

Copy link
Copy Markdown
Member

@stephentoub Should get fixed by #60514

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Thanks

Comment threadsrc/libraries/System.Text.RegularExpressions/tests/RegexReductionTests.cs Outdated
@stephentoub
stephentoubforce-pushed the simplebacktrackingloops branch 3 times, most recently from 85a8031 to 7dfe534CompareOctober 19, 2021 14:30
@runfoapprunfoappBot mentioned this pull request Oct 19, 2021

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a few questions - mostly for my learning.

Comment threadsrc/libraries/System.Text.RegularExpressions/tests/RegexReductionTests.cs Outdated
Comment threadsrc/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs Outdated
Comment threadsrc/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs Outdated
Comment threadsrc/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs Outdated
@stephentoub
stephentoubforce-pushed the simplebacktrackingloops branch 2 times, most recently from dd72b32 to 124ddcfCompareOctober 21, 2021 18:05
@stephentoub

stephentoub commented Oct 21, 2021

Copy link
Copy Markdown
MemberAuthor

@safern, some drawing tests have repeatedly failed on this PR. It's not clear to me how my changes here could have broken this, but it's failed multiple times.

 System.Drawing.Tests.PenTests.Ctor_Brush_Width<SolidBrush>(brush: SolidBrush { Color = Color [Red] }, width: 0, expectedPenType: SolidColor) [FAIL]
Assert.Equal() Failure
Expected: 0
Actual: 1
Stack Trace:
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(1384,0): at System.Drawing.Tests.PenTests.VerifyPen[T](Pen pen, PenType expectedPenType, Single expectedWidth)
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(62,0): at System.Drawing.Tests.PenTests.Ctor_Brush_Width[T](T brush, Single width, PenType expectedPenType)
System.Drawing.Tests.PenTests.Ctor_Brush_Width<SolidBrush>(brush: SolidBrush { Color = Color [Red] }, width: -1, expectedPenType: SolidColor) [FAIL]
Assert.Equal() Failure
Expected: -1
Actual: 1
Stack Trace:
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(1384,0): at System.Drawing.Tests.PenTests.VerifyPen[T](Pen pen, PenType expectedPenType, Single expectedWidth)
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(62,0): at System.Drawing.Tests.PenTests.Ctor_Brush_Width[T](T brush, Single width, PenType expectedPenType)
System.Drawing.Tests.PenTests.Ctor_Brush_Width<SolidBrush>(brush: SolidBrush { Color = Color [Red] }, width: -�, expectedPenType: SolidColor) [FAIL]
Assert.Equal() Failure
Expected: -�
Actual: 1
Stack Trace:
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(1384,0): at System.Drawing.Tests.PenTests.VerifyPen[T](Pen pen, PenType expectedPenType, Single expectedWidth)
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(62,0): at System.Drawing.Tests.PenTests.Ctor_Brush_Width[T](T brush, Single width, PenType expectedPenType)
System.Drawing.Tests.PenTests.Ctor_Color_Width(width: -1) [FAIL]
Assert.Equal() Failure
Expected: -1
Actual: 1
Stack Trace:
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(1384,0): at System.Drawing.Tests.PenTests.VerifyPen[T](Pen pen, PenType expectedPenType, Single expectedWidth)
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(116,0): at System.Drawing.Tests.PenTests.Ctor_Color_Width(Single width)
System.Drawing.Tests.PenTests.Ctor_Color_Width(width: 0) [FAIL]
Assert.Equal() Failure
Expected: 0
Actual: 1
Stack Trace:
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(1384,0): at System.Drawing.Tests.PenTests.VerifyPen[T](Pen pen, PenType expectedPenType, Single expectedWidth)
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(116,0): at System.Drawing.Tests.PenTests.Ctor_Color_Width(Single width)
System.Drawing.Tests.PenTests.Ctor_Color_Width(width: -�) [FAIL]
Assert.Equal() Failure
Expected: -�
Actual: 1
Stack Trace:
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(1384,0): at System.Drawing.Tests.PenTests.VerifyPen[T](Pen pen, PenType expectedPenType, Single expectedWidth)
/_/src/libraries/System.Drawing.Common/tests/PenTests.cs(116,0): at System.Drawing.Tests.PenTests.Ctor_Color_Width(Single width)

Is there any known issue here? I don't see any open issues for it.

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Ah, I see #60731 was literally just created.

@safern

safern commented Oct 21, 2021

Copy link
Copy Markdown
Member

Hmm interesting, I'll dig into what cause them to start failing and disable them. Thanks for the ping.

…ified code gen
In .NET 5, we added simpler compiled code gen for regexes that didn't entail backtracking (or that had only very constrained backtracking, such as a top-level alternation). In our corpus of ~90K regular expressions, that code generator is employed for ~40% of them. The primary purpose of adding that code generator initially was performance, as it was able to avoid lots of the expense that original code generator had, especially for simple regexes. However, with the source generator, it's much more valuable to use this code gen as the generated code is human-readable and really helps to understand how the regex is operating, is much more easily debugged, etc.
This change allows the simplified code gen to be used even if there are backtracking single-character loops in the regex, as long as those loops are in a top-level concatenation (or a simple grouping structure like a capture). This increases the percentage of expressions in our corpus that will use the simplified code gen to ~65%.
Once we have the simplified loop code gen, it's also a lot easier to add in vectorization of searching for the next location to back off to based on a literal that comes immediately after the loop (e.g. "abc.*def"). This adds support into both RegexOptions.Compiled and the source generator to use LastIndexOf in that case.
The change also entailed adding/updating a few recursive functions. The plan has been to adopt the same model as in System.Linq.Expressions, Roslyn, and elsewhere, where we fork processing to continue on a secondary thread, rather than trying to enforce some max depth or rewrite as iterative, so I've done that as part of this change as well.
@stephentoub
stephentoubforce-pushed the simplebacktrackingloops branch from 124ddcf to e8bb072CompareOctober 21, 2021 21:14
@stephentoub
stephentoub merged commit 8c8157f into dotnet:mainOct 22, 2021
@stephentoub
stephentoub deleted the simplebacktrackingloops branch October 22, 2021 04:34
@kunalspathak

Copy link
Copy Markdown
Contributor

Linux/x64 improvement - dotnet/perf-autofiling-issues#1975

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Linux/x64 improvement

Excellent.

@ghostghost locked as resolved and limited conversation to collaborators Nov 25, 2021
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.

8 participants

@stephentoub@danmoseley@jeffhandley@BrzVlad@safern@kunalspathak@am11@eerhardt