Tweak overheads of Regex cache access

[stephentoub]

Small improvement, but then the cited regression was also small. I tried a few other things (e.g. passing the Key by in, storing an int milliseconds timeout instead of the TimeSpan, etc.), but they didn't help, and I'm not seeing a ton more that can be done. Closes #50051.

This is on 64-bit:

Method	Toolchain	total	unique	cacheSize	Mean	Error	StdDev	Median	Min	Max	Ratio
IsMatch	\main\corerun.exe	40000	1600	3200	11.26 ms	0.099 ms	0.083 ms	11.24 ms	11.08 ms	11.40 ms	1.00
IsMatch	\pr\corerun.exe	40000	1600	3200	11.08 ms	0.177 ms	0.165 ms	11.06 ms	10.87 ms	11.40 ms	0.99
IsMatch	\main\corerun.exe	400000	1	15	43.40 ms	0.285 ms	0.238 ms	43.37 ms	43.00 ms	43.85 ms	1.00
IsMatch	\pr\corerun.exe	400000	1	15	41.89 ms	0.215 ms	0.180 ms	41.84 ms	41.66 ms	42.37 ms	0.97
IsMatch	\main\corerun.exe	400000	7	15	57.78 ms	0.367 ms	0.343 ms	57.65 ms	57.30 ms	58.45 ms	1.00
IsMatch	\pr\corerun.exe	400000	7	15	54.56 ms	0.277 ms	0.259 ms	54.54 ms	54.13 ms	55.02 ms	0.94

[ghost]

Tagging subscribers to this area: @eerhardt, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Small improvement, but then the cited regression was also small. I tried a few other things (e.g. passing the Key by in, storing an int milliseconds timeout instead of the TimeSpan, etc.), but they didn't help, and I'm not seeing a ton more that can be done. Closes #50051.

This is on 64-bit:

Method	Toolchain	total	unique	cacheSize	Mean	Error	StdDev	Median	Min	Max	Ratio
IsMatch	\main\corerun.exe	40000	1600	3200	11.26 ms	0.099 ms	0.083 ms	11.24 ms	11.08 ms	11.40 ms	1.00
IsMatch	\pr\corerun.exe	40000	1600	3200	11.08 ms	0.177 ms	0.165 ms	11.06 ms	10.87 ms	11.40 ms	0.99
IsMatch	\main\corerun.exe	400000	1	15	43.40 ms	0.285 ms	0.238 ms	43.37 ms	43.00 ms	43.85 ms	1.00
IsMatch	\pr\corerun.exe	400000	1	15	41.89 ms	0.215 ms	0.180 ms	41.84 ms	41.66 ms	42.37 ms	0.97
IsMatch	\main\corerun.exe	400000	7	15	57.78 ms	0.367 ms	0.343 ms	57.65 ms	57.30 ms	58.45 ms	1.00
IsMatch	\pr\corerun.exe	400000	7	15	54.56 ms	0.277 ms	0.259 ms	54.54 ms	54.13 ms	55.02 ms	0.94

Author:	stephentoub
Assignees:	-
Labels:	area-System.Text.RegularExpressions
Milestone:	6.0.0

[stephentoub]

@CertifiedRice, you "approved" about 25 PRs in rapid succession. Did you actually review them? Can you help me understand what action you took that led you to "approve" them? Thanks.

[eerhardt]

Looks good to me.

[eerhardt]

For my knowledge - did this line make a perf difference? Or does it just read better this way?

[stephentoub]

It did for an intermediate stage where I was experimenting with passing key as in. I ended up reverting that, but left this as it was as I thought it still read slightly better.

[pgovind]

The code change LGTM. But, I don't really get why this is performing slightly better :/

[eerhardt]

But, I don't really get why this is performing slightly better :/

I'll take a stab at answering, and @stephentoub can correct me 😄. It basically comes to 2 micro-optimizations:

