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

Optimize System.Text.ASCIIUtility for arm64 using cross-platform intrinsics #41292

Closed
1 of 2 tasks
jeffhandley opened this issue Aug 24, 2020 · 7 comments · Fixed by #70080 or #71637
Closed
1 of 2 tasks

Optimize System.Text.ASCIIUtility for arm64 using cross-platform intrinsics #41292

jeffhandley opened this issue Aug 24, 2020 · 7 comments · Fixed by #70080 or #71637
Assignees
Labels
arch-arm64 area-System.Text.Encoding Cost:M Work that requires one engineer up to 2 weeks Priority:3 Work that is nice to have
Milestone

Comments

@jeffhandley
Copy link
Member

jeffhandley commented Aug 24, 2020

This item tracks the remaining System.Text.ASCIIUtility methods that can be optimized for ARM64 using intrinsics. Many were completed in 5.0.0 with #35034, but two methods remain. Both of them had prior attempts to apply ARM64 intrinsics, but the efforts were not completed.

Update for .NET 7
Now that we have cross-platform intrinsics APIs (#49397), these optimizations should be completed using those APIs instead of adding ARM64-specific intrinsics code paths. The effort could optionally include first measuring performance of these methods with the ARM64-specific intrinsics in place, and then measuring the performance of these methods with the cross-platform intrinsics.

We previously attempted to implement these optimizations during .NET 5, but the draft PRs linked above were closed because they were introducing unexpected regressions. Therefore, the effort on these optimizations must include investigation of those pitfalls and finding an approach that simultaneously optimizes the code for ARM64 and avoids regressions elsewhere.

Example use of the new cross-platform intrinsics: #63722

@jeffhandley jeffhandley added this to the 6.0.0 milestone Aug 24, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Aug 24, 2020
@jeffhandley jeffhandley removed the untriaged New issue has not been triaged by the area owner label Aug 24, 2020
@ghost
Copy link

ghost commented Aug 25, 2020

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

@tarekgh
Copy link
Member

tarekgh commented Aug 25, 2020

CC @GrabYourPitchforks

@jeffhandley jeffhandley added the Priority:3 Work that is nice to have label Jan 14, 2021
@pgovind pgovind added the Cost:S Work that requires one engineer up to 1 week label Jan 15, 2021
@jeffhandley jeffhandley modified the milestones: 6.0.0, Future Apr 14, 2021
@jeffhandley jeffhandley changed the title Optimize remaining System.Text.ASCIIUtility methods using arm64 intrinsics Optimize remaining System.Text.ASCIIUtility methods for arm64 using cross-platform intrinsics Feb 23, 2022
@jeffhandley jeffhandley modified the milestones: Future, 7.0.0 Feb 23, 2022
@jeffhandley jeffhandley changed the title Optimize remaining System.Text.ASCIIUtility methods for arm64 using cross-platform intrinsics Optimize System.Text.ASCIIUtility methods for arm64 using cross-platform intrinsics Feb 23, 2022
@jeffhandley jeffhandley changed the title Optimize System.Text.ASCIIUtility methods for arm64 using cross-platform intrinsics Optimize System.Text.ASCIIUtility for arm64 using cross-platform intrinsics Feb 23, 2022
@jeffhandley jeffhandley added Cost:M Work that requires one engineer up to 2 weeks and removed Cost:S Work that requires one engineer up to 1 week labels Feb 23, 2022
@a74nh
Copy link
Contributor

a74nh commented May 3, 2022

Could you please assign this to @SwapnilGaikwad

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 1, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 30, 2022
@danmoseley
Copy link
Member

@kunalspathak with the attached change is this now addressed for all architectures, or should this remain open?

@a74nh
Copy link
Contributor

a74nh commented Jun 30, 2022

System.Text.ASCIIUtility.GetIndexOfFirstNonAsciiChar() is done for Arm64/X64.

Expecting a pull request for System.Text.ASCIIUtility.NarrowUtf16ToAscii() to be raised late this week/early next week.

@kunalspathak
Copy link
Member

System.Text.ASCIIUtility.GetIndexOfFirstNonAsciiChar() is done for Arm64/X64.

Expecting a pull request for System.Text.ASCIIUtility.NarrowUtf16ToAscii() to be raised late this week/early next week.

It's other way round. NarrowUtf16ToAscii() is complete and System.Text.ASCIIUtility.GetIndexOfFirstNonAsciiChar() is not yet complete.

@kunalspathak kunalspathak reopened this Jun 30, 2022
@a74nh
Copy link
Contributor

a74nh commented Jun 30, 2022

It's other way round. NarrowUtf16ToAscii() is complete and System.Text.ASCIIUtility.GetIndexOfFirstNonAsciiChar() is not yet complete.

Oops, yes :)

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 5, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-System.Text.Encoding Cost:M Work that requires one engineer up to 2 weeks Priority:3 Work that is nice to have
Projects
None yet
9 participants