Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IndexOfAnyValues in CompareInfo.Icu.cs #79787

Merged
merged 1 commit into from
Dec 30, 2022

Conversation

MihaZupan
Copy link
Member

Contributes to #78204

@MihaZupan MihaZupan added this to the 8.0.0 milestone Dec 17, 2022
@ghost ghost assigned MihaZupan Dec 17, 2022
@ghost
Copy link

ghost commented Dec 17, 2022

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
Copy link
Member

jkotas commented Dec 17, 2022

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
Copy link
Member

tarekgh commented Dec 17, 2022

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
Copy link
Member Author

MihaZupan commented Dec 17, 2022

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
Copy link
Member Author

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
Copy link
Member

tarekgh commented Dec 17, 2022

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
Copy link
Member Author

MihaZupan commented Dec 17, 2022

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
Copy link
Member

jkotas commented Dec 19, 2022

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
Copy link
Member Author

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
Copy link
Member Author

Failures are #79453 and #79874

@ghost ghost locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants