Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

DateTime.ToString(“o”) allocates 32 objects, #7836

Merged
merged 8 commits into from
Oct 29, 2016
Merged

DateTime.ToString(“o”) allocates 32 objects, #7836

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8d69b5d
Adding lower allocating DateTime.ToString('o')
Petermarcu Oct 26, 2016
132e5d1
removing unreachable switch cases
Petermarcu Oct 26, 2016
f49c5d1
more cleanup
Petermarcu Oct 26, 2016
262c07a
updates based on review feedback
Petermarcu Oct 27, 2016
dfe10ff
fixing whitespace
Petermarcu Oct 27, 2016
bf79a18
Fix Casing
Petermarcu Oct 27, 2016
1321303
fixing DateTimeOffset
Petermarcu Oct 28, 2016
ec5af22
adding assert
Petermarcu Oct 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 78 additions & 10 deletions src/mscorlib/src/System/Globalization/DateTimeFormat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,10 +807,6 @@ internal static String GetRealFormat(String format, DateTimeFormatInfo dtfi)
case 'M': // Month/Day Date
realFormat = dtfi.MonthDayPattern;
break;
case 'o':
case 'O':
realFormat = RoundtripFormat;
break;
case 'r':
case 'R': // RFC 1123 Standard
realFormat = dtfi.RFC1123Pattern;
Expand Down Expand Up @@ -848,10 +844,6 @@ internal static String GetRealFormat(String format, DateTimeFormatInfo dtfi)
//
private static String ExpandPredefinedFormat(String format, ref DateTime dateTime, ref DateTimeFormatInfo dtfi, ref TimeSpan offset) {
switch (format[0]) {
case 'o':
case 'O': // Round trip format
dtfi = DateTimeFormatInfo.InvariantInfo;
break;
case 'r':
case 'R': // RFC 1123 Standard
if (offset != NullOffset) {
Expand Down Expand Up @@ -957,12 +949,88 @@ internal static String Format(DateTime dateTime, String format, DateTimeFormatIn
}

if (format.Length == 1) {
switch (format[0]) {
case 'o':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: using if-statement maybe better than switch here as we are testing 2 values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do that, it was another thing I debated and switches seemed to be used so much in this file I went with the style of the file :)

case 'O':
// Fast track for round trip format without going through a format string.
return RoundTripFormat(ref dateTime);
}

format = ExpandPredefinedFormat(format, ref dateTime, ref dtfi, ref offset);
}
}

return (FormatCustomized(dateTime, format, dtfi, offset));
}


internal static string RoundTripFormat(ref DateTime dateTime)
Copy link
Member

@tarekgh tarekgh Oct 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RoundTripFormat [](start = 31, length = 15)

one last thing, you named this method as RoundtripFormat which is same name as the defined string constant. would be better to rename the method to something better (e.g. FormatAsISO8601() or anything you think better).

internal const String RoundtripFormat = "yyyy'-'MM'-'dd'T'HH':'mm':'ss.fffffffK";

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good feedback

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I measured it using the Perf tools in Visual Studio. I can show you if you'd like to see it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ref DateTime dateTime rather than just DateTime dateTime?

Copy link
Member

@tarekgh tarekgh Oct 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, DateTime is long size so it can passed as value

{
StringBuilder result = StringBuilderCache.Acquire();
Append(result, dateTime.Year, 4);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that calling .Year, .Month, etc are all doing quite a bit of work, and you are doing this each and every time.
This is computationally expensive, and you can probably avoid doing this if you build the entire thing in one go.

result.Append('-');

Append(result, dateTime.Month, 2);
result.Append('-');

Append(result, dateTime.Day, 2);
result.Append('T');

Append(result, dateTime.Hour, 2);
result.Append(':');

Append(result, dateTime.Minute, 2);
result.Append(':');

Append(result, dateTime.Second, 2);
result.Append('.');

long fraction = dateTime.Ticks % TimeSpan.TicksPerSecond;

Append(result, fraction, 7);

switch (dateTime.Kind)
{
case DateTimeKind.Local:
{
TimeSpan offset = TimeZoneInfo.Local.GetUtcOffset(dateTime);

if (offset >= TimeSpan.Zero)
{
result.Append('+');
}
else
{
result.Append('-');
offset = offset.Negate();
}

Append(result, offset.Hours, 2);
result.Append(':');
Append(result, offset.Minutes, 2);
}
break;

case DateTimeKind.Utc:
result.Append('Z');
break;

default:
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this default case could be removed

}

return StringBuilderCache.GetStringAndRelease(result);
}

internal static void Append(StringBuilder builder, long val, int digit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append [](start = 29, length = 6)

nit: could you call this AppendNumber instead. it will make easier to not confuse it with the StringBuilder Append

{
if (digit > 1)
{
Append(builder, val / 10, digit - 1);
}
Copy link
Member

@tarekgh tarekgh Oct 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may be expensive as it will be recursive call. I would do it in a loop better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was torn because its limited in its current use to not being too deep but I can convert to a loop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really happy with my options for a loop here. Will still think about it but may stick with recursion if I can't come up with a better idea for how to do it in a loop cheaply.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every solution I've come up with for the loop requires more allocations and the use of a stack of some kind to append the digits from most significant to least. Given the max recursive depth here is 7 and its usually 2 I think the recursive solution is fine and the code is much more readable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is some implementation suggestion but I'll leave it to you to decide if you think the recursion is better here.

        internal static void AppendNumber(StringBuilder builder, long val, int digit)
        {
            for (int i = 0; i < digit; i++)
            {
                builder.Append('0');
            }

            int index = 1;

            while (val > 0 && index <= digit)
            {
                builder[builder.Length - index] = (char)('0' + (val % 10));
                val = val / 10;
                index++;
            }

            index = builder.Length - digit;

            while (val > 0)
            {
                builder.Insert(index, (char)('0' + (val % 10)));
                val = val / 10;
            }
        }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage of the string builder is expensive. You are first allocating the string builder (even cached), then in the end you are allocating the string each and every time.

We have run into perf issues here and we solved them in the following manner:
https://ayende.com/blog/169798/excerpts-from-the-ravendb-performance-team-report-dates-take-a-lot-of-time

Note that this code does a single allocation for the string, compute the date parts only once.

In our tests, it was 15% of the runtime of the default impl. And it can probably be improved still.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am seeing you have removed the second loop. I would suggest we just assert val == 0 if we need this method be reliable in case someone else later use it wrong way


builder.Append((char)('0' + (val % 10)));
}
Copy link

@redknightlois redknightlois Oct 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that AppendNumber is used multiple times in the same method, I would look if there is a performance improvement by aggresively inlining AppendNumber. JIT may recognize it and build a single function pushing jump offsets to the stack. That would ensure that the code can avoid the call overhead altogether.

EDIT: If not, it's probably worth to open an issue for the JIT team ;)



internal static String[] GetAllDateTimes(DateTime dateTime, char format, DateTimeFormatInfo dtfi)
{
Contract.Requires(dtfi != null);
Expand Down