Regex refactoring #27473

ViktorHofer · 2018-02-26T16:26:11Z

From the previous PR with the changes now grouped into multiple commits. No squash please!

@stephentoub I will incorporate your feedback here later today.

danmoseley · 2018-02-26T18:00:57Z

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Match.cs

+        /// <summary>
+        /// Returns an empty Match object.
+        /// </summary>
+        public static Match Empty => new Match(null, 1, string.Empty, 0, 0, 0);


I think this is a mistake. Previously it would only make a singleton empty match object, now it's making one every time it's requested.

Happy to see a test is failing for this reason: NextMatch_EmptyMatch_ReturnsEmptyMatch

To speed things up I pushed this change.

Correction, I didn't as I don't have permissions. It does fix the test though.

Right, this need to be { get; } = instead of =>

danmoseley · 2018-02-26T18:08:39Z

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCharClass.cs


            _canonical = true;
-            _rangelist.Sort(SingleRangeComparer.Instance);
+            _rangelist.Sort(SingleRangeComparer.s_instance);


Style wise I don't know how we feel about directly accessing fields, even of nested classes.

Oh I see we were already doing it, you renamed. Fine.

danmoseley

LGTM if you fix the Match.Empty mistake.

Also please do a sanity test of some perf scenario.

I agree let's not squash this.

stephentoub · 2018-02-27T14:56:20Z

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/CaptureCollection.cs

        int IList<Capture>.IndexOf(Capture item)
        {
-            var comparer = EqualityComparer<Capture>.Default;
+            EqualityComparer<Capture> comparer = EqualityComparer<Capture>.Default;


Separately, I would suggest looking at not storing this into a local first. With devirtualization support for EqualityComparer.Default, this might be better as:

for (int i = 0; i < Count; i++) { if (EqualityComparer<Capture>.Default.Equals(this[i], item) { return i; } } return -1;

There's also a lot of code behind the seemingly innocent this[i]; something else to look at separately.

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Group.cs

stephentoub · 2018-02-27T15:09:44Z

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

        public static readonly TimeSpan InfiniteMatchTimeout = Timeout.InfiniteTimeSpan;

-        // timeout for the execution of this regex
+        // timeout for the execution of this regex.


Nit: it's strange to me to see a period added at the end, when it's not a full sentence and doesn't begin with a capital

stephentoub · 2018-02-27T15:11:19Z

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

+                if (caps == null)
+                {
+                    caps = new Hashtable(value);
+                }


Nit: this (and other cases like it) can be simplified, e.g.

caps = value as Hashtable ?? new Hashtable(value);

stephentoub · 2018-02-27T15:12:11Z

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

            return RegexCompiler.Compile(code, roptions);
        }
+
+        [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "assemblyname", Justification = "Microsoft: already shipped since v1 - can't fix without causing a breaking change")]


Do we run these FxCop naming rules still? I'm wondering if all of these SuppressMessage attributes can just be removed.

I think you are right, we can remove them all.

stephentoub · 2018-02-27T15:17:04Z

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCharClass.cs

+                sb.Append(s_hex[(ch >> shift) & 0xF]);
            }

            return sb.ToString();


I assume this CharDescription code does not need to be efficient? (It's not ;)

tell me more please :) We loose a lot of performance in RegexCharClass.

We loose a lot of performance in RegexCharClass.

I was referring specifically to these "Description" methods. They're used in common code paths in release builds?

you're right, that method is only used in DEBUG code.

