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

Fix string comparison with ordinal casing with Surrogates #55771

Merged
merged 2 commits into from
Jul 16, 2021

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jul 15, 2021

This change is fixing the string comparisons with ordinal casing when using surrogate pairs.
I have measured the perf for ordinal casing comparisons in general to ensure there is no regression introduced because of this change.

.NET 6.0 Before changes

|                             Method |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|          SimpleAsciiNoCasingNeeded |  1.626 ns | 0.0603 ns | 0.0619 ns |     - |     - |     - |         - |
|            SimpleAsciiCasingNeeded | 31.667 ns | 0.3536 ns | 0.3135 ns |     - |     - |     - |         - |
|              NonSimpleCasingNeeded | 40.866 ns | 0.6449 ns | 0.9453 ns |     - |     - |     - |         - |
| NonSimpleCasingNeededWithSurrogate | 48.083 ns | 0.9228 ns | 0.8180 ns |     - |     - |     - |         - |

.NET 6.0 After changes

|                             Method |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|          SimpleAsciiNoCasingNeeded |  1.309 ns | 0.0251 ns | 0.0222 ns |     - |     - |     - |         - |
|            SimpleAsciiCasingNeeded | 30.796 ns | 0.3370 ns | 0.2988 ns |     - |     - |     - |         - |
|              NonSimpleCasingNeeded | 40.400 ns | 0.5521 ns | 0.5164 ns |     - |     - |     - |         - |
| NonSimpleCasingNeededWithSurrogate | 48.030 ns | 0.7178 ns | 0.6715 ns |     - |     - |     - |         - |

@ghost
Copy link

ghost commented Jul 15, 2021

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

Issue Details

This change is fixing the string comparisons with ordinal casing when using surrogate pairs.
I have measured the perf for ordinal casing comparisons in general to ensure there is no regression introduced because of this change.

.NET 6.0 Before changes

|                             Method |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|          SimpleAsciiNoCasingNeeded |  1.626 ns | 0.0603 ns | 0.0619 ns |     - |     - |     - |         - |
|            SimpleAsciiCasingNeeded | 31.667 ns | 0.3536 ns | 0.3135 ns |     - |     - |     - |         - |
|              NonSimpleCasingNeeded | 40.866 ns | 0.6449 ns | 0.9453 ns |     - |     - |     - |         - |
| NonSimpleCasingNeededWithSurrogate | 48.083 ns | 0.9228 ns | 0.8180 ns |     - |     - |     - |         - |

.NET 6.0 After changes

|                             Method |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|          SimpleAsciiNoCasingNeeded |  1.309 ns | 0.0251 ns | 0.0222 ns |     - |     - |     - |         - |
|            SimpleAsciiCasingNeeded | 30.796 ns | 0.3370 ns | 0.2988 ns |     - |     - |     - |         - |
|              NonSimpleCasingNeeded | 40.400 ns | 0.5521 ns | 0.5164 ns |     - |     - |     - |         - |
| NonSimpleCasingNeededWithSurrogate | 48.030 ns | 0.7178 ns | 0.6715 ns |     - |     - |     - |         - |
Author: tarekgh
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh tarekgh added this to the 6.0.0 milestone Jul 15, 2021
@tarekgh
Copy link
Member Author

tarekgh commented Jul 15, 2021

CC @safern

No rush for this one :-)

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tarekgh
Copy link
Member Author

tarekgh commented Jul 15, 2021

Thanks @safern

@tarekgh tarekgh merged commit 1d065b6 into dotnet:main Jul 16, 2021
@tarekgh tarekgh deleted the FixOrdinalCasingBehaviorWithSurrogates branch July 16, 2021 16:26
@ghost ghost locked as resolved and limited conversation to collaborators Aug 15, 2021
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.

2 participants