Optimize DateTimeOffset #111112

pentp · 2025-01-06T05:04:00Z

Optimize and cleanup DateTimeOffset.
Some additional optimizations for DateTime.
Unify and improve NumberFormatInfo.GetInstance and DateTimeFormatInfo.GetInstance.
Fix DateTimeOffset.UtcNow to not have Kind=Utc in its _dateTime value (the existing assert didn't catch it because it was basically a no-op). Added better asserts.
Cleanup some uses of DateTime[Offset].

Addressed feedback from the previous version of this PR: #58169

dotnet-policy-service · 2025-01-06T05:04:34Z

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

huoyaoyuan

This is really questionable to me. It's employing a lot of anti-patterns to workaround a maybe-realistic codegen limitation as mentioned in #58169. Do you have any measured performance data to support it? Moreover, the codegen limitations are better solved in JIT, instead of workarounded in managed code.

src/libraries/Common/src/System/TimeProvider.cs

huoyaoyuan · 2025-01-06T05:10:04Z

src/libraries/System.Private.CoreLib/src/System/DateTime.Windows.cs

@@ -41,26 +41,28 @@ public static unsafe DateTime UtcNow
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
-        internal static unsafe bool IsValidTimeWithLeapSeconds(int year, int month, int day, int hour, int minute, DateTimeKind kind)
+        private static unsafe bool IsValidTimeWithLeapSeconds(DateTime value)


This is likely a deoptimization. Decomposing DateTime has its overhead.

This method is never called in practice. And hopefully there will be no more leap seconds added in the future.

huoyaoyuan · 2025-01-06T05:12:18Z

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

+        public DateTime AddMonths(int months) => AddMonths(this, months);
+        private static DateTime AddMonths(DateTime date, int months)


This is not a good practice. The static method doesn't save anything.

The calling convention for struct instance methods always forces the struct on stack, unless the method is inlined.

I see the problem in calling convention. Is there any measurement about the performance impact?

huoyaoyuan · 2025-01-06T05:14:30Z

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

+            _dateData >> KindShift == KindLocalAmbiguousDst >> KindShift;

        public DateTimeKind Kind
        {
            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => InternalKind switch
+            get => (_dateData >> KindShift) switch


It's anti-pattern to operate on raw bits rather than reusable properties.

This is the reusable property (InternalKind is inefficient for this purpose).

InternalKind is expected to be efficient. What's the problem here? It should always be inlined.

Using shift instead of mask produces smaller code (due to the large values of FlagsMask, etc.).
I measured 3 different common scenarios using 3 different approaches:

BASELINE Mean Error StdDev

DateTime_Kind 0.9101 ns 0.0050 ns 0.0030 ns

DateTime_KindBranch 0.6964 ns 0.0093 ns 0.0055 ns

DateTime_KindSwitch 2.6310 ns 0.2747 ns 0.1817 ns

SHIFT Mean Error StdDev

DateTime_Kind 0.9214 ns 0.0087 ns 0.0058 ns

DateTime_KindBranch 0.4721 ns 0.0038 ns 0.0025 ns

DateTime_KindSwitch 2.0659 ns 0.0146 ns 0.0087 ns

BRANCHLESS Mean Error StdDev

DateTime_Kind 0.4690 ns 0.0019 ns 0.0010 ns

DateTime_KindBranch 0.5248 ns 0.0039 ns 0.0026 ns

DateTime_KindSwitch 0.7505 ns 0.0083 ns 0.0055 ns

Bechmark source (note that the branching versions rely on perfect branch prediction)

int _ops = 1000; DateTime _empty; DateTime _utcNow = DateTime.UtcNow; [Benchmark(OperationsPerInvoke = 1000)] public int DateTime_Kind() { int res = 0; int c = _ops; DateTime value = _empty, next = _utcNow; do { res += (int)value.Kind; value = next; } while (--c > 0); return res; } [Benchmark(OperationsPerInvoke = 1000)] public int DateTime_KindBranch() { int res = 0; int c = _ops; DateTime value = _empty, next = _utcNow; do { if (value.Kind == DateTimeKind.Utc) res++; value = next; } while (--c > 0); return res; } [Benchmark(OperationsPerInvoke = 1000)] public int DateTime_KindSwitch() { int res = 0; int c = _ops; DateTime value = _empty, next = _utcNow; do { switch (value.Kind) { case DateTimeKind.Unspecified: res += 7; break; case DateTimeKind.Utc: res += 9; break; default: res += 3; break; } value = next; } while (--c > 0); return res; }

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs

src/libraries/System.Private.CoreLib/src/System/DateTime.Unix.cs

hez2010 · 2025-01-06T07:44:49Z

src/libraries/Common/src/System/TimeProvider.cs

-        private static readonly long s_minDateTicks = DateTime.MinValue.Ticks;
-        private static readonly long s_maxDateTicks = DateTime.MaxValue.Ticks;
-


readonly statics are JIT-time constants, removing this doesn't improve any performance.

class C { static readonly long s_minDateTicks = DateTime.MinValue.Ticks; static readonly long s_maxDateTicks = DateTime.MaxValue.Ticks; [MethodImpl(MethodImplOptions.NoInlining)] static long GetMinTicks() => s_minDateTicks; [MethodImpl(MethodImplOptions.NoInlining)] static long GetMaxTicks() => s_maxDateTicks; }

Tier-1 codegen for GetMinTicks and GetMaxTicks:

; Assembly listing for method C:GetMinTicks():long (Tier1) G_M18344_IG01: ;; offset=0x0000 ;; size=0 bbWeight=1 PerfScore 0.00 G_M18344_IG02: ;; offset=0x0000 xor eax, eax ;; size=2 bbWeight=1 PerfScore 0.25 G_M18344_IG03: ;; offset=0x0002 ret ;; size=1 bbWeight=1 PerfScore 1.00 ; Assembly listing for method C:GetMaxTicks():long (Tier1) G_M12726_IG01: ;; offset=0x0000 ;; size=0 bbWeight=1 PerfScore 0.00 G_M12726_IG02: ;; offset=0x0000 mov rax, 0x2BCA2875F4373FFF ;; size=10 bbWeight=1 PerfScore 0.25 G_M12726_IG03: ;; offset=0x000A ret ;; size=1 bbWeight=1 PerfScore 1.00

They are not guaranteed to be JIT time constants and only work well with tiered compilation. New code should try to avoid such fields for constant values.
Previous discussion: #58169 (comment)

For runtime doesn't run tiered compilation like NativeAOT, static readonly fields like the above are always preinitialized so this won't be a problem.

static readonly fields like the above are always preinitialized so this won't be a problem.

That's not correct in this case today. DateTime.MaxValue.Ticks is too complicated expression for NAOT compiler to interpret at build time: https://godbolt.org/z/bdcErcEa6

src/libraries/Common/src/System/TimeProvider.cs

stephentoub · 2025-01-06T14:14:58Z

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

@@ -293,48 +292,57 @@ public DateTime(int year, int month, int day, int hour, int minute, int second,
            if (second != 60 || !SystemSupportsLeapSeconds)
            {
                ulong ticks = calendar.ToDateTime(year, month, day, hour, minute, second, millisecond).UTicks;
-                _dateData = ticks | ((ulong)kind << KindShift);
+                _dateData = ticks | ((ulong)(uint)kind << KindShift);


If there's an improvement to be gained from the intermediate cast, shouldn't the JIT be able to just generate better code accordingly, given it knows from L290 that kind is in range of a uint?
cc: @EgorBo

stephentoub · 2025-01-06T14:17:24Z

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

            }
        }

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static ulong WithLeapSecond(Calendar calendar, int year, int month, int day, int hour, int minute, int millisecond, DateTimeKind kind)


What is the benefit of this manual outlining? Does the ctor get inlined automatically with this factored out but not with it left in line?

The ctors are quite inlineable in both cases, but this just reduces the code size footprint of leap seconds even further: https://www.diffchecker.com/TbFmWwyz/

stephentoub · 2025-01-06T14:17:57Z

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

-                // codeql[cs/leap-year/unsafe-date-construction-from-two-elements] - DateTime is constructed using the user specified values, not a combination of different sources.  It would be intentional to throw if an invalid combination occurred.
-                this = new DateTime(year, month, day, hour, minute, 59);
-                ValidateLeapSecond();
+                _dateData = WithLeapSecond(ticks, hour, minute);


Same question

The ctors that don't use Calendar now take advantage of the fact that year/month/day (+kind) can be converted to ticks before leap seconds check, thus reducing the number of ctor variants that need to directly handle leap seconds and saving on code size: https://www.diffchecker.com/Iuqa8iGs/

stephentoub · 2025-01-06T14:27:35Z

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormatInfo.cs

-            CurrentInfo; // Couldn't get anything, just use currentInfo as fallback
+        public static DateTimeFormatInfo GetInstance(IFormatProvider? provider)
+        {
+            return provider == null ? CurrentInfo : GetProviderNonNull(provider);


This appears to be optimizing for the case where provider is null at the expense of more code at the call site... why is that desirable?

I took this approach from NumberFormatInfo which looks like it has been more thoroughly optimized. It adds a small code size increase on some callsites (but not all - in case of const provider = null it's an improvement).

stephentoub · 2025-01-06T14:29:17Z

Thanks. Can you share benchmarks/numbers?

pentp · 2025-01-06T16:09:23Z

Some benchmarks based on real-world use case of DateTimeOffset:

BASELINE	Mean	Error	StdDev
CacheEntry_SetAbsoluteExpiration	38.0316 ns	0.4504 ns	0.2979 ns
CacheEntry_GetAbsoluteExpiration	7.7407 ns	0.0972 ns	0.0508 ns

PR	Mean	Error	StdDev
CacheEntry_SetAbsoluteExpiration	26.8186 ns	0.1775 ns	0.1056 ns
CacheEntry_GetAbsoluteExpiration	6.1049 ns	0.0358 ns	0.0187 ns

Benchmark code distilled from a codebase using IMemoryCache

ICacheEntry _cacheEntry = new MemoryCache(new MemoryCacheOptions()).CreateEntry("test").SetAbsoluteExpiration(DateTimeOffset.UtcNow);
TimeSpan _defaultAbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(30);

[Benchmark]
public void CacheEntry_SetAbsoluteExpiration() => _cacheEntry.AbsoluteExpiration = DateTimeOffset.UtcNow.Add(_defaultAbsoluteExpirationRelativeToNow);

[Benchmark]
public DateTime CacheEntry_GetAbsoluteExpiration() => _cacheEntry.AbsoluteExpiration.GetValueOrDefault().UtcDateTime;

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs

tarekgh · 2025-01-06T18:07:44Z

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs

-        private static readonly TimeSpan MaxOffset = TimeSpan.FromHours(14.0);
-        private static readonly TimeSpan MinOffset = -MaxOffset;
+        private static TimeSpan MaxOffset => TimeSpan.FromHours(14);
+        private static TimeSpan MinOffset => TimeSpan.FromHours(-14);


What is the benefit changing this to be not readonly?

To enable them to be JIT/AOT constants in all scenarios, same discussion above: #111112 (comment)

tarekgh · 2025-01-06T18:28:40Z

@pentp thanks for providing this change. Could you please provide the benchmark numbers for the touched public APIs? like the DateTime constructors, AddXXXX methods, etc. I am not expecting any surprises but want to ensure no regressions

pentp · 2025-01-07T13:19:10Z

@pentp thanks for providing this change. Could you please provide the benchmark numbers for the touched public APIs? like the DateTime constructors, AddXXXX methods, etc. I am not expecting any surprises but want to ensure no regressions

Both the constructors and AddMonths/AddYears generate strictly less code now and the calling convention change removes memory accesses from both callers and inside the method: https://www.diffchecker.com/qUu8YUVA/

About the struct calling convention - is this something that the runtime/JIT could automatically do for every int/pointer sized single-field readonly struct for non-virtual methods perhaps? There's the theoretical issue that someone could do Unsafe.AsRef<T>(in this) and get a mutable reference so it might need some struct level opt-in attribute. And I guess there's also the issue that any contained fields would also need to have this attribute or be assumed/asserted to be truly readonly (and even then there might be theoretical perf regressions when calling virtual/interface methods on such a field)...

Optimize DateTimeOffset

3a3a12e

dotnet-issue-labeler bot added the area-System.DateTime label Jan 6, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 6, 2025

huoyaoyuan reviewed Jan 6, 2025

View reviewed changes

Fix build. Address PR feedback.

0694e0a

MichalPetryka reviewed Jan 6, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateTime.Unix.cs Show resolved Hide resolved

hez2010 reviewed Jan 6, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jan 6, 2025

[wasm] Out of memory exception in System.Xml.XmlWriterApiTests.TCFullEndElement tests #110090

Open

stephentoub reviewed Jan 6, 2025

View reviewed changes

pentp added 2 commits January 6, 2025 17:15

Branchless DateTime.Kind

86a7969

Address PR feedback

82f15b7

jkotas reviewed Jan 6, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateTime.cs Show resolved Hide resolved

tarekgh reviewed Jan 6, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs Show resolved Hide resolved

build-analysis bot mentioned this pull request Jan 6, 2025

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

3 tasks

tarekgh reviewed Jan 6, 2025

View reviewed changes

Address PR feedback

ff95bef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize DateTimeOffset #111112

Optimize DateTimeOffset #111112

pentp commented Jan 6, 2025 •

edited

Loading

dotnet-policy-service bot commented Jan 6, 2025

huoyaoyuan left a comment

huoyaoyuan Jan 6, 2025

pentp Jan 6, 2025

huoyaoyuan Jan 6, 2025

pentp Jan 6, 2025

huoyaoyuan Jan 6, 2025

huoyaoyuan Jan 6, 2025

pentp Jan 6, 2025

huoyaoyuan Jan 6, 2025

pentp Jan 6, 2025

hez2010 Jan 6, 2025

pentp Jan 6, 2025

hez2010 Jan 6, 2025

jkotas Jan 6, 2025

stephentoub Jan 6, 2025

stephentoub Jan 6, 2025

pentp Jan 6, 2025

stephentoub Jan 6, 2025

pentp Jan 6, 2025

stephentoub Jan 6, 2025

pentp Jan 6, 2025

stephentoub commented Jan 6, 2025

pentp commented Jan 6, 2025

tarekgh Jan 6, 2025

pentp Jan 7, 2025

tarekgh commented Jan 6, 2025

pentp commented Jan 7, 2025

		public DateTime AddMonths(int months) => AddMonths(this, months);
		private static DateTime AddMonths(DateTime date, int months)

BASELINE	Mean	Error	StdDev
DateTime_Kind	0.9101 ns	0.0050 ns	0.0030 ns
DateTime_KindBranch	0.6964 ns	0.0093 ns	0.0055 ns
DateTime_KindSwitch	2.6310 ns	0.2747 ns	0.1817 ns

SHIFT	Mean	Error	StdDev
DateTime_Kind	0.9214 ns	0.0087 ns	0.0058 ns
DateTime_KindBranch	0.4721 ns	0.0038 ns	0.0025 ns
DateTime_KindSwitch	2.0659 ns	0.0146 ns	0.0087 ns

BRANCHLESS	Mean	Error	StdDev
DateTime_Kind	0.4690 ns	0.0019 ns	0.0010 ns
DateTime_KindBranch	0.5248 ns	0.0039 ns	0.0026 ns
DateTime_KindSwitch	0.7505 ns	0.0083 ns	0.0055 ns

		private static readonly long s_minDateTicks = DateTime.MinValue.Ticks;
		private static readonly long s_maxDateTicks = DateTime.MaxValue.Ticks;

Optimize DateTimeOffset #111112

Are you sure you want to change the base?

Optimize DateTimeOffset #111112

Conversation

pentp commented Jan 6, 2025 • edited Loading

dotnet-policy-service bot commented Jan 6, 2025

huoyaoyuan left a 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

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

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

stephentoub commented Jan 6, 2025

pentp commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Jan 6, 2025

pentp commented Jan 7, 2025

pentp commented Jan 6, 2025 •

edited

Loading