stephentoub · 2018-02-27T15:17:46Z

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCharClass.cs

        private sealed class SingleRangeComparer : IComparer<SingleRange>
        {
-            public static readonly SingleRangeComparer Instance = new SingleRangeComparer();
+            public static readonly SingleRangeComparer s_instance = new SingleRangeComparer();


Nit: This is a public field, so we generally have used PascalCasing. It just happens to be on a private type.

Maybe we should adjust our .editorconfig file as VS recommends such a change.

Changing to internal would be non breaking?

On a private class

stephentoub · 2018-02-27T21:47:59Z

No squash please!

Are you going to do any squashing prior to merging? There are a huge number of commits in this PR, and many of them don't need to stand on their own.

ViktorHofer · 2018-02-27T21:51:11Z

Yes I will squash the appropriate ones before merging. E.g. code cleanups together, split of Regex class into subfiles together, Span for Match & Capture

stephentoub · 2018-02-27T21:52:17Z

Thanks.

ViktorHofer · 2018-02-27T22:58:44Z

Thanks.

Done.

Also please do a sanity test of some perf scenario.

Just done locally. Results are the same for ctor construction and Match. Replace could be slightly faster because we use Slice instead of Substring now.

I will merge this when CI turns green.

Move Cache related code to Regex.Cache.cs Move Match related code to Regex.Match.cs Move Replace related code to Regex.Replace.cs Move Split related code to Match.Split.cs Move Timeout related code to Regex.Timeout.cs

Code cleanup in RegexBoyerMoore Code Cleanup in RegexCharClass RegexFCD code cleanup Code cleanup & limit visibility of helper methods in RegexNode Code cleanup in RegexReplacement Code cleanup in RegexTree Group code cleanup Match code cleanup Capture code cleanup Fix Match.Empty initalization to only compute ones Regex code cleanup and comments added

ViktorHofer · 2018-02-28T00:03:51Z

@dotnet-bot test this

System\Text\RegularExpressions\Capture.cs(54,64): error CS1501: No overload for method 'AsReadOnlySpan' takes 2 arguments [D:\j\workspace\windows-TGrou---f8ac6754\src\System.Text.RegularExpressions\src\System.Text.RegularExpressions.csproj]
System\Text\RegularExpressions\Capture.cs(59,65): error CS1501: No overload for method 'AsReadOnlySpan' takes 2 arguments [D:\j\workspace\windows-TGrou---f8ac6754\src\System.Text.RegularExpressions\src\System.Text.RegularExpressions.csproj]
System\Text\RegularExpressions\Match.cs(149,25): error CS1501: No overload for method 'AsReadOnlySpan' takes 2 arguments [D:\j\workspace\windows-TGrou---f8ac6754\src\System.Text.RegularExpressions\src\System.Text.RegularExpressions.csproj]

No idea what's going on here. AFAIK AsReadOnlySpan takes 2 arguments... Ah it is renamed to ToSpan: 913d11e

Use Span in Match and avoid intermidiate string allocation

danmoseley · 2018-02-28T06:43:29Z

MetadataReaderTests failed on OSX10.12 with BIFException. I"ll open an issue if that proves to be more than a build glitch.

danmoseley · 2018-02-28T06:43:49Z

I nearly squashed...

danmoseley · 2018-02-28T06:44:42Z

@stephentoub I overlooked that you hadn't hit signoff.

stephentoub · 2018-02-28T06:45:58Z

I overlooked that you hadn't hit signoff.

I guess that means I have a get-out-of-jail-free card if this causes everything to go haywire.

…() (dotnet#27473) * Revert removal of SuppressGCTransition from SystemNative_GetTimestamp() * Insert GC_POLL before statement with unmanaged call. * JIT test for insertion of GCPoll Signed-off-by: dotnet-bot <[email protected]>

…() (#27473) * Revert removal of SuppressGCTransition from SystemNative_GetTimestamp() * Insert GC_POLL before statement with unmanaged call. * JIT test for insertion of GCPoll Signed-off-by: dotnet-bot <[email protected]>

ViktorHofer added the area-System.Text.RegularExpressions label Feb 26, 2018

ViktorHofer self-assigned this Feb 26, 2018

ViktorHofer requested a review from danmoseley February 26, 2018 16:26

danmoseley reviewed Feb 26, 2018

View reviewed changes

danmoseley approved these changes Feb 26, 2018

View reviewed changes

danmoseley added the * NO SQUASH * The PR should not be squashed label Feb 26, 2018

stephentoub reviewed Feb 27, 2018

View reviewed changes

src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Group.cs Show resolved Hide resolved

stephentoub reviewed Feb 27, 2018

View reviewed changes

ViktorHofer force-pushed the RegexRefa branch from 13eba3c to 4407742 Compare February 27, 2018 15:51

ViktorHofer force-pushed the RegexRefa branch 2 times, most recently from f339978 to 7716503 Compare February 27, 2018 22:47

danmoseley mentioned this pull request Feb 27, 2018

Improve Regex cache speed when cache is large #27278

Merged

ViktorHofer added 8 commits February 28, 2018 01:02

Move Regex helper files to System.Collections dir

677eaf8

Rename Regex files to match class name & sort Includes in csproj

ee919a4

Split Regex code into subfiles

e420872

Move Cache related code to Regex.Cache.cs Move Match related code to Regex.Match.cs Move Replace related code to Regex.Replace.cs Move Split related code to Match.Split.cs Move Timeout related code to Regex.Timeout.cs

Move Regex Shared/Exclusive Reference to own file

b837e28

Inline EqualityComparer and avoid local in CaptureCollection

481ea19

Avoid discard in Group

ae7ab84

Limit access modifers throughout the Regex codebase

aac160e

ViktorHofer force-pushed the RegexRefa branch from 7716503 to 6bbdbbe Compare February 28, 2018 00:56

ViktorHofer added 2 commits February 28, 2018 02:12

Use Span in Capture and avoid intermidiate string allocation

1d2d885

Use Span in Match and avoid intermidiate string allocation

Volatile and readonly in Reference helper classes in Regex

072036f

ViktorHofer force-pushed the RegexRefa branch from 6bbdbbe to 072036f Compare February 28, 2018 01:12

danmoseley merged commit 28e3939 into dotnet:master Feb 28, 2018

ViktorHofer deleted the RegexRefa branch February 28, 2018 10:45

karelz added this to the 2.1.0 milestone Mar 10, 2018

Regex refactoring #27473

Regex refactoring #27473

Uh oh!

Conversation

ViktorHofer commented Feb 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 27, 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!

danmoseley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 27, 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!

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 Feb 27, 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!

stephentoub commented Feb 27, 2018

Uh oh!

ViktorHofer commented Feb 27, 2018

Uh oh!

stephentoub commented Feb 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

danmoseley commented Feb 28, 2018

Uh oh!

danmoseley commented Feb 28, 2018

Uh oh!

danmoseley commented Feb 28, 2018

Uh oh!

stephentoub commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

ViktorHofer commented Feb 26, 2018 •

edited

Loading

stephentoub Feb 27, 2018 •

edited

Loading

stephentoub Feb 27, 2018 •

edited

Loading

ViktorHofer Feb 27, 2018 •

edited

Loading

ViktorHofer commented Feb 27, 2018 •

edited

Loading

ViktorHofer commented Feb 28, 2018 •

edited

Loading