-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Implement IUtf8SpanFormattable on all the numeric types in corelib #84587
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsAugments SByte, Byte, Int16, UInt16, Int32, UInt32, Int64, UInt64, IntPtr, UIntPtr, Int128, UInt128, Half, Single, Double, NFloat, and Decimal to implement IUtf8SpanFormattable. Also fixes corelib TODOs around using IUtf8SpanFormattable. And removes some duplicate code from Utf8Formatter. There is still more consolidation to be done between FormattingHelpers and Number.Formatting. There are a few small (e.g. a few nanoseconds) regressions on my machine. I'm seeing what can be done about them. There are also some improvements. Most things are neutral. For Utf8Formatter, most of the APIs are now just delegating into the corresponding IUtf8SpanFormattable functionality, which means things that weren't previously supported (e.g. various format specifiers) now are. I opted not to block them. We can revisit that decision if desired.
|
src/libraries/System.Private.CoreLib/src/System/Globalization/NumberFormatInfo.cs
Outdated
Show resolved
Hide resolved
I tried this on mono with the interpreter. Nothing concerning.
|
src/libraries/System.Private.CoreLib/src/System/Globalization/NumberFormatInfo.cs
Outdated
Show resolved
Hide resolved
Augments SByte, Byte, Int16, UInt16, Int32, UInt32, Int64, UInt64, IntPtr, UIntPtr, Int128, UInt128, Half, Single, Double, NFloat, and Decimal. Also fixes corelib TODOs around using IUtf8SpanFormattable. And removes some duplicate code from Utf8Formatter. There is still more consolidation to be done between FormattingHelpers and Number.Formatting.
- Use an internal interface implemented by char and byte to have dedicated CastFrom methods that are always inlineable due to very small size. - Use pointers in some core formatting routines to avoid needing bulky IL for manipulating refs with spans, making various members more inlineable. - Avoid Encoding.UTF8.GetBytes in various code paths by caching more UTF8 sequences on DateTimeFormatInfo and NumberFormatInfo - Change FormatCustomizedTimeZone to special-case 2 vs 3+ tokens in order to avoid extra AppendSpan calls - Fix growth logic in ValueListBuilder to not forcibly grow more than is needed - Inline ValueListBuilder.AppendSpan and remove some bounds checks (at least on 64-bit) - Change FormatDigits to special-case lengths of 1/2/4 and to use existing formatting routines rather than a custom one - Remove the FormatDigits wrapper overload and just have all calls go to the main workhorse method. - Remove the use of "..."u8 in R/O formatting that leads to needing to use additional span-based helpers. The minimal gain on coreclr isn't worth the extra complication - Changed some switches to include half the cases based on lowercasing the ASCII input char - Moved Date/TimeOnly charsWritten into Try method to be closer to the source of truth rather than having the value far aware (this isn't for perf and could possibly even be a microregression, so I included it here to ensure it's not measurable).
7215595
to
b593d86
Compare
src/libraries/System.Private.CoreLib/src/System/Globalization/NumberFormatInfo.cs
Show resolved
Hide resolved
The second commit here tries to address some of the regressions seen on some platforms in CI after the DateTime change went in. I did it in this PR in order to then be able to measure the end-to-end, as this PR was also modifying some of those code paths. Details on the changes made are in the second commit description. After merging this, we'll want to rebase @tannergooding's parsing PR in order to unify the interfaces introduced; Tanner, I can do that if you'd prefer, or you're of course welcome to. Locally, here's what I see now comparing main to this pr... On coreclr:
On mono with JIT:
On mono with interpreter:
Note that all of these numbers are based on commit 7526a4c, and thus all include fixes @vargaz already made in response to this issue (279c65a, 8699942, and ae4c82b). |
The failures for browser-wasm interp and mono_interpreter linux look of the same kind, so I think if jiterp is involved it's correctly emulating interp behavior here. Happy to help look into it once I get time if Vlad is too busy though, and if the interp gets changed I can update the jiterp to match. |
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs
Show resolved
Hide resolved
The failures appear to be some kind of memory corruption when running with the interpreter. Testcase:
|
This repro works fine in debug wasm builds but fails in release, so it's likely an interpreter optimization problem. |
If I remove all the '#if MONO' stuff in the Number.Formatting file, everything works fine. |
For the record, the mono failures are caused by the #if MONO block in WriteFourDigits. Running the interpreter with |
Thanks. I've removed those blocks, but we obviously should also sepatately figure out what the issue is in the interpreter. |
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.
LGTM.
Are you going to switch the APIs to be public in a follow up PR?
Yup.
Thanks. |
Augments SByte, Byte, Int16, UInt16, Int32, UInt32, Int64, UInt64, IntPtr, UIntPtr, Int128, UInt128, Half, Single, Double, NFloat, and Decimal to implement IUtf8SpanFormattable. Also fixes corelib TODOs around using IUtf8SpanFormattable. And removes some duplicate code from Utf8Formatter.
There is still more consolidation to be done between FormattingHelpers and Number.Formatting.
There are a few small (e.g. a few nanoseconds) regressions on my machine. I'm seeing what can be done about them. There are also some improvements. Most things are neutral.
For Utf8Formatter, most of the APIs are now just delegating into the corresponding IUtf8SpanFormattable functionality, which means things that weren't previously supported (e.g. various format specifiers) now are. I opted not to block them. We can revisit that decision if desired.
Fixes #84527
Contributes to #81500