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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these three fields should all probably be prefixed with "Invariant"
@@ -993,7 +1020,17 @@ internal static string FastFormatRoundtrip(DateTime dateTime, TimeSpan offset) | |||
|
|||
return StringBuilderCache.GetStringAndRelease(result); | |||
} | |||
|
|||
|
|||
private static void AppendTimeOfDay(StringBuilder result, DateTime dateTime) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
const int Rfc1123Length = 29;
StringBuilder result = StringBuilderCache.Acquire(Rfc123Length);
?
Good feedback. I incorporated it all. |
internal static string FastFormatRoundtrip(DateTime dateTime, TimeSpan offset) | ||
{ | ||
StringBuilder result = StringBuilderCache.Acquire(); | ||
// yyyy-MM-ddTHH:mm:ss.fffffffK | ||
const int roundTrimFormatLength = 28; |
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.
Nit: Trim => Trip
|
||
result.Append(InvariantAbbreviatedDayNames[(int)dateTime.DayOfWeek]); | ||
result.Append(','); | ||
result.Append(' '); |
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.
you may append both in on append call .Append(", ");
return FastFormatRoundtrip(dateTime, offset); | ||
case 'R': | ||
case 'r': |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Addresses issue https://github.com/dotnet/coreclr/issues/7881 . I still need to complete full testing but it looks good so far.