Added custom DateTimeFormatter #716

khellang · 2016-09-30T12:12:01Z

Closes #715

Benchmark Results

About ~60% faster, and ~85% less allocations 😁

rynowak · 2016-09-30T15:56:24Z

src/Microsoft.Net.Http.Headers/ContentDispositionHeaderValue.cs

@@ -321,7 +321,7 @@ private void SetDate(string parameter, DateTimeOffset? date)
            else
            {
                // Must always be quoted
-                var dateString = string.Format(CultureInfo.InvariantCulture, "\"{0}\"", HttpRuleParser.DateToString(date.Value));
+                var dateString = string.Format(CultureInfo.InvariantCulture, "\"{0}\"", HeaderUtilities.FormatDate(date.Value));


Why isn't the quoting part of the formatting?

I think this might be a special requirement for Content-Disposition? @Tratcher?

Correct, the date only needs to be quoted in some contexts like CDHV

Maybe it worth adding quoted argument to HeaderUtilities.FormatDate?

BrennanConroy · 2016-09-30T16:02:12Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+        {
+            var offset = 0;
+            char* target = stackalloc char[Rfc1123DateLength];
+            var universalDateTime = dateTime.ToUniversalTime();


It may be possible to use dateTime.UtcDateTime to save the extra allocation that ToUniversalTime() does, but I would test perf for both in case DateTimeOffset is better for this scenario than DateTime

Doesn't ToUniversalTime() return a struct?

Yeah, ToUniversalTime just returns another DateTimeOffset using UtcDateTime; http://referencesource.microsoft.com/#mscorlib/system/datetimeoffset.cs,683

Do we really need to convert if input is already utc?

if (dateTime.Offset > TimeSpan.Zero) dateTime = dateTime.ToUniversalTime();

rynowak · 2016-09-30T16:04:58Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+
+namespace Microsoft.Net.Http.Headers
+{
+    internal static class DateTimeFormatter


I'm unsure of the level of optimization that's really required for this code, but a few off the cuff thoughts:

My intuition says there's gotta be a better way of copying the bytes than a foreach

It might be beneficial to 'pack' all of these bytes into an array + offset for locality

We should look into the inlinability of all of this (most related to 1)

What about now?

@hallatore did some testing with Buffer.Memcpy earlier today. It didn't change much. He also verified that stuff was inlined nicely.

I was testing changing to foreach to use Buffer.memcpy, but the results where identical.

Ref memcpy: https://gist.github.com/hallatore/a10ffd0d69a508d09b74a4f720c0511a

You could also consider vectorizing this where the lengths are known, using (int *) or (long *). I think that what's there is probably fine, and this is important it would come up again.

pakrym · 2016-09-30T15:56:21Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+        private static readonly byte[] SepBytes = UTF8.GetBytes(GetMonthName(9));
+        private static readonly byte[] OctBytes = UTF8.GetBytes(GetMonthName(10));
+        private static readonly byte[] NovBytes = UTF8.GetBytes(GetMonthName(11));
+        private static readonly byte[] DecBytes = UTF8.GetBytes(GetMonthName(12));


Why do you store these as bytes and then cast back to chars?

Uuh. That's a good question. I've been back and forth between char* and byte* a couple of times because of missing APIs for different targets. I'll revert back to char[].

pakrym · 2016-09-30T15:57:17Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+        {
+            switch (dayOfWeek)
+            {
+                case DayOfWeek.Sunday: return Format(SunBytes, target, offset);


Store as array and use indexing instead of switch

You want these in an array of arrays? 😄

Why not array of strings?

Uhhh, yes, of course. I guess we could just reuse DateTimeFormatInfo.AbbreviatedDayNames and DateTimeFormatInfo.AbbreviatedMonthNames.

Did you try this measuring packing all of these into a single array? Then your switch just returns an offset into the array.

For instance your format could be like:

M O N , T U E , W E D , ...

Then you can just cast to `(int *) and blit 4 bytes at once including the comma

pakrym · 2016-09-30T15:57:31Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+        {
+            switch (month)
+            {
+                case 1: return Format(JanBytes, target, offset);


khellang · 2016-09-30T16:35:32Z

That got rid of quite a bit of code 😝

BrennanConroy · 2016-09-30T16:44:20Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+
+        private static unsafe int FormatDayOfWeek(DayOfWeek dayOfWeek, char* target, int offset)
+        {
+            return Format(FormatInfo.AbbreviatedDayNames[(int) dayOfWeek], target, offset);


Could we cache this array and similarly for the month names
See http://referencesource.microsoft.com/#mscorlib/system/globalization/datetimeformatinfo.cs,1393 for what it does every get

Yeah, using those regressed perf substantially:

Caching the arrays did help, but it's still worse than it was:

Where did additional allocations come from?

Probably a string enumerator. Let me get rid of that foreach loop 😉

Aaaand we're back... Much better ✨

cesarblum · 2016-09-30T21:47:00Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+        private static readonly string[] MonthNames = FormatInfo.AbbreviatedMonthNames;
+
+        // The format is "ddd, dd MMM yyyy HH:mm:ss GMT".
+        private const int Rfc1123DateLength = 29;


nitpicking but you could have done Rfc1123DateLength = "ddd, dd MMM yyyy HH:mm:ss GMT".Length

khellang · 2016-10-03T09:48:08Z

Alright. Here's the current state of this PR:

About ~60% faster, and ~85% less allocations.

There are other, faster alternatives, but I'm not sure how far you'd like to stretch; https://gist.github.com/aL3891/2644c97e55f2f6c67b1ade15228031b8

BrennanConroy · 2016-10-03T15:31:07Z

Where did the extra 3 bytes come from?

khellang · 2016-10-03T15:36:16Z

@BrennanConroy It varies a bit from run to run. Not really sure why. I think it might average out the bytes allocated for the statics on the first op. @mattwarren?

EDIT: Yeah, looks like it takes the total amount of bytes allocated and divides it by the total number of operations. I think the number of ops can vary for each run. https://github.com/PerfDotNet/BenchmarkDotNet/blob/b35d523dfc859cc0f94897be124e675b79f74845/src/BenchmarkDotNet.Diagnostics.Windows/MemoryDiagnoser.cs#L160

pakrym · 2016-10-03T17:03:06Z

@khellang we have multiple PR's commit with similar use case of string construction, can you please try using dotnet/extensions#157 to format the date time so we can have common pattern in both of them?

khellang · 2016-10-03T17:56:20Z

@pakrym I already have it sitting here; khellang@6d9345f. Ran a quick benchmar and it looks like it regressed perf slightly (not sure why), but I think it's nice for consistency. Want me to include it in this PR?

khellang · 2016-10-03T17:58:08Z

The allocations are still the same, though. That was the main point of this PR (and related issue) before things turned into a perf contest 😛

pakrym · 2016-10-03T17:58:18Z

@khellang would be awesome.

khellang · 2016-10-04T10:55:38Z

🆙📅 Moved to InplaceStringBuilder and added a quoted argument.

Tratcher · 2016-10-04T16:56:34Z

src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs

+        {
+            var universal = dateTime.UtcDateTime;
+
+            var length = quoted ? QuotedRfc1123DateLength : Rfc1123DateLength;


QuotedRfc1123DateLength is only used once, you might as well put the +2 here.

Tratcher · 2016-10-04T16:58:36Z

src/Microsoft.Net.Http.Headers/project.json

@@ -24,6 +24,7 @@
  "frameworks": {
    "netstandard1.1": {
      "dependencies": {
+        "Microsoft.Extensions.Primitives": "1.1.0-*",


This goes under the root dependencies, it's not framework specific.

What about System.Buffers? That's not framework specific either?

Move them all

Shouldn't NETStandard.Library be moved to be framework-specific? Like khellang@b5f8a8d?

Doesn't it pull down lots of stuff you don't need when you target net451?

But does it hurt to move it under netstandard? Is this what you're recommending library authors do?

pakrym · 2016-10-05T19:19:35Z

Thank you @khellang !

dnfclas added the cla-already-signed label Sep 30, 2016

khellang force-pushed the custom-datetime-formatter branch 11 times, most recently from e20df05 to 248d389 Compare September 30, 2016 15:42

rynowak reviewed Sep 30, 2016

View reviewed changes

BrennanConroy reviewed Sep 30, 2016

View reviewed changes

rynowak reviewed Sep 30, 2016

View reviewed changes

pakrym suggested changes Sep 30, 2016

View reviewed changes

BrennanConroy reviewed Sep 30, 2016

View reviewed changes

This was referenced Sep 30, 2016

Don't allocate for ResponseCookiesFeature #699

Closed

Add InplaceStringFormatter #717

Closed

Add InplaceStringBuilder dotnet/extensions#157

Merged

cesarblum reviewed Sep 30, 2016

View reviewed changes

khellang force-pushed the custom-datetime-formatter branch 2 times, most recently from 5ba9c76 to df88a1c Compare October 3, 2016 13:34

pakrym approved these changes Oct 3, 2016

View reviewed changes

khellang added 2 commits October 4, 2016 12:53

Added custom DateTimeFormatter

f340186

Use InplaceStringBuilder

3f5f7e4

khellang force-pushed the custom-datetime-formatter branch from 0c806c2 to 7e454c9 Compare October 4, 2016 10:53

Added 'quoted' parameter

bd26df3

khellang force-pushed the custom-datetime-formatter branch from 7e454c9 to bd26df3 Compare October 4, 2016 11:02

Tratcher reviewed Oct 4, 2016

View reviewed changes

Moved dependencies

baceeb9

pakrym merged commit 063d6ec into aspnet:dev Oct 5, 2016

khellang deleted the custom-datetime-formatter branch October 5, 2016 20:55

khellang mentioned this pull request Dec 18, 2018

Replace custom DateTimeFormatter with ToString("r") or TryFormat dotnet/aspnetcore#5695

Merged

Added custom DateTimeFormatter #716

Added custom DateTimeFormatter #716

Conversation

khellang commented Sep 30, 2016 • edited Loading

Benchmark Results

Choose a reason for hiding this comment

khellang Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang commented Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang commented Oct 3, 2016

BrennanConroy commented Oct 3, 2016

khellang commented Oct 3, 2016 • edited Loading

pakrym commented Oct 3, 2016

khellang commented Oct 3, 2016

khellang commented Oct 3, 2016

pakrym commented Oct 3, 2016

khellang commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Oct 5, 2016

khellang commented Sep 30, 2016 •

edited

Loading

khellang Sep 30, 2016 •

edited

Loading

khellang Sep 30, 2016 •

edited

Loading

khellang Sep 30, 2016 •

edited

Loading

khellang Sep 30, 2016 •

edited

Loading

khellang commented Sep 30, 2016 •

edited

Loading

khellang Sep 30, 2016 •

edited

Loading

khellang commented Oct 3, 2016 •

edited

Loading