Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Added custom DateTimeFormatter #716

Merged
merged 4 commits into from
Oct 5, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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 = HeaderUtilities.FormatDate(date.Value, quoted: true);
if (dateParameter != null)
{
dateParameter.Value = dateString;
Expand Down
100 changes: 100 additions & 0 deletions src/Microsoft.Net.Http.Headers/DateTimeFormatter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Globalization;
using System.Runtime.CompilerServices;
using Microsoft.Extensions.Primitives;

namespace Microsoft.Net.Http.Headers
{
internal static class DateTimeFormatter
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. My intuition says there's gotta be a better way of copying the bytes than a foreach
  2. It might be beneficial to 'pack' all of these bytes into an array + offset for locality
  3. We should look into the inlinability of all of this (most related to 1)

Copy link
Contributor Author

@khellang khellang Sep 30, 2016

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
private static readonly DateTimeFormatInfo FormatInfo = CultureInfo.InvariantCulture.DateTimeFormat;

private static readonly string[] MonthNames = FormatInfo.AbbreviatedMonthNames;
private static readonly string[] DayNames = FormatInfo.AbbreviatedDayNames;

private static readonly int Rfc1123DateLength = "ddd, dd MMM yyyy HH:mm:ss GMT".Length;
private static readonly int QuotedRfc1123DateLength = Rfc1123DateLength + 2;

// ASCII numbers are in the range 48 - 57.
private const int AsciiNumberOffset = 0x30;

private const string Gmt = "GMT";
private const char Comma = ',';
private const char Space = ' ';
private const char Colon = ':';
private const char Quote = '"';

public static string ToRfc1123String(this DateTimeOffset dateTime)
{
return ToRfc1123String(dateTime, false);
}

public static string ToRfc1123String(this DateTimeOffset dateTime, bool quoted)
{
var universal = dateTime.UtcDateTime;

var length = quoted ? QuotedRfc1123DateLength : Rfc1123DateLength;
Copy link
Member

Choose a reason for hiding this comment

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

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

var target = new InplaceStringBuilder(length);

if (quoted)
{
target.Append(Quote);
}

target.Append(DayNames[(int)universal.DayOfWeek]);
target.Append(Comma);
target.Append(Space);
AppendNumber(ref target, universal.Day);
target.Append(Space);
target.Append(MonthNames[universal.Month - 1]);
target.Append(Space);
AppendYear(ref target, universal.Year);
target.Append(Space);
AppendTimeOfDay(ref target, universal.TimeOfDay);
target.Append(Space);
target.Append(Gmt);

if (quoted)
{
target.Append(Quote);
}

return target.ToString();
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void AppendYear(ref InplaceStringBuilder target, int year)
{
target.Append(GetAsciiChar(year / 1000));
target.Append(GetAsciiChar(year % 1000 / 100));
target.Append(GetAsciiChar(year % 100 / 10));
target.Append(GetAsciiChar(year % 10));
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void AppendTimeOfDay(ref InplaceStringBuilder target, TimeSpan timeOfDay)
{
AppendNumber(ref target, timeOfDay.Hours);
target.Append(Colon);
AppendNumber(ref target, timeOfDay.Minutes);
target.Append(Colon);
AppendNumber(ref target, timeOfDay.Seconds);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void AppendNumber(ref InplaceStringBuilder target, int number)
{
target.Append(GetAsciiChar(number / 10));
target.Append(GetAsciiChar(number % 10));
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static char GetAsciiChar(int value)
{
return (char)(AsciiNumberOffset + value);
}
}
}
7 changes: 6 additions & 1 deletion src/Microsoft.Net.Http.Headers/HeaderUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,12 @@ public static bool TryParseDate(string input, out DateTimeOffset result)

public static string FormatDate(DateTimeOffset dateTime)
{
return HttpRuleParser.DateToString(dateTime);
return FormatDate(dateTime, false);
}

public static string FormatDate(DateTimeOffset dateTime, bool quoted)
{
return dateTime.ToRfc1123String(quoted);
}

public static string RemoveQuotes(string input)
Expand Down
6 changes: 0 additions & 6 deletions src/Microsoft.Net.Http.Headers/HttpRuleParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,6 @@ internal static HttpParseResult GetQuotedPairLength(string input, int startIndex
return HttpParseResult.Parsed;
}

internal static string DateToString(DateTimeOffset dateTime)
{
// Format according to RFC1123; 'r' uses invariant info (DateTimeFormatInfo.InvariantInfo)
return dateTime.ToUniversalTime().ToString("r", CultureInfo.InvariantCulture);
}

internal static bool TryStringToDate(string input, out DateTimeOffset result)
{
// Try the various date formats in the order listed above.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public override string ToString()
{
if (_entityTag == null)
{
return HttpRuleParser.DateToString(_lastModified.Value);
return HeaderUtilities.FormatDate(_lastModified.Value);
}
return _entityTag.ToString();
}
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.Net.Http.Headers/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"frameworks": {
"netstandard1.1": {
"dependencies": {
"Microsoft.Extensions.Primitives": "1.1.0-*",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Move them all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Sure, once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"System.Buffers": "4.3.0-*",
"System.Diagnostics.Contracts": "4.3.0-*"
}
Expand Down
48 changes: 48 additions & 0 deletions test/Microsoft.Net.Http.Headers.Tests/HeaderUtilitiesTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using Xunit;

namespace Microsoft.Net.Http.Headers
{
public static class HeaderUtilitiesTest
{
private const string Rfc1123Format = "r";

[Theory]
[MemberData(nameof(TestValues))]
public static void ReturnsSameResultAsRfc1123String(DateTimeOffset dateTime, bool quoted)
{
var formatted = dateTime.ToString(Rfc1123Format);
var expected = quoted ? $"\"{formatted}\"" : formatted;
var actual = HeaderUtilities.FormatDate(dateTime, quoted);

Assert.Equal(expected, actual);
}

public static TheoryData<DateTimeOffset, bool> TestValues
{
get
{
var data = new TheoryData<DateTimeOffset, bool>();

var now = DateTimeOffset.Now;

foreach (var quoted in new[] { true, false })
{
for (var i = 0; i < 60; i++)
{
data.Add(now.AddSeconds(i), quoted);
data.Add(now.AddMinutes(i), quoted);
data.Add(now.AddDays(i), quoted);
data.Add(now.AddMonths(i), quoted);
data.Add(now.AddYears(i), quoted);
}
}

return data;
}
}
}
}