Impove StringComparer.OrdinalIgnoreCase.Equals

[EgorBo]

Closes #86999

Unifies OrdinalIgnoreCaseComparer with OrdinalCaseSensitiveComparer to just call String's APIs.

static bool Test(string s) => 
    StringComparer.OrdinalIgnoreCase.Equals(s, "hello");

; Assembly listing for method Program:Test(System.String):bool
       test     rcx, rcx
       je       SHORT G_M18217_IG05
       cmp      dword ptr [rcx+08H], 5
       jne      SHORT G_M18217_IG05
       mov      rax, 0x20002000200020
       or       rax, qword ptr [rcx+0CH]
       mov      rdx, 0x6C006C00650068
       xor      rax, rdx
       mov      edx, dword ptr [rcx+12H]
       or       edx, 0x200020
       xor      edx, 0x6F006C
       or       rax, rdx
       sete     al
       movzx    rax, al
       jmp      SHORT G_M18217_IG06
G_M18217_IG05:  ;; offset=0040H
       xor      eax, eax
G_M18217_IG06:  ;; offset=0042H
       movzx    rax, al
       ret      
; Total bytes of code 70

[EgorBo]

Unfortunately, we hit here exactly the same problem that we faced with Encoding.UTF8 -- it needs Tiered compilation + PGO to propertly inline StringComparer.OrdinalIgnoreCase.Equals.

[jkotas]

The most common use of these APIs is for non-constant strings in collections or sorting. Are there any changes in the codegen or perf due to this change in the common case?

[EgorBo]

The most common use of these APIs is for non-constant strings in collections or sorting. Are there any changes in the codegen or perf due to this change in the common case?

Ideally, it should be exactly the same codegen, but currently it is not, to have the same codegen we need to make string.Equals(x, y, StringComparison.OrdinalIgnoreCase) inlineable so the internal switch over compare mode is elided 🤔

[EgorBo]

runtime/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs

Lines 704 to 742 in 9c4d51c

	public static bool Equals(string? a, string? b, StringComparison comparisonType)
	{
	if (object.ReferenceEquals(a, b))
	{
	CheckStringComparison(comparisonType);
	return true;
	}
	if (a is null || b is null)
	{
	CheckStringComparison(comparisonType);
	return false;
	}
	switch (comparisonType)
	{
	case StringComparison.CurrentCulture:
	case StringComparison.CurrentCultureIgnoreCase:
	return CultureInfo.CurrentCulture.CompareInfo.Compare(a, b, GetCaseCompareOfComparisonCulture(comparisonType)) == 0;
	case StringComparison.InvariantCulture:
	case StringComparison.InvariantCultureIgnoreCase:
	return CompareInfo.Invariant.Compare(a, b, GetCaseCompareOfComparisonCulture(comparisonType)) == 0;
	case StringComparison.Ordinal:
	if (a.Length != b.Length)
	return false;
	return EqualsHelper(a, b);
	case StringComparison.OrdinalIgnoreCase:
	if (a.Length != b.Length)
	return false;
	return EqualsOrdinalIgnoreCaseNoLengthCheck(a, b);
	default:
	throw new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType));
	}
	}

and CheckStringComparison should also be folded to no-op.

Presumably, this API is mostly called with a constant string comparison mode?

[EgorBo]

@jkotas so do you have any preference of what we can do here?

1. Mark string.Equals(string,string,StringComparison) as aggressive inline (it's not inlined due to "too many IL bytes")
2. Leave as is (this PR) because the other OrdinalCaseSensitiveComparer already just transfers the job to string.Equals today
3. Use IsKnownConstant
4. Give up

https://grep.app/search?q=StringComparer.OrdinalIgnoreCase.Equals

[jkotas]

Mark string.Equals(string,string,StringComparison) as aggressive inline

I do not think we would want to mark the current implementation as aggressive inline. It would eat the inlining budget too fast. This API is used a lot and it is not unusual to see it called multiple times from a single method.

I think it may be ok to make an assumption that this API is almost always called with a constant StringComparison argument and refactor the implementation to optimize for this case: The public method that has just a switch over StringComparison enum and calls worker method for each comparison type and invalid input. This public method can be marked as aggressive inlining if it does not get inlined naturally.

There are number of methods in CoreLib that take StringComparison and where this optimization can be potentially applied to.

[jkotas]

Leave as is (this PR) because the other OrdinalCaseSensitiveComparer already just transfers the job to string.Equals today

I think it would be a regression on average. I would expect that these comparers are much more likely to be used with collections. I think that the examples from your query are minority. In fact, we may want to have an analyzer that recommends replacing calls of virtual Comparer to more direct string.Equals calls that are smaller and easier to optimize.

[ghost]

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

Issue Details

---

Closes #86999

Unifies OrdinalIgnoreCaseComparer with OrdinalCaseSensitiveComparer to just call String's APIs.

static bool Test(string s) => 
    StringComparer.OrdinalIgnoreCase.Equals(s, "hello");

; Assembly listing for method Program:Test(System.String):bool
       test     rcx, rcx
       je       SHORT G_M18217_IG05
       cmp      dword ptr [rcx+08H], 5
       jne      SHORT G_M18217_IG05
       mov      rax, 0x20002000200020
       or       rax, qword ptr [rcx+0CH]
       mov      rdx, 0x6C006C00650068
       xor      rax, rdx
       mov      edx, dword ptr [rcx+12H]
       or       edx, 0x200020
       xor      edx, 0x6F006C
       or       rax, rdx
       sete     al
       movzx    rax, al
       jmp      SHORT G_M18217_IG06
G_M18217_IG05:  ;; offset=0040H
       xor      eax, eax
G_M18217_IG06:  ;; offset=0042H
       movzx    rax, al
       ret      
; Total bytes of code 70

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-System.Runtime
Milestone:	-

[EgorBo]

Leave as is (this PR) because the other OrdinalCaseSensitiveComparer already just transfers the job to string.Equals today

I think it would be a regression on average. I would expect that these comparers are much more likely to be used with collections. I think that the examples from your query are minority. In fact, we may want to have an analyzer that recommends replacing calls of virtual Comparer to more direct string.Equals calls that are smaller and easier to optimize.

Ok, I agree. As I don't see how we can make that String.Eqauls overload inlineable without the [AI] attribute and satisfy conservative's inliner limitation on number of basic blocks