-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Revert "Reduce unsafe usage in TextInfo.cs" #123692
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
Conversation
This reverts commit bb5ea9b.
|
rt-sz report MichalStrehovsky/rt-sz#203 |
Sorry about that. |
It is one of the strategies that we have used in other similar situations to avoid the code bloat. For this specific case, we have 30+ places where we do string allocation using unsafe FastAllocateString in core globalization and formatting routines to avoid the code bloat and other overheads from the safe variant. I do not think we need to go out of our way to avoid the unsafe code for this one. We should aim to have a solution that scales without introducing regressions for majority of places that use FastAllocateString. The solution likely includes #85014 . cc @EgorBo |
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.
Pull request overview
This PR reverts PR #122116 which reduced unsafe usage in TextInfo.cs. The revert is being done to investigate a NativeAOT binary size regression on osx-x64 (issue #123667), where the size increased from ~1.5MB to 1.551MB, causing size tests to fail.
Changes:
- Reverts
ToLowerAsciiInvariant(string)method from safe span-based implementation usingstring.Createback to unsafe pointer-based implementation usingfixedstatements and manual pointer arithmetic
|
FYI the original PR made it into 11.0-preview1 so we'd need to backport if we think it should be fixed there. |
|
@akoeplinger I do not see this PR in preview1 branch https://github.com/dotnet/runtime/blob/release/11.0-preview1/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs However, I do see the other reverted PR (#123307) in preview1 branch. It needs to be reverted there. |
|
@jkotas ah yeah, I got confused because the original PR was linked in dotnet/dotnet#4440 but we found out that the wrong build from main was flown accidentally so the runtime repo was reset in dotnet/dotnet#4454 (which reverted that change) |
Reverts #122116
Investigating #123667