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

TextEncoder.FindFirstCharacterToEncodeUtf8 not being used anywhere. #39829

Closed
eiriktsarpalis opened this issue Jul 23, 2020 · 4 comments · Fixed by #49373
Closed

TextEncoder.FindFirstCharacterToEncodeUtf8 not being used anywhere. #39829

eiriktsarpalis opened this issue Jul 23, 2020 · 4 comments · Fixed by #49373
Milestone

Comments

@eiriktsarpalis
Copy link
Member

Discovered this issue while working on ARM64 intrinsics optimizations for System.Text.Encodings.Web. While the method is public, it is also marked EditorBrowsable.Never. There are however unit tests consuming it.

A lot of the intrinsics optimizations for this project live in FindFirstCharacterToEncodeUtf8 implementations, and as a result we're not seeing performance improvements when benchmarking TextEncoder.EncodeUtf8.

@ghost
Copy link

ghost commented Jul 23, 2020

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

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 23, 2020
@tarekgh
Copy link
Member

tarekgh commented Jul 23, 2020

@GrabYourPitchforks just checking if you are triaging this area?

@GrabYourPitchforks
Copy link
Member

This method was at one point used by System.Text.Json (see source). They've stopped using the API since then, but since it's a public API we need to keep it around in perpetuity.

What is this bug tracking? That we should stop making perf changes to this API? I'd be fine with that if that's the direction we want to go.

@GrabYourPitchforks GrabYourPitchforks removed the untriaged New issue has not been triaged by the area owner label Aug 14, 2020
@GrabYourPitchforks GrabYourPitchforks added this to the Future milestone Aug 14, 2020
@eiriktsarpalis
Copy link
Member Author

What is this bug tracking?

Unlike TextEncoder.Encode(string) which depends on FindFirstCharacterToEncode(char*), TextEncoder.EncodeUtf8 does not make use of FindFirstCharacterToEncodeUtf8. The issue then is that TextEncoder.EncodeUtf8 does not take advantage of any of the hardware intrinsics optimizations. If that's intentional or not necessary to improve on, I can close the issue.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 9, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 19, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants