-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Optimize Ascii.Equals for small values #93191
Conversation
- 64bit swar - Improve 128 widening on avx and remove unused Vector64 widen - Mask the ascii check into the same cmp
Tagging subscribers to this area: @dotnet/area-system-text-encoding Issue DetailsAn attempt to optimize Ascii.Equals for smaller values Changes include:
Now that the code is shared across the different vector sizes now, it should be easy to convert to ISimdVector :) but it doesn't have any widening methods yet. Benchmarks:
Benchmark code: just added different sizes to System.Text.Perf_Ascii in dotnet/performance
|
@gfoidl would you be interested in reviewing this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a bug in the case Vector128.IsHardwareAccelerated = false
-- see comments for rationale.
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Equality.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming CI will pass --> LGTM
Updated benchmarks. Note that the regressions comes mainly from different codegen when returning the bool condition vs true/false. See comment #93191 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
yield return new object[] { new string(i, i), string.Create(i, i, (destination, iteration) => | ||
if (chr != '?') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment was valuable, please restore it
if (chr != '?') | |
if (chr != '?') // ASCIIEncoding maps invalid ASCII to ? |
public static IEnumerable<object[]> ValidAsciiInputs | ||
{ | ||
get | ||
{ | ||
yield return new object[] { "test" }; | ||
|
||
for (char textLength = (char)0; textLength <= 127; textLength++) | ||
foreach (int textLength in BufferLengths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a lot of test cases, I am not sure how long it's going to take to run all of them with debug builds.
Before your change, this test was covering strings that were 0 to 127 chars long. Now the max length is reduced to 33. I am not convinced that this is the right thing to do, please revert this particular change or convince me that it's the right thing to do.
@@ -55,15 +75,18 @@ public static IEnumerable<object[]> DifferentInputs | |||
{ | |||
yield return new object[] { "tak", "nie" }; | |||
|
|||
for (char i = (char)1; i <= 127; i++) | |||
foreach (int textLength in BufferLengths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I don't see benefits of this change.
char left = i; | ||
char right = char.IsAsciiLetterUpper(left) ? char.ToLower(left) : char.IsAsciiLetterLower(left) ? char.ToUpper(left) : left; | ||
yield return new object[] { new string(left, i), new string(right, i) }; | ||
for (char chr = (char)0; chr <= 127; chr++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the value of this change, but we should decrease the number of test cases. How about just focusing on Ascii letters? Because for other characters we would be duplicating other tests work.
This pull request has been automatically marked |
This pull request will now be closed since it had been marked |
An attempt to optimize Ascii.Equals for smaller values
Changes include:
Now that the code is shared across the different vector sizes now, it should be easy to convert to ISimdVector :) but it doesn't have any widening methods yet.
Benchmarks:
Benchmark code: just added different sizes to System.Text.Perf_Ascii in dotnet/performance
dotnet run -c Release -f net8.0 --filter System.Text.Perf_Ascii.Equals_* --memoryRandomization --launchCount 5 --corerun ... --artifacts ...