- Notifications
You must be signed in to change notification settings - Fork2.6k
DateTime.ToString() Fast Track for RFC1123#7891
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Petermarcu commentedOct 30, 2016
@tarekgh , I measured a more than 50% reduction in allocated bytes and 75% reduction in allocations. |
| returnFastFormatRoundtrip(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 overformat[0] is likely to generate better code.
| internalstaticreadonlyDateTimeFormatInfoFormatInfo=CultureInfo.InvariantCulture.DateTimeFormat; | ||
| internalstaticreadonlystring[]AbbreviatedMonthNames=FormatInfo.AbbreviatedMonthNames; | ||
| internalstaticreadonlystring[]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"
| } | ||
| privatestaticvoidAppendTimeOfDay(StringBuilderresult,DateTimedateTime) |
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.
| internalstaticstringFastFormatRfc1123(DateTimedateTime,TimeSpanoffset,DateTimeFormatInfodtfi) | ||
| { | ||
| // ddd, dd MMM yyyy HH:mm:ss GMT | ||
| StringBuilderresult=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:
constintRfc1123Length=29;StringBuilderresult=StringBuilderCache.Acquire(Rfc123Length);
?
Petermarcu commentedOct 30, 2016
Good feedback. I incorporated it all. |
| { | ||
| StringBuilderresult=StringBuilderCache.Acquire(); | ||
| // yyyy-MM-ddTHH:mm:ss.fffffffK | ||
| constintroundTrimFormatLength=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(", ");
| case'o': | ||
| returnFastFormatRoundtrip(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?
Petermarcu commentedOct 30, 2016
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
Petermarcu commentedOct 31, 2016
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
Petermarcu commentedOct 31, 2016
@tarekgh , does that last update look good and address your feedback? |
tarekgh commentedOct 31, 2016
LGTM. |
Addresses issuehttps://github.com/dotnet/coreclr/issues/7881 . I still need to complete full testing but it looks good so far.