API Proposal: Add OffsetMinutes to DateTimeOffset

[JeremyKuhne]

Background and Motivation

In advanced DateTimeOffset scenarios, getting fast access to whether or not the DateTimeOffset has an offset and what it is in minutes is very useful.

Proposed API

namespace System
{
    public readonly struct DateTimeOffset
	{
	   // Existing field
	   private readonly short _offsetMinutes;
	   
           // Not adding per discussion below
+	   // public bool IsUtc => _offsetMinutes == 0;
+	   public int TotalOffsetMinutes => _offsetMinutes; 
	}
}

Usage Examples

Before

        public Value(DateTimeOffset value)
        {
            this = default;
            TimeSpan offset = value.Offset;
            if (offset.Ticks == 0)
            {
                // This is a UTC time
                _union.Ticks = value.Ticks;
                _object = TypeFlags.DateTimeOffset;
            }
            else if (PackedDateTimeOffset.TryCreate(value, offset, out var packed))
            {
                _union.PackedDateTimeOffset = packed;
                _object = TypeFlags.PackedDateTimeOffset;
            }
            else
            {
                _object = value;
            }
        }

After

        public Value(DateTimeOffset value)
        {
            this = default;
            int offset = value.TotalOffsetMinutes;
            if (offset == 0)
            {
                // This is a UTC time
                _union.Ticks = value.Ticks;
                _object = TypeFlags.DateTimeOffset;
            }
            else if (PackedDateTimeOffset.TryCreate(value, offset, out var packed))
            {
                _union.PackedDateTimeOffset = packed;
                _object = TypeFlags.PackedDateTimeOffset;
            }
            else
            {
                _object = value;
            }
        }

Related to this, DateTimeOffset.Ticks could be calling an internal constructor to avoid the checks in DateTime. That isn't public surface area, but worth mentioning.

cc: @KrzysztofCwalina, @tarekgh, @lonitra

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

FWIW, if we did this, I'd want OffsetMinutes to be typed as int, not short. Makes the API a bit easier to use, and it allows us to change the internal implementation to int in the future as a perf optimization.

[ghost]

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

In advanced DateTimeOffset scenarios, getting fast access to whether or not the DateTimeOffset has an offset and what it is in minutes is very useful.

Proposed API

namespace System
{
    public readonly struct DateTimeOffset
	{
	   // Existing field
	   private readonly short _offsetMinutes;
	   
+	   public bool IsUTC => _offsetMinutes == 0;
+	   public short OffsetMinutes => _offsetMinutes; 
	}
}

Usage Examples

Before

        public Value(DateTimeOffset value)
        {
            this = default;
            TimeSpan offset = value.Offset;
            if (offset.Ticks == 0)
            {
                // This is a UTC time
                _union.Ticks = value.Ticks;
                _object = TypeFlags.DateTimeOffset;
            }
            else if (PackedDateTimeOffset.TryCreate(value, offset, out var packed))
            {
                _union.PackedDateTimeOffset = packed;
                _object = TypeFlags.PackedDateTimeOffset;
            }
            else
            {
                _object = value;
            }
        }

After

        public Value(DateTimeOffset value)
        {
            this = default;
            if (value.IsUTC)
            {
                // This is a UTC time
                _union.Ticks = value.Ticks;
                _object = TypeFlags.DateTimeOffset;
            }
            else if (PackedDateTimeOffset.TryCreate(value, value.OffsetMinutes, out var packed))
            {
                _union.PackedDateTimeOffset = packed;
                _object = TypeFlags.PackedDateTimeOffset;
            }
            else
            {
                _object = value;
            }
        }

Related to this, DateTimeOffset.Ticks could be calling an internal constructor to avoid the checks in DateTime. That isn't public surface area, but worth mentioning.

cc: @KrzysztofCwalina, @tarekgh, @lonitra

Author:	JeremyKuhne
Assignees:	-
Labels:	api-suggestion, area-System.Runtime, untriaged
Milestone:	-

[tarekgh]

I think if we expose OffsetMinutes, we'll not need IsUtc.

[tarekgh]

Also, why thinking using DateTimeOffset.Offset would not be good here? the check will be if (value.Offset == TimeSpan.Zero)`.

I am not seeing much value exposing Offset.Minutes as we already have DateTimeOffset.Offset property which is giving everything we need.

[JeremyKuhne]

I am not seeing much value exposing Offset.Minutes as we already have DateTimeOffset.Offset property which is giving everything we need.

It is about performance, not functionality. Creating a TimeSpan is a significant amount of work:

        public TimeSpan(int hours, int minutes, int seconds)
        {
            _ticks = TimeToTicks(hours, minutes, seconds);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        internal static long TimeToTicks(int hour, int minute, int second)
        {
            // totalSeconds is bounded by 2^31 * 2^12 + 2^31 * 2^8 + 2^31,
            // which is less than 2^44, meaning we won't overflow totalSeconds.
            long totalSeconds = (long)hour * 3600 + (long)minute * 60 + (long)second;
            if (totalSeconds > MaxSeconds || totalSeconds < MinSeconds)
                ThrowHelper.ThrowArgumentOutOfRange_TimeSpanTooLong();
            return totalSeconds * TicksPerSecond;
        }

That is a lot of work for checking _offsetMinutes == 0. In my case I'm also trying to pack the raw data and I need minutes, not ticks. It is quite measurable in my Value prototype which is currently being used by Azure.

[tarekgh]

That is a lot of work for checking _offsetMinutes == 0. In my case I'm also trying to pack the raw data and I need minutes, not ticks. It is quite measurable in my Value prototype which is currently being used by Azure.

This can be optimized in the implementation by creating TimeSpan(_offsetMinutes * TicksPerMinute) and bypass all checks.

[JeremyKuhne]

This can be optimized in the implementation by creating TimeSpan(_offsetMinutes * TicksPerMinute) and bypass all checks.

That is better, but still suboptimal, as I'm still getting out the minutes again.

[Clockwork-Muse]

It is about performance, not functionality. Creating a TimeSpan is a significant amount of work

... but TimeSpan.Zero is a static property... ?

As an aside DateTimeOffset.Offset == TimeSpan.Zero does not mean that that particular DateTimeOffset actually represents a date/time value in Utc. An equivalent instant, sure. However there are other time zones that have zero offsets.

[tarekgh]

but still suboptimal, as I'm still getting out the minutes again.

Accessing the minutes from the offset is not that expensive I guess public int Minutes => (int)((_ticks / TicksPerMinute) % 60);. you can cache the minutes from the offset if you are going to use it more than one time.

[GrabYourPitchforks]

FWIW, here's the current codegen for static int TotalMinutes(DateTimeOffset o) => (int)o.Offset.TotalMinutes;:

C.TotalMinutes(System.DateTimeOffset)
    L0000: sub rsp, 0x28
    L0004: vzeroupper
    L0007: movsx rax, word ptr [rcx]
    L000b: movsxd rax, eax
    L000e: imul rax, 0x3c
    L0012: mov rdx, 0xd6bf94d5e5
    L001c: cmp rax, rdx
    L001f: jg short L0051
    L0021: mov rdx, 0xffffff29406b2a1b
    L002b: cmp rax, rdx
    L002e: jl short L0051
    L0030: imul rax, 0x989680
    L0037: vxorps xmm0, xmm0, xmm0
    L003b: vcvtsi2sd xmm0, xmm0, rax
    L0040: vdivsd xmm0, xmm0, [C.TotalMinutes(System.DateTimeOffset)]
    L0048: vcvttsd2si eax, xmm0
    L004c: add rsp, 0x28
    L0050: ret
    L0051: call 

Edit: Had to fix my codegen above since I was calling get_Minutes instead of get_TotalMinutes.

[JeremyKuhne]

Accessing the minutes from the offset is not that expensive I guess public int Minutes => (int)((_ticks / TicksPerMinute) % 60);. you can cache the minutes from the offset if you are going to use it more than one time.

The issue I have is not getting it out multiple times, it is getting it out for many DateTimeOffset instances. This is code that is already in Azure and is intended to be used more broadly there.

[tarekgh]

@JeremyKuhne got it. if you think exposing it would make noticeable difference then I am ok. I assume we'll not need IsUtc anymore especially @Clockwork-Muse comment #52081 (comment) is a valid comment.

[JeremyKuhne]

I assume we'll not need IsUtc anymore

Yes, it is just a convenience if the minutes are exposed so it isn't critical.

[Clockwork-Muse]

And another thing - minutes are technically not sufficient. Historically there have been timezones with second offsets, usually as the first offset from the zone being set from local solar noon to a more even division.
I'm not aware of any zone with a sub-minute offset currently, but there's nothing preventing a country from adjusting their clock to have such an offset in the future.

[tarekgh]

And another thing - minutes are technically not sufficient. Historically there have been timezones with second offsets

Currently we don't support that. we always round to the minutes.

[Clockwork-Muse]

And another thing - minutes are technically not sufficient. Historically there have been timezones with second offsets

Currently we don't support that. we always round to the minutes.

Or throw an exception on construction.

Is this something that's going to be affected by your changes to TimeZoneInfo?

[tarekgh]

@Clockwork-Muse we can't throw because mostly the TZ data is read from the system when running on Linux. That means if we throw, anyone enumerating the time zones on the system will get exception. Anyway, this is a corner case because it happens with a couple of time zones in the historical old dates which not really a concern.

[Clockwork-Muse]

@tarekgh -
Documentation is wrong then.

[tarekgh]

@Clockwork-Muse the doc is correct we throw if calling the public constructor. what I meant when I said we don't throw, is when we internally reading the TZ data and encounter fractional minutes, we don't throw. we just round the minutes before using it with the DateTimeOffset.

[svick]

Had to fix my codegen above since I was calling get_Minutes instead of get_TotalMinutes.

I think this raises a question about naming: should the property be called TotalOffsetMinutes (or OffsetTotalMinutes) instead of OffsetMinutes, to make it clear it's a shorthand for Offset.TotalMinutes and not Offset.Minutes?

If this confused anybody, they would pretty quickly find there is no OffsetHours, which would probably make them look up the documentation for OffsetMinutes. But wouldn't it be better to make this clearer by using consistent naming?

[JeremyKuhne]

should the property be called TotalOffsetMinutes

Sounds like a better term to me.

[tannergooding]

@tarekgh, any other feedback here or is this in a shape that could be marked ready-for-review?

[tarekgh]

The API (after renaming it to TotalOffsetMinutes) looks good to me. I am not sure if @JeremyKuhne still needs it through for his perf scenario?

[mattjohnsonpint]

LGTM also.

[JeremyKuhne]

I am not sure if @JeremyKuhne still needs it through for his perf scenario?

Yes. :)

[tarekgh]

I have updated the API name in the original proposal. Should it return int instead of short? I know short is enough but looking at the API usage in the code sample looks using int anyway.

[JeremyKuhne]

Should it return int instead of short?

I'd say int is the better choice given @GrabYourPitchforks comment above.

[tarekgh]

Ok, I have updated the proposal and marked it as ready for review.

[bartonjs]

• We discussed potential future limitations and don't believe they'll matter.
  • Dear world governments, please do not add a UTC offset measured in non-minute seconds.
• Looks good as proposed.

namespace System
{
    public readonly partial struct DateTimeOffset
    {
        public int TotalOffsetMinutes => _offsetMinutes; 
    }
}