DateTime.ToString() Fast Track for RFC1123 #7891
Conversation
| @tarekgh , I measured a more than 50% reduction in allocated bytes and 75% reduction in allocations. |
| return FastFormatRoundtrip(dateTime, offset); | ||
| } | ||
| | ||
| if (format[0] == 'r' || format[0] == 'R') { |
There was a problem hiding this comment.
Switch statement over format[0] is likely to generate better code.
| | ||
| internal static readonly DateTimeFormatInfo FormatInfo = CultureInfo.InvariantCulture.DateTimeFormat; | ||
| internal static readonly string[] AbbreviatedMonthNames = FormatInfo.AbbreviatedMonthNames; | ||
| internal static readonly string[] AbbreviatedDayNames = FormatInfo.AbbreviatedDayNames; |
There was a problem hiding this comment.
Nit: these three fields should all probably be prefixed with "Invariant"
| } | ||
| | ||
| | ||
| private static void AppendTimeOfDay(StringBuilder result, DateTime dateTime) |
There was a problem hiding this comment.
Nit: the name should probably include format information, e.g. AppendHHmmssTimeOfDay, or something along those lines.
| internal static string FastFormatRfc1123(DateTime dateTime, TimeSpan offset, DateTimeFormatInfo dtfi) | ||
| { | ||
| // ddd, dd MMM yyyy HH:mm:ss GMT | ||
| StringBuilder result = StringBuilderCache.Acquire(); |
There was a problem hiding this comment.
Maybe:
const int Rfc1123Length = 29; StringBuilder result = StringBuilderCache.Acquire(Rfc123Length);?
| Good feedback. I incorporated it all. |
| { | ||
| StringBuilder result = StringBuilderCache.Acquire(); | ||
| // yyyy-MM-ddTHH:mm:ss.fffffffK | ||
| const int roundTrimFormatLength = 28; |
| | ||
| result.Append(InvariantAbbreviatedDayNames[(int)dateTime.DayOfWeek]); | ||
| result.Append(','); | ||
| result.Append(' '); |
There was a problem hiding this comment.
you may append both in on append call .Append(", ");
| case 'o': | ||
| return FastFormatRoundtrip(dateTime, offset); | ||
| case 'R': | ||
| case 'r': |
There was a problem hiding this comment.
I am seeing in the ExpandPredefinedFormat we have the following code, but it looks you don't handle that
case 'r': case 'R': // RFC 1123 Standard if (offset != NullOffset) { // Convert to UTC invariants mean this will be in range dateTime = dateTime - offset; } else if (dateTime.Kind == DateTimeKind.Local) { InvalidFormatForLocal(format, dateTime); } dtfi = DateTimeFormatInfo.InvariantInfo; break; There was a problem hiding this comment.
Yeah, that is special handling for DateTimeOffset I need to apply. Do you know what InvalidFormatForLocal(...) is for and why it exists and is empty? MDA?
| @dotnet-bot test Linux ARM Emulator Cross Debug Build please |
| @dotnet-bot test Linux ARM Emulator Cross Debug Build please |
| @tarekgh , does that last update look good and address your feedback? |
| LGTM. |
6 participants
Addresses issue https://github.com/dotnet/coreclr/issues/7881 . I still need to complete full testing but it looks good so far.