Use IndexOfAnyValues in CompareInfo.Icu.cs

[MihaZupan]

Contributes to #78204

[ghost]

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

Issue Details

---

Contributes to #78204

Author:	MihaZupan
Assignees:	-
Labels:	area-System.Globalization
Milestone:	8.0.0

[jkotas]

I expect that this is going to be regression for small strings and improvement for long strings. What do the performance numbers for string compare benchmarks look like?

[tarekgh]

I agree with @jkotas here. Could you please provide some benchmarks with short and long strings? note that the main scenarios usually are using short strings.

[MihaZupan]

The scalar path that used IndexOfAnyValues.Contains(char) ran into some horrible perf (e.g. 50% regression for long inputs, even after the benefit from vectorization on the target). I'll keep looking into that.

For now, I've reverted that part of the change, having this PR only replace the vectorizable part.
This code already has a pretty high per-call overhead.

Method	Toolchain	Length	ValueIsLonger	Mean	Ratio
IndexOf	main	1	False	21.16 ns	1.00
IndexOf	pr	1	False	19.54 ns	0.94
IndexOf	main	1	True	17.09 ns	1.00
IndexOf	pr	1	True	19.76 ns	1.16
IndexOf	main	6	False	26.01 ns	1.00
IndexOf	pr	6	False	29.17 ns	1.12
IndexOf	main	6	True	22.84 ns	1.00
IndexOf	pr	6	True	27.36 ns	1.20
IndexOf	main	30	False	66.52 ns	1.00
IndexOf	pr	30	False	59.58 ns	0.90
IndexOf	main	30	True	52.28 ns	1.00
IndexOf	pr	30	True	23.40 ns	0.45
IndexOf	main	10000	False	16,908.62 ns	1.00
IndexOf	pr	10000	False	12,022.45 ns	0.71
IndexOf	main	10000	True	12,906.77 ns	1.00
IndexOf	pr	10000	True	1,301.53 ns	0.10

[MihaZupan]

note that the main scenarios usually are using short strings.

In that case, there is still some low-hanging fruit here to lower the per-call overhead (e.g. forcing this to inline).

[tarekgh]

In that case, there is still some low-hanging fruit here to lower the per-call overhead (e.g. forcing this to inline).

Yes, sure. Is it possible we can check the string length and use the new changes with a longer string? I agree we can inline https://github.com/dotnet/runtime/blob/19488dd74f769007d510c74f96ccdb646c5027cc/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1096-L1099 anyway.

[MihaZupan]

I updated the numbers above after the inlining changes. It's now an improvement regardless of the input length. I split these changes out of this PR.

Is it possible we can check the string length and use the new changes with a longer string?

Are inputs shorter than 8 characters that common? I would prefer not to duplicate the logic to save the overhead of a method call.

[jkotas]

Is the AggressiveInlining going to bloat callsites of the public methods?

You can often make micro-benchmarks faster by AggressiveInlining, but it is not necessarily the right thing to do for real-world performance. When you are liberally applying AggressiveInlining, you have to also watch what it is going to do to code size that is an important performance metric as well.

[MihaZupan]

Is the AggressiveInlining going to bloat callsites of the public methods?

Not in this case, I was careful to only do so for internal helpers.
Public callers still call into IndexOf, and the internal helpers are inlined into it.

[MihaZupan]

Failures are #79453 and #79874

[EgorBo]

Improvements on win-x64:

• [Perf] Windows/x64: 3 Improvements on 12/30/2022 6:15:13 AM perf-autofiling-issues#11315
• [Perf] Linux/x64: 7 Improvements on 12/30/2022 6:15:13 AM perf-autofiling-issues#11279

linux-x64:

• [Perf] Alpine/x64: 1 Improvement on 12/30/2022 6:15:13 AM perf-autofiling-issues#11262