-
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
Improve DateTime{Offset} formatting further in a variety of cases #84963
Conversation
Some failures in the new test cases due to timezone disparity... will fix tomorrow... |
src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs
Show resolved
Hide resolved
@stephentoub when I run this against Fedora 38, I'm seeing two types of failures. Unexpected values, like:
And also FormatExceptions, like:
|
@tmds, yes, that's #84963 (comment) |
That's unexpected. You see those with this PR but not with main? Can you debug what the parser is choking on? This PR really doesn't change the parser, and CI doesn't show such failures. |
I see those on main also, but only on our CI server. I haven't been able to reproduce them on my machine. |
Ok, thanks, so not related to this PR. |
This looks correct. I see most of the failures are with the "U" format which will depend on the time zone. |
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.
Modulo fixing the tests, LGTM.
- Utf8Formatter special-cases 'G' / the default format. By moving that specialized implementation into DateTimeFormat, we can benefit from it from DateTime{Offset}.ToString/TryFormat as well. This was the last custom formatting routine in Utf8Formatter. As part of deleting two Utf8Formatter.Date.* files, I also changed the TryFormatDateTimeL shim function to go directly to TryFormatR rather than going through the wrapping TryFormat function. - The "s" and "u" formats are also reasonably popular and have a fixed pattern that's not sensitive to culture. By writing custom routines for those, we can not only speed them up, but also restructure the calling code to avoid needing the FormatIntoBuilder for some default/G cases. - The above requires being able to check whether a provider is invariant, but DateTimeFormatInfo.InvariantInfo and CultureInfo.InvariantCulture.DateTimeFormat actually returned different instances. I made them the same instance, such that we can now just compare against DateTimeFormatInfo.InvariantInfo to handle the 99.9% case of checking whether a DTFI represents the invariant culture. This also make access to DTFI.InvariantInfo faster, as it's now just returning a static readonly field rather than a lazily-initialized volatile field. - For "U", we were allocating a new DateTimeFormatInfo and GregorianCalendar (and supporting strings) on every formatting, resulting in ~1K of allocation. I fixed it to only allocate when necessary, which is rare. - Removed some bounds checking in ParseRepeatPattern and various DateTimeFormatInfo.GetXx helpers - Removed a % from 'h' handling in FormatCustomized. - Plus a few renames and changing DateTimeOffsetPattern to not access a lazily-initialized property on every iteration of the search loop. - And Tarek noticed one place we were appending an unchecked char that could have been non-ASCII, so I fixed that to check appropriately.
Utf8Formatter special-cases 'G' / the default format. By moving that specialized implementation into DateTimeFormat, we can benefit from it from DateTime{Offset}.ToString/TryFormat as well. This was the last custom formatting routine in Utf8Formatter. As part of deleting two Utf8Formatter.Date.* files, I also changed the TryFormatDateTimeL shim function to go directly to TryFormatR rather than going through the wrapping TryFormat function.
The "s" and "u" formats are also reasonably popular and have a fixed pattern that's not sensitive to culture. By writing custom routines for those, we can not only speed them up, but also restructure the calling code to avoid needing the FormatIntoBuilder for some default/G cases.
The above requires being able to check whether a provider is invariant, but DateTimeFormatInfo.InvariantInfo and CultureInfo.InvariantCulture.DateTimeFormat actually returned different instances. I made them the same instance, such that we can now just compare against DateTimeFormatInfo.InvariantInfo to handle the 99.9% case of checking whether a DTFI represents the invariant culture. This also make access to DTFI.InvariantInfo faster, as it's now just returning a static readonly field rather than a lazily-initialized volatile field.
For "U", we were allocating a new DateTimeFormatInfo and GregorianCalendar (and supporting strings) on every formatting, resulting in ~1K of allocation. I fixed it to only allocate when necessary, which is rare.
For formats without a fast path, TryFormat ends up formatting into a ValueListBuilder and then copying from that into the destination span. We can save on a copy by seeding the ValueListBuilder with the destination span itself.
Removed some bounds checking in ParseRepeatPattern and various DateTimeFormatInfo.GetXx helpers
Removed a % from 'h' handling in FormatCustomized.
Plus a few renames and changing DateTimeOffsetPattern to not access a lazily-initialized property on every iteration of the search loop.
And Tarek noticed one place we were appending an unchecked char that could have been non-ASCII, so I fixed that to check appropriately. Fixes Some DateTimeTest format tests fail on the upcoming Fedora 39 #84763.
Added a bunch of test cases.