Regex parser nits

[danmoseley]

trivial changes.

1. I ran the real world patterns corpus through the parser and didn't notice anything that could be obviously improved except for one dictionary which was almost always resizing. Set size to eliminate resizing in 90% of cases.
2. Inlined some more low value helpers in RegexParser where I think it makes it clearer.
3. Marked methods readonly where our analyzer instructed it.
4. Some use of IndexOf() per feedback in Inline more helpers in RegexParser #88392

[ghost]

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

Issue Details

---

trivial changes.

1. I ran the real world patterns corpus through the parser and didn't notice anything that could be obviously improved except for one dictionary which was almost always resizing. Set size to eliminate resizing in 90% of cases.
2. Inlined some more low value helpers in RegexParser where I think it makes it clearer.
3. Marked methods readonly where our analyzer instructed it.

Author:	danmoseley
Assignees:	-
Labels:	area-System.Text.RegularExpressions
Milestone:	-

[danmoseley]

removed these asserts because there'll be a null ref a couple lines down in each case; we don't put asserts in front of every possible null ref.

[buyaa-n]

Not sure I understood the comment, but it looks to me that these are asserting The current reading position not a null ref.

[danmoseley]

I should have said IndexOutOfRangeException

[stephentoub]

we don't put asserts in front of every possible null ref.

No, but we do put asserts in places that are meant to act as preconditions / contracts. These asserts make it really easy for a maintainer to see that these methods should only be called when there's pattern remaining. I don't think removing them was valuable.

[danmoseley]

Test failures unrelated

[stephentoub]

I ran the real world patterns corpus through the parser and didn't notice anything that could be obviously improved except for one dictionary which was almost always resizing. Set size to eliminate resizing in 90% of cases.

I'm not seeing it... which line is the change to presize the dictionary?

[danmoseley]

I'm not seeing it... which line is the change to presize the dictionary?

Thanks for spotting that -- it got lost in a rebase, apparently:
https://github.com/dotnet/runtime/compare/main...danmoseley:regex.presize?expand=1

[stephentoub]

Thanks for spotting that -- it got lost in a rebase, apparently: https://github.com/dotnet/runtime/compare/main...danmoseley:regex.presize?expand=1

_stringTable = new Dictionary<string, int>(15); // avoids resize for 90% of real world patterns

How much of an overallocation is it for those? i.e. we could probably avoid it for 100% of the real-world patterns if we made it new Dictionary<string, int>(1_000_000), but that's obviously a bad answer.

Said another way, what does the histogram look like for number of strings involved in the various patterns?

[danmoseley]

https://gist.github.com/danmoseley/eeb38412d74c3eb22bc69472940b1f95

Note horizontal scale starts skipping around 60

[danmoseley]

For something like this, it would be nice if the dictionary could start with on stack buffers.

[danmoseley]

If the dictionary starts at zero, resizes when full to the next prime at least 2x the size, then it will have sizes 0, 3, 7, 17, 37, 89

61% would fit in 3, 80% in 7 and 96% in 17. Only 5% are zero and would not allocate at all.

0	12647	0.05	3
1	62735	0.31	3
2	43475	0.50	3
3	27852	0.61	3
4	17408	0.69	7
5	11870	0.74	7
6	8788	0.77	7
7	7623	0.80	7
8	7035	0.83	17
9	6381	0.86	17
10	5069	0.88	17
11	4178	0.90	17
12	3397	0.91	17
13	3023	0.93	17
14	2840	0.94	17
15	2243	0.95	17
16	2103	0.96	17
17	1800	0.96	17
18	1169	0.97	37
19	1360	0.97	37
20	1157	0.98	37
21	925	0.98	37
22	828	0.99	37
23	468	0.99	37
24	425	0.99	37
25	314	0.99	37
26	264	0.99	37
27	240	0.99	37
28	293	0.99	37
29	188	0.99	37
30	38	0.99	37
31	86	1.00	37

[stephentoub]

61% would fit in 3, 80% in 7 and 96% in 17. Only 5% are zero and would not allocate at all.

Right. So presizing to 15 would overallocate for the majority.

[danmoseley]

I guess it depends how you trade off a small transient allocation with a small CPU cost of copying. I do not think this is a hot path when aggregated with actually running the interpreter. But it seemed that we have some hard data here and it was easy to avoid that CPU cost in 90% of cases. I don't mind either way.

[stephentoub]

Thanks.

But it seemed that we have some hard data here and it was easy to avoid that CPU cost in 90% of cases.

There's CPU cost associated with overallocation as well.

I don't think presizing it to 15 is the right tradeoff.