Captures support for NonBacktracking #65129

olsaarik · 2022-02-10T10:21:39Z

This adds captures support for RegexOptions.NonBacktracking by replacing the third phase of the match generation algorithm with an NFA simulation that includes additional effects to record capture group starts and ends on transitions. Multiple captures of a single group are not supported, only the last one is.

The support relies on a new variant of the symbolic version of the Antimirov derivative that only includes the parts of the pattern that the backtracking engine would visit before hitting a nullable path (one that doesn't consume any characters). This in conjunction with maintaining a prioritized set of states in the NFA simulation allows faithful reproduction of the match generation semantics that the backtracking engines have.

The captures support does incur a slowdown, which is sometimes significant, since NFA simulation with application of effects is significantly more heavy weight than DFA simulation. Currently this cost is also paid when no subcaptures are needed, which can be avoided in the future by modifying the existing third phase to use the new prioritized "eager" derivative. A Brzozowski variant of it would be relatively straightforward to create.

One optimization that is currently missing, is how effects associated with finalizing a match are applied. They are currently interpreted from the SymbolicRegexNode tree. These should either be cached or delayed until the match is actually decided to avoid paying the cost multiple times.

I've gone through all the tests and enabled everything relying on subcaptures that I could find.

Re-enable replacement patterns. Fix eager derivative. Fix capture numbering to work with sparsely numbered groups.

Also take any subcaptures out of RegexExperiment intersection and negation tests, as capture semantics with these are not correct yet.

This ensures the correct length matches always.

ghost · 2022-02-10T10:21:49Z

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

Issue Details

This adds captures support for RegexOptions.NonBacktracking by replacing the third phase of the match generation algorithm with an NFA simulation that includes additional effects to record capture group starts and ends on transitions. Multiple captures of a single group are not supported, only the last one is.

The support relies on a new variant of the symbolic version of the Antimirov derivative that only includes the parts of the pattern that the backtracking engine would visit before hitting a nullable path (one that doesn't consume any characters). This in conjunction with maintaining a prioritized set of states in the NFA simulation allows faithful reproduction of the match generation semantics that the backtracking engines have.

The captures support does incur a slowdown, which is sometimes significant, since NFA simulation with application of effects is significantly more heavy weight than DFA simulation. Currently this cost is also paid when no subcaptures are needed, which can be avoided in the future by modifying the existing third phase to use the new prioritized "eager" derivative. A Brzozowski variant of it would be relatively straightforward to create.

One optimization that is currently missing, is how effects associated with finalizing a match are applied. They are currently interpreted from the SymbolicRegexNode tree. These should either be cached or delayed until the match is actually decided to avoid paying the cost multiple times.

I've gone through all the tests and enabled everything relying on subcaptures that I could find.

Author:	olsaarik
Assignees:	-
Labels:	`area-System.Text.RegularExpressions`
Milestone:	-

src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.cs

...stem.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/DerivativeEffect.cs

...stem.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/DfaMatchingState.cs

...s/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SparseIntMap.cs

....Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexBuilder.cs

stephentoub · 2022-02-11T20:13:05Z

Thanks, @olsaarik!

The performance hit in most cases pretty reasonable

Is it a fair assumption with the most recent round of changes, if there aren't any captures / if someone specifies ExplicitCaptures, that additional overhead evaporate? So effectively if someone wanted the old behavior and perf, they can still get it?

olsaarik · 2022-02-11T20:17:25Z

Yes, exactly. I had to add those groups to the Sherlock tests to get the performance hit.

olsaarik · 2022-02-11T20:24:42Z

@veanes Something I found I needed to add in addition to the loop subsumption/combining optimization was ordered deduplication such that entries don't get repeated. I'm guessing that was causing a significant performance hit before I added it (with things blowing up).

veanes · 2022-02-11T20:51:35Z

@olsaarik I think getting rid of unordered-or and the related data-structure that uses HashSets and Dictionaries, (as we discussed earlier) and instead normalizes the ordered-or say using hashcodes and removes duplicates would benefit both ordered and unordered cases. In unordered cases simply more aggressive simplifications would be used but e.g. removing duplicates would be done in both cases. The underlying AST node would just be the un-ordered-or. I am not suggesting to do it this time around, but maybe for the next round of edits, because this is a bigger edit.

This avoids some repeated allocations in the capturing mode.

olsaarik · 2022-02-11T21:22:49Z

@veanes I agree, the unordered nodes would just additionally sort with the hash code oslt. The deduplication is already in, so I think making that change is mostly just removing code all around. I agree lets leave it for after this PR.

src/libraries/System.Text.RegularExpressions/src/System.Text.RegularExpressions.csproj

src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.cs

...stem.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/DfaMatchingState.cs

...gularExpressions/src/System/Text/RegularExpressions/Symbolic/RegexNodeToSymbolicConverter.cs

.../System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicMatch.cs

....Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexBuilder.cs

....Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexMatcher.cs

joperezr · 2022-02-11T23:13:30Z

...tem.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexNode.cs

@@ -220,6 +224,11 @@ bool WithCache(uint context)
                        is_nullable = _alts.IsNullableFor(context);
                        break;

+                    case SymbolicRegexKind.OrderedOr:


Another thing that is for sure something that should be done outside of this PR, but we should probably should unify with the none-symbolic engines and use RegexNodeKind enum for these instead.

Sounds like a good idea. The RegexNodeKind has some entries that aren't valid for SymbolicRegexNode, so some extra checks on the symbolic side might be in order, and some new node kinds in RegexNodeKind would be required.

There is some overlap but also some differences between RegexNodeKind and SymbolicRegexKind,
(e.g. Not, And, if they are going to remain, but also other cases such as EndAnchorZRev that is EndAnchorZ when reversed, WatchDog, and some cases in RegexNodeKind that are not distinguished in SymbolicRegexKind concerning for example different kinds of loops and their loop bodies)

...RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexRunnerFactory.cs

src/libraries/System.Text.RegularExpressions/tests/AttRegexTests.cs

stephentoub · 2022-02-12T17:33:32Z

src/libraries/System.Text.RegularExpressions/tests/RegexExperiment.cs

-        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]
-        public void SRMTest_ConjuctionIsMatch()
+        //[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]
+        private void SRMTest_ConjuctionIsMatch()


What's the deal with all of these commented out tests?

These are tests that were added for the unexposed conjunction and negation support, but which the captures support breaks. We're not quite sure what the semantics for conjunction and negation should be with captures, so it was easiest to disable the tests for now.

stephentoub · 2022-02-12T17:34:15Z

Thanks, @olsaarik. Let's get this merged, and we can subsequently address style, comments, remaining perf concerns, etc.

olsaarik added 14 commits February 10, 2022 01:21

Initial version of captures in NonBacktracking

21d2cd0

Working version of captures in NonBacktracking

998321f

State/transition priorities for NonBacktracking

ba9cffd

Various fixes for capturing in NonBacktracking

55f6d27

Re-enable replacement patterns. Fix eager derivative. Fix capture numbering to work with sparsely numbered groups.

Enable many subcapture tests for NonBacktracking

7d400a4

Also take any subcaptures out of RegexExperiment intersection and negation tests, as capture semantics with these are not correct yet.

Use new eager derivative even without subcaptures

3436ba0

This ensures the correct length matches always.

Enable more subcapture tests for NonBacktracking

23f9299

Performance work for NonBacktracking captures

6b2797e

Bug fixes and comments

1c44a5d

Fix for changes removing exclusive_end

8b4be26

Rename back to _lower

a0c8212

Fix beginning handling for captures

0134dd3

Resurrect deleted test

a8c7c20

Remove debugging test

a908fa4

olsaarik requested review from stephentoub and veanes February 10, 2022 10:21

dotnet-issue-labeler bot added the area-System.Text.RegularExpressions label Feb 10, 2022

ghost assigned olsaarik Feb 10, 2022