1. We are no longer considering _culture.GetHashCode() in the hash code of the Key.
2. Even a smaller optimization is that we don't need to pass back 2 values from TryGet - a bool and a Regex. Instead, we are just passing back the Regex. Honestly, I'm not sure myself exactly why that part makes a measurable change....

[pgovind]

We are no longer considering _culture.GetHashCode() in the hash code of the Key.

Yup, I noticed that part. We're reducing the amount of computation here, so this makes sense.

that we don't need to pass back 2 values from TryGet - a bool and a Regex.

This is the part that I'm surprised by. The returned bool was on the stack, so I can't think of any reason why this would make a measurable change. Maybe when there's some down time I'll look at the IL generated here and/or measure how much this change contributed to the overall speedup. I'm guessing all of the speedup here is from point 1?

[eerhardt]

Doing a really small example:

https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgAwAEmuAdACoCmAHglQEp0DmArgDYQBOAUQYAHPnRQooAexgoA3ESKYAzBWxkAwmQDeRMvrKioANwgI6FXADYybdozIoA+rxQIAggGNP4lHQAmCoQGZHoGKrYcDgDidAgAFACUOmEh+lAAZmTxzq4e3r4BZFAokfYMZGLlialpusFpjZgA7JVRDEGN+gC+tSEtZDA83J0GvYS1EcBSUtxkNHwAnrEJUpwIZQ58yfVdxVk5Lqj5PhJFJZsVVYw1DY27ewZ8ZAC8beWjjxStCHycdJ80uM9s83kNuCM+uFWhkINw/IDxt0gA

C.Get()
    L0000: mov ecx, 0x17e5c5f0
    L0005: xor edx, edx
    L0007: call 0x5c2ffde0
    L000c: mov eax, [eax]
    L000e: test eax, eax
    L0010: je short L0013
    L0012: ret
    L0013: xor eax, eax
    L0015: ret

C.TryGet(System.Text.RegularExpressions.Regex ByRef)
    L0000: push esi
    L0001: mov esi, edx
    L0003: mov ecx, 0x17e5c5f0
    L0008: xor edx, edx
    L000a: call 0x5c2ffde0
    L000f: mov eax, [eax]
    L0011: test eax, eax
    L0013: je short L0023
    L0015: mov edx, esi
    L0017: call 0x5c1d7098
    L001c: mov eax, 1
    L0021: pop esi
    L0022: ret
    L0023: xor eax, eax
    L0025: mov [esi], eax
    L0027: pop esi
    L0028: ret

[stephentoub]

There's a difference between handing back an object via a return and handing back an object via a ref... the latter requires a write barrier:

; Program.ByReturn()
       mov       rax,[rcx+8]
       ret
; Total bytes of code 5

; Program.ByOut(System.Object ByRef)
       mov       rax,rdx
       mov       rdx,[rcx+8]
       mov       rcx,rax
       call      CORINFO_HELP_CHECKED_ASSIGN_REF
       nop
       ret
; Total bytes of code 17

Our Try pattern goals afoul of this for usability reasons, but you can see for example as a minor optimization our internal abstraction in LINQ switches the bool/T positions so that when T is a reference type it's returned rather than passed out by ref:

runtime/src/libraries/System.Linq/src/System/Linq/IPartition.cs

Lines 33 to 47 in 7e65185

	TElement? TryGetElementAt(int index, out bool found);
	/// <summary>
	/// Gets the first item in this sequence.
	/// </summary>
	/// <param name="found"><c>true</c> if the sequence contains an element, <c>false</c> otherwise.</param>
	/// <returns>The element if <paramref name="found"/> is <c>true</c>, otherwise, the default value of <typeparamref name="TElement"/>.</returns>
	TElement? TryGetFirst(out bool found);
	/// <summary>
	/// Gets the last item in this sequence.
	/// </summary>
	/// <param name="found"><c>true</c> if the sequence contains an element, <c>false</c> otherwise.</param>
	/// <returns>The element if <paramref name="found"/> is <c>true</c>, otherwise, the default value of <typeparamref name="TElement"/>.</returns>
	TElement? TryGetLast(out bool found);