-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Improve DateTime{Offset} "r" and "o" formatting performance #17092
Conversation
FYI @Petermarcu, since you previously did some optimizations for allocations with these formats. |
Nice win! |
} | ||
|
||
public String ToString(IFormatProvider formatProvider) | ||
{ | ||
return DateTimeFormat.Format(ClockDateTime, null, DateTimeFormatInfo.GetInstance(formatProvider), Offset); | ||
return DateTimeFormat.Format(ClockDateTime, null, null, Offset); |
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.
should we pass the formatProvider here?
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.
Oops! Yes, thanks.
} | ||
|
||
public String ToString(String format, IFormatProvider formatProvider) | ||
{ | ||
return DateTimeFormat.Format(ClockDateTime, format, DateTimeFormatInfo.GetInstance(formatProvider), Offset); | ||
return DateTimeFormat.Format(ClockDateTime, format, null, Offset); |
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.
should we pass the formatProvider here?
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.
Oops! Yes, thanks.
Two main changes: 1. Rewrote the formatting to use span, only to then discover that we already had almost exactly the same implementation in Utf8Formatter. As that one had some extra optimizations around JIT behaviors, I ported that over instead. 2. Avoided [ThreadStatic] lookups unless necessary. ToString/TryFormat for "o"/"O" improve by ~2.5x. ToString/TryFormat for "r"/"R" improve by ~3x.
e10f1b4
to
dfad091
Compare
private static void Append2DigitNumber(StringBuilder result, int val) | ||
{ | ||
result.Append((char)('0' + (val / 10))); | ||
result.Append((char)('0' + (val % 10))); |
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.
Could drop the the two idiv
s?
int tens = (int)((val * 205u) >> 11); // div10, valid to 1028
result.Append((char)('0' + tens);
result.Append((char)('0' + (val - (tens * 10));
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.
Actually the Jit will already do this since its a const
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.
Actually the Jit will already do this since its a const
Yeah, all of the ones that have been commented on here involve a const denominator. I'm going to let the JIT do its thing.
private static void Append2DigitNumber(StringBuilder result, int val) | ||
{ | ||
result.Append((char)('0' + (val / 10))); | ||
result.Append((char)('0' + (val % 10))); |
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.
Use the DivMod helper method that we use in Utf8Formatter?
private static void Append2DigitNumber(StringBuilder result, int val)
{
result.Append((char)('0' + (val / 10)));
result.Append((char)('0' + (val % 10)));
}
private static void Append2DigitNumber2(StringBuilder result, int val)
{
uint div = DivMod((uint)val, out uint modulo);
result.Append((char)('0' + div));
result.Append((char)('0' + modulo));
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static uint DivMod(uint numerator, out uint modulo)
{
uint div = numerator / 10;
modulo = numerator - (div * 10);
return div;
}
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.
Use the DivMod helper method that we use in Utf8Formatter?
I can switch to Math.DivRem if it makes a difference.
case 'r': | ||
case 'R': | ||
const int FormatRLength = 29; | ||
string str = string.FastAllocateString(FormatRLength); |
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.
Can you please explain why we use a stackalloc span for 'o' (and then call ToString), but create a string for '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.
Because 'r' is always a fixed result length; 'o' is not.
charsWritten = charsRequired; | ||
|
||
// Hoist most of the bounds checks on destination. | ||
{ var unused = destination[MinimumBytesNeeded - 1]; } |
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.
Interesting trick. @AndyAyersMS, the JIT can exclude bounds checks within loops. Any way for it to optimize out the bounds checks for multiple uses outside of a loop, such as this?
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.
(This is just ported from the Utf8Formatter implementation.)
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.
Sure, if the array length is not known, and the code is writing at fixed indices, write or access the largest first, and the jit should know the subsequent smaller indices are in bounds. More or less what is done here.
If indices contain unknown values the jit can sometimes reason about similar patterns, but not always.
val = val / 10; | ||
index++; | ||
ulong temp = '0' + value; | ||
value /= 10; |
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.
If you add the DivMod helper, we can call it in all these places.
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.
If you add the DivMod helper
Math already has a DivRem method.
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.
Ah yes, we have access to that here.
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.
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.
Also note that this code came from Utf8Formatter.
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.
LGTM
Two main changes:
Before:
After:
Test:
cc: @danmosemsft, @jkotas, @ahsonkhan