Regex: reduce allocation slightly, add tests, code cleanup, add parser comments by ViktorHofer · Pull Request #30632 · dotnet/corefx

ViktorHofer · 2018-06-24T15:19:37Z

Make RegexParser a ref struct and use ValueListBuilder with Span for the options stack instead of a List.
Add tests for Group.Synchronized & regex comments #comment and other corner cases that weren't covered especially with right to left.
Replace some method entry condition checks with Debug Asserts where applicable.
Code cleanup & simplify code in a few cases (readonly, ToLower on Span instead of single chars, variable declaration moved away from on top of method, some comments replaced with XML comments)
Add comments to better describe RegexParser behavior.

Before (master)

Method	Mean	Error	StdDev	Gen 0	Allocated
RegexCtor	101.8 ms	0.5352 ms	0.4179 ms	56500.0000	113.07 MB
RegexCtorIgnoreCase	98.15 ms	1.913 ms	2.278 ms	66312.5000	132.64 MB

After

Method	Mean	Error	StdDev	Gen 0	Allocated
RegexCtor	97.78 ms	0.5337 ms	0.4731 ms	53687.5000	107.44 MB
RegexCtorIgnoreCase	94.40 ms	1.030 ms	0.8600 ms	62312.5000	124.65 MB

Only minor improvements, I didn't expect anything severe. 5-6% less allocation, ~2-3% better perf.
Contributes towards https://github.com/dotnet/corefx/issues/30507

jkotas · 2018-06-24T16:36:46Z

            {
                pattern = string.Create(pattern.Length, (pattern, culture), (span, state) =>
-                {
-                    // We do the ToLower character by character for consistency.  With surrogate chars, doing


Is the issue described in this comment no longer relevant?

Do we have a test for it?

I don't think the comment ever applied, I don't really get the part with the consistency and I prefer to operate on the whole string not just because of convenience but also because of integrity, like the surrogate pairs that the author mentioned here.

Yes we have several tests that hit these code paths. Our test bed is fortunately very rich.

jkotas · 2018-06-24T16:38:24Z

@@ -43,15 +43,7 @@ public RegexBoyerMoore(string pattern, bool caseInsensitive, bool rightToLeft, C
            if (caseInsensitive)
            {
                pattern = string.Create(pattern.Length, (pattern, culture), (span, state) =>


You do not need to call string.Create at all here now. Just call pattern.ToLower(culture)

right, missed that. thanks

I was able to completely get rid of the call as it is already lowered by the RegexParser. I changed a few test cases to have more hits of the combination of a prefix and the IgnoreCase option and added an assert.

jkotas · 2018-06-24T16:41:15Z


-        private RegexOptions _options;
-        private List<RegexOptions> _optionsStack;
+        private RegexOptions _option;


Can this have more than one bit set? If it is the case, _options is a better name for it.

yes it usually has multiple bits set. You are right, plural makes more sense then. I'll revert my change.

jkotas · 2018-06-24T16:45:21Z

-            RegexParser p;
-            RegexNode root;
+            Span<RegexOptions> optionSpan = stackalloc RegexOptions[OptionStackDefaultSize];
+            var parser = new RegexParser(pattern, option, culture, optionSpan);


Is the Regex parsing recursive?

If it is, is there a problem with consuming a lot more stack than before and running into stackoverflow?

Is the Regex parsing recursive?

The whole parsing logic happens in ScanRegex which then calls helper functions for different branches. There should not be any recursion involved in the parsing. Also we have a very good test coverage in S.T.RegularExpressions (> 90% code and branch coverage) and with #29178 we added a lot of tests cases for the RegexParser.

jkotas · 2018-06-24T16:46:03Z

-                    {
-                        ReadOnlySpan<char> input = state._pattern.AsSpan(pos, cch);
-
-                        // We do the ToLower character by character for consistency.  With surrogate chars, doing


Same concern as in the other place

Yes we have several tests that hit these code paths. Our test bed is fortunately very rich.

I do not see any tests in the test bed that exercise surrogates that this comment is talking about.

Because our engine operates on UTF16 code units and does not support surrogate pairs?

https://docs.microsoft.com/en-us/dotnet/standard/base-types/character-classes-in-regular-expressions#SupportedNamedBlocks

We should still have tests for it, even if it is documented as unsupported in the documentation. E.g. we should not be crashing.

The comment you are removing suggests that handling of surrogates showed up as a problem in the past and that somebody tried to make it better/more predictable at least.

Right, there weren't any tests that exercised our misinterpretation. I added a positive and negative test for surrogate pairs. The positive one is working around the code unit problem and the negative one is expecting the ArgumentException because of the way interpret the input.

The comment you are removing suggests that handling of surrogates showed up as a problem in the past and that somebody tried to make it better/more predictable at least.

Right and that's why I believe we should handle the lowering of the (sub-)pattern in its entirety and not on a character basis. If we ever switch from our current UTF-16 handling to something else the result should still be valid. To check my sanity I just compared the results of two ToLower operations - one on an entire string and one per character - with surrogate values and the results are the identical, of course.

jkotas · 2018-06-24T18:43:11Z

What is the RegexCtor benchmark doing? I am not able to find it in https://github.com/dotnet/corefx/tree/master/src/System.Text.RegularExpressions/tests/Performance

ViktorHofer · 2018-06-24T18:49:29Z

I use the following test code https://gist.github.com/ViktorHofer/2b8338fc164ea105f79a456856ff43cb and run it locally with this script:

.\CoreRun ..\..\..\tools\csc.dll /noconfig /optimize /r:System.Private.Corelib.dll /r:System.Runtime.dll /r:System.Runtime.Extensions.dll /r:System.Console.dll /r:System.Text.RegularExpressions.dll /r:System.Collections.dll /r:BenchmarkDotNet.dll /r:BenchmarkDotNet.Core.dll /out:regex-perf.dll regex-perf.cs
.\CoreRun regex-perf.dll

jkotas · 2018-06-24T22:35:27Z

                                    // (?...

-                                    // get the options if it's an option construct (?cimsx-cimsx...)
+                                    // get the options if it's an options construct (?cimsx-cimsx...)


an options - accidental find&replace ?

Prefix patterns which are passed to RegexBoyerMoore are already lowercased by the parser. Remove the redundant ToLower() call and assert the patterns lowercase state

ViktorHofer · 2018-06-25T13:31:45Z

@dotnet-bot test Linux arm64 Release Build
@dotnet-bot test Linux x64 Release Build
@dotnet-bot test Linux-musl x64 Debug Build

jkotas · 2018-06-25T15:27:34Z

+        /// </summary>
+        public bool IsMatch(string text, int index, int beglimit, int endlimit)
        {
+            // Length-cognizance optimization


Are you sure that this optimization is valid for case-insensitive mode?

that optimization was already in place, I did not change it: http://source.dot.net/#System.Text.RegularExpressions/System/Text/RegularExpressions/RegexBoyerMoore.cs,257

but yes you could be right, I'm currently debugging the code paths, let me see...

sorry I discovered that the code I removed is indeed used therefore I reverted my changes and your comment shouldn't apply anymore. I added tests to cover the hidden paths.

Add tests for rtl anchored patterns.

ViktorHofer · 2018-06-26T16:52:20Z

@dotnet-bot test Linux arm Release Build
@dotnet-bot test Tizen armel Debug Build

danmoseley · 2018-06-27T01:04:36Z

Any more feedback @jkotas?

jkotas · 2018-06-27T01:54:20Z

I do not have more feedback, however I have not reviewed this in detail.

MarcoRossignoli · 2018-06-28T08:22:08Z

I use the following test code https://gist.github.com/ViktorHofer/2b8338fc164ea105f79a456856ff43cb and run it locally with this script:
.\CoreRun ......\tools\csc.dll /noconfig /optimize /r:System.Private.Corelib.dll /r:System.Runtime.dll /r:System.Runtime.Extensions.dll /r:System.Console.dll /r:System.Text.RegularExpressions.dll /r:System.Collections.dll /r:BenchmarkDotNet.dll /r:BenchmarkDotNet.Core.dll /out:regex-perf.dll regex-perf.cs
.\CoreRun regex-perf.dll

@ViktorHofer cool and light way!You could add this way at the botton of benchmarking with BDN doc.

ViktorHofer · 2018-06-28T08:30:40Z

We don't recommend using this approach as it involves using internals (csc) and manually referencing the ref assemblies. That said, I documented it a few months ago under advanced inner loop in docs.

MarcoRossignoli · 2018-06-28T09:04:08Z

We don't recommend using this approach as it involves using internals (csc) and manually referencing the ref assemblies. That said, I documented it a few months ago under advanced inner loop in docs.

Understood, however it's an "alternative" until BDN benchmarking will be fixed. Found doc thank's a lot!

ViktorHofer · 2018-06-28T18:07:51Z

@danmosemsft @stephentoub do you mind reviewing / approving?

stephentoub · 2018-06-29T13:26:27Z

-                        for (int i = 0; i < input.Length; i++)
-                            span[i] = textInfo.ToLower(input[i]);
-                    });
+                        state._pattern.AsSpan(pos, cch).ToLower(span, state._culture));


This is capturing pos and cch. You need to use the one that's in state.

stephentoub

One comment that needs to be addressed, otherwise LGTM.

ViktorHofer · 2018-06-29T14:12:47Z

Thanks a lot!

danmoseley · 2018-06-29T14:14:52Z

Is there a missing test, based on your last commit here

ViktorHofer · 2018-06-29T14:20:08Z

No, that's an internal behavior that can't be captured by a test. Not using the locals from the transferred state didn't cause an error but probably has diminished the perf improvements.

…r comments (dotnet/corefx#30632) * RegexParser & optionsstack ref * Add test coverage for Group.Synchronized * Adjust options mode test case * Add inline comment '#' test branch * Add comments * Replace manual ToLower calls by Span.ToLower * Make applicable fields readonly in parser * Change to Assert to reduce an if check in one branch * Code formatting * Avoid string allocation when IgnoreCase set Prefix patterns which are passed to RegexBoyerMoore are already lowercased by the parser. Remove the redundant ToLower() call and assert the patterns lowercase state * Add surrogate pair positive & negative tests * Add test cases for rtl anchor Commit migrated from dotnet/corefx@2f259de

ViktorHofer added 10 commits June 23, 2018 16:13

RegexParser & optionsstack ref

384cc6c

Add test coverage for Group.Synchronized

fbfcda6

Adjust options mode test case

a52d8f7

Add inline comment '#' test branch

948217e

Add comments

1765741

Change runtime check to Assert in Parser

6cc6da6

Replace manual ToLower calls by Span.ToLower

ea7161b

Make applicable fields readonly in parser

1059f3f

Change to Assert to reduce an if check in one branch

c1fa82b

Code formatting

298c603

ViktorHofer self-assigned this Jun 24, 2018

ViktorHofer added the area-System.Text.RegularExpressions label Jun 24, 2018

ViktorHofer modified the milestone: Future Jun 24, 2018

jkotas reviewed Jun 24, 2018

View reviewed changes

ViktorHofer added 4 commits June 25, 2018 00:50

Revert RegexOptions rename in parser

67d8a24

Avoid string allocation when IgnoreCase set

f84c8f3

Prefix patterns which are passed to RegexBoyerMoore are already lowercased by the parser. Remove the redundant ToLower() call and assert the patterns lowercase state

Remove wrong test case

0c7947a

Add surrogate pair positive & negative tests

7ab140e

ViktorHofer force-pushed the RegAlloc branch from 3f95036 to 7ab140e Compare June 24, 2018 22:50

Remove wrong test case

d15bac9

ViktorHofer force-pushed the RegAlloc branch from bac08eb to efab43f Compare June 25, 2018 15:22

ViktorHofer force-pushed the RegAlloc branch from efab43f to 95878d6 Compare June 25, 2018 15:23

jkotas reviewed Jun 25, 2018

View reviewed changes

ViktorHofer force-pushed the RegAlloc branch from 95878d6 to 557aff3 Compare June 25, 2018 16:00

Add test cases for rtl anchor

4f31b01

Add tests for rtl anchored patterns.

ViktorHofer force-pushed the RegAlloc branch from c6ee926 to 4f31b01 Compare June 25, 2018 16:25

stephentoub reviewed Jun 29, 2018

View reviewed changes

stephentoub approved these changes Jun 29, 2018

View reviewed changes

Use state locals in RegexParser

0e114e6

ViktorHofer merged commit 2f259de into dotnet:master Jun 29, 2018

ViktorHofer deleted the RegAlloc branch June 29, 2018 15:47

karelz added this to the 3.0 milestone Jul 8, 2018

soverkaer mentioned this pull request Feb 19, 2020

Regex "^aA$" does not match "aA" for danish culture using IgnoreCase in 3.1 dotnet/runtime#32534

Closed

stephentoub mentioned this pull request Sep 21, 2021

Fix regression in Regex lowercasing in parser dotnet/runtime#59434

Merged

Conversation

ViktorHofer commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before (master)

After

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jun 24, 2018

Uh oh!

ViktorHofer commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 26, 2018

Uh oh!

danmoseley commented Jun 27, 2018

Uh oh!

jkotas commented Jun 27, 2018

Uh oh!

MarcoRossignoli commented Jun 28, 2018

Uh oh!

ViktorHofer commented Jun 28, 2018

Uh oh!

MarcoRossignoli commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 24, 2018 •

edited

Loading

ViktorHofer Jun 24, 2018 •

edited

Loading

jkotas Jun 24, 2018 •

edited

Loading

ViktorHofer Jun 24, 2018 •

edited

Loading

ViktorHofer commented Jun 24, 2018 •

edited

Loading

MarcoRossignoli commented Jun 28, 2018 •

edited

Loading

ViktorHofer commented Jun 29, 2018 •

edited

Loading