Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimeProvider GetLocalNow enhancement #90060

Closed
WeihanLi opened this issue Aug 5, 2023 · 9 comments · Fixed by #90066
Closed

TimeProvider GetLocalNow enhancement #90060

WeihanLi opened this issue Aug 5, 2023 · 9 comments · Fixed by #90066
Labels
area-System.DateTime needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@WeihanLi
Copy link
Contributor

WeihanLi commented Aug 5, 2023

Currently, we're creating a new DateTimeOffset even when the local time zone is the same as UTC

public DateTimeOffset GetLocalNow()
{
DateTimeOffset utcDateTime = GetUtcNow();
TimeZoneInfo zoneInfo = LocalTimeZone;
if (zoneInfo is null)
{
#if SYSTEM_PRIVATE_CORELIB
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_TimeProviderNullLocalTimeZone);
#else
throw new InvalidOperationException(SR.InvalidOperation_TimeProviderNullLocalTimeZone);
#endif // SYSTEM_PRIVATE_CORELIB
}
TimeSpan offset = zoneInfo.GetUtcOffset(utcDateTime);
long localTicks = utcDateTime.Ticks + offset.Ticks;
if ((ulong)localTicks > (ulong)s_maxDateTicks)
{
localTicks = localTicks < s_minDateTicks ? s_minDateTicks : s_maxDateTicks;
}
return new DateTimeOffset(localTicks, offset);
}

Could we use the utc now when the local time zone is UTC, possible update:

TimeSpan offset = zoneInfo.GetUtcOffset(utcDateTime);
+ if (offset.Ticks is 0) return utcDateTime;
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 5, 2023
@ghost
Copy link

ghost commented Aug 5, 2023

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

Issue Details

Currently, we're creating a new DateTimeOffset even when the local time zone is the same as UTC

public DateTimeOffset GetLocalNow()
{
DateTimeOffset utcDateTime = GetUtcNow();
TimeZoneInfo zoneInfo = LocalTimeZone;
if (zoneInfo is null)
{
#if SYSTEM_PRIVATE_CORELIB
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_TimeProviderNullLocalTimeZone);
#else
throw new InvalidOperationException(SR.InvalidOperation_TimeProviderNullLocalTimeZone);
#endif // SYSTEM_PRIVATE_CORELIB
}
TimeSpan offset = zoneInfo.GetUtcOffset(utcDateTime);
long localTicks = utcDateTime.Ticks + offset.Ticks;
if ((ulong)localTicks > (ulong)s_maxDateTicks)
{
localTicks = localTicks < s_minDateTicks ? s_minDateTicks : s_maxDateTicks;
}
return new DateTimeOffset(localTicks, offset);
}

Could we use the utc now when the local time zone is UTC, possible update:

TimeSpan offset = zoneInfo.GetUtcOffset(utcDateTime);
+ if (offset.Ticks == 0) return utcDateTime;
Author: WeihanLi
Assignees: -
Labels:

untriaged, area-System.DateTime

Milestone: -

@tarekgh tarekgh added this to the Future milestone Aug 5, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 5, 2023
@tarekgh
Copy link
Member

tarekgh commented Aug 5, 2023

@WeihanLi did you measure it and see noticeable perf improvement?

@tarekgh tarekgh added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 5, 2023
@ghost
Copy link

ghost commented Aug 5, 2023

This issue has been marked needs-author-action and may be missing some important information.

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Aug 5, 2023

@tarekgh here's the test on my local device
image

test code:

[MemoryDiagnoser]
public class TimeProviderLocalTimeTest
{
    private static readonly long s_minDateTicks = DateTime.MinValue.Ticks;
    private static readonly long s_maxDateTicks = DateTime.MaxValue.Ticks;
    
    [Benchmark(Baseline = true)]
    public DateTimeOffset GetLocalNow() 
    {
        var utcDateTime = DateTimeOffset.UtcNow; 
        var zoneInfo = TimeZoneInfo.Utc;
        var offset = zoneInfo.GetUtcOffset(utcDateTime); 
  
        var localTicks = utcDateTime.Ticks + offset.Ticks; 
        if ((ulong)localTicks > (ulong)s_maxDateTicks) 
        { 
            localTicks = localTicks < s_minDateTicks ? s_minDateTicks : s_maxDateTicks; 
        } 
  
        return new DateTimeOffset(localTicks, offset); 
    }
    
    [Benchmark]
    public DateTimeOffset GetLocalNow_Update() 
    { 
        var utcDateTime = DateTimeOffset.UtcNow; 
        var zoneInfo = TimeZoneInfo.Utc;
        var offset = zoneInfo.GetUtcOffset(utcDateTime);
        if (offset.Ticks is 0) return utcDateTime;
  
        var localTicks = utcDateTime.Ticks + offset.Ticks; 
        if ((ulong)localTicks > (ulong)s_maxDateTicks) 
        { 
            localTicks = localTicks < s_minDateTicks ? s_minDateTicks : s_maxDateTicks; 
        }
        return new DateTimeOffset(localTicks, offset); 
    }
}

https://github.com/WeihanLi/PerformanceTest/blob/master/PerformanceTest/TimeProviderLocalTimeTest.cs

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Aug 5, 2023
@tarekgh
Copy link
Member

tarekgh commented Aug 5, 2023

Thanks @WeihanLi, could you please add the measurement when using non-UTC zone too? to compare the perf cost for the added condition too.

@tarekgh tarekgh added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Aug 5, 2023
@ghost
Copy link

ghost commented Aug 5, 2023

This issue has been marked needs-author-action and may be missing some important information.

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Aug 5, 2023

@tarekgh test result for the non-UTC test
image

I'm using the TimeZoneInfo.Local, and my local time zone is UTC+8

[MemoryDiagnoser]
public class TimeProviderLocalTimeNonUtcTest
{
    private static readonly long s_minDateTicks = DateTime.MinValue.Ticks;
    private static readonly long s_maxDateTicks = DateTime.MaxValue.Ticks;
    
    [Benchmark(Baseline = true)]
    public DateTimeOffset GetLocalNow() 
    {
        var utcDateTime = DateTimeOffset.UtcNow; 
        var zoneInfo = TimeZoneInfo.Local;
        var offset = zoneInfo.GetUtcOffset(utcDateTime); 
  
        var localTicks = utcDateTime.Ticks + offset.Ticks; 
        if ((ulong)localTicks > (ulong)s_maxDateTicks) 
        { 
            localTicks = localTicks < s_minDateTicks ? s_minDateTicks : s_maxDateTicks; 
        } 
  
        return new DateTimeOffset(localTicks, offset); 
    }
    
    [Benchmark]
    public DateTimeOffset GetLocalNow_Update() 
    { 
        var utcDateTime = DateTimeOffset.UtcNow; 
        var zoneInfo = TimeZoneInfo.Local;
        var offset = zoneInfo.GetUtcOffset(utcDateTime);
        if (offset.Ticks is 0) return utcDateTime;
  
        var localTicks = utcDateTime.Ticks + offset.Ticks; 
        if ((ulong)localTicks > (ulong)s_maxDateTicks) 
        { 
            localTicks = localTicks < s_minDateTicks ? s_minDateTicks : s_maxDateTicks; 
        }
        return new DateTimeOffset(localTicks, offset); 
    }
}

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Aug 5, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 5, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 6, 2023
@TonyValenti
Copy link

If I read these stats correctly, it seems like the rare but least-expected, best-case scenario got slightly better but the most expected, normal-case scenario got slightly worse.

Is that correct?

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Aug 7, 2023

If I read these stats correctly, it seems like the rare but least-expected, best-case scenario got slightly better but the most expected, normal-case scenario got slightly worse.

From the test result Ratio, we would get 17% performance gains for the UTC time zone, for non-UTC timezones, we could still get almost the same performance as before, from the Mean result, 0.27ns downgrade for non-UTC time zones, could it be accepted?

In our cases, our app services are deployed with docker containers and the default timezone, which would be UTC timezone, we would try to use the Utc time while local time may also be used by mistake.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DateTime needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants