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

[API Proposal]: Add variation of TimeZoneInfo.GetSystemTimeZones() not sorted on DisplayName #88691

Closed
mdh1418 opened this issue Jul 11, 2023 · 14 comments · Fixed by #89985
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime
Milestone

Comments

@mdh1418
Copy link
Member

mdh1418 commented Jul 11, 2023

Edited by @tarekgh

Background and motivation

It was discovered in dotnet/android#8050 that on Android, System.TimeZoneInfo.GetSystemTimeZones() is noticeably slow. Investigation revealed that populating the DisplayName internal field accounts for approximately 60% of the GetSystemTimeZones execution time (timings done on an Android emulator running on 2020 M1 mac mini).

#88368 made an effort to improve the performance of GetSystemTimeZones by lazy initializing the fields corresponding to DisplayName, StandardName, and DaylightName instead of populating them within the private TimeZoneInfo(byte[] data, string id, bool dstDisabled) unix constructor. However, as GetSystemTimeZones is documented to be sorted on DisplayName (after sorting on BaseUtcOffset), it will still take a significant (on Android and mobile) amount of time for users to grab a collection of TimeZoneInfo objects corresponding to the system time zones.

As there is currently no other API to grab system time zones, users are left with no choice other than enduring the cost of populating DisplayName even if they do not utilize said property. This proposal looks to introduce an API akin to GetSystemTimeZones without the need to sort by DisplayName that provides users a more performant option to grab all system time zones as TimeZoneInfo objects.

API Proposal

    namespace System;

    public sealed partial class TimeZoneInfo : IEquatable<TimeZoneInfo?>, ISerializable, IDeserializationCallback
    {
        public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZones()
+       public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZones(bool unsorted)
    }

API Usage

var myTimeZones = TimeZoneInfo.GetSystemTimeZones(unsorted: true);

Alternative Designs

No response

Risks

No risk, this API would emulate GetSystemTimeZones() without incurring the cost of populating DisplayName and sorting.

@mdh1418 mdh1418 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 11, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 11, 2023
@ghost
Copy link

ghost commented Jul 11, 2023

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

Issue Details

Background and motivation

It was discovered in dotnet/android#8050 that on Android, System.TimeZoneInfo.GetSystemTimeZones() is noticeably slow. Investigation revealed that populating the DisplayName internal field accounts for approximately 60% of the GetSystemTimeZones execution time (timings done on an Android emulator running on 2020 M1 mac mini).

#88368 made an effort to improve the performance of GetSystemTimeZones by lazy initializing the fields corresponding to DisplayName, StandardName, and DaylightName instead of populating them within the private TimeZoneInfo(byte[] data, string id, bool dstDisabled) unix constructor. However, as GetSystemTimeZones is documented to be sorted on DisplayName (after sorting on BaseUtcOffset), it will still take a significant (on Android and mobile) amount of time for users to grab a collection of TimeZoneInfo objects corresponding to the system time zones.

As there is currently no other API to grab system time zones, users are left with no choice other than enduring the cost of populating DisplayName even if they do not utilize said property. This proposal looks to introduce an API akin to GetSystemTimeZones without the need to sort by DisplayName that provides users a more performant option to grab all system time zones as TimeZoneInfo objects.

API Proposal

namespace System;

private sealed partial class CachedData
{
    ....
    public bool _isSystemTimeZonesSorted;
}

/// <summary>
/// Returns a <see cref="ReadOnlyCollection{TimeZoneInfo}"/> containing all valid TimeZone's
/// from the local machine. The entries in the collection are unsorted.
/// This method does *not* throw TimeZoneNotFoundException or InvalidTimeZoneException.
/// </summary>
public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZonesUnsorted()
{
    CachedData cachedData = s_cachedData;

    lock (cachedData)
    {
        if (cachedData._readOnlySystemTimeZones == null)
        {
            PopulateAllSystemTimeZones(cachedData);
            cachedData._allSystemTimeZonesRead = true;
            cachedData._isSystemTimeZonesSorted = true;

            cachedData._readOnlySystemTimeZones = cachedData._systemTimeZones ??
                                                  new ReadOnlyCollection<TimeZoneInfo>(cached._systemTimeZones.Values) :
                                                  ReadOnlyCollection<TimeZoneInfo>.Empty;
        }
    }

    return cachedData._readOnlySystemTimeZones;
}

API Usage

// Grab all system timezones without incurring the cost of populating `DisplayName`
var myTimeZones = TimeZoneInfo.GetSystemTimeZonesUnsorted();

Alternative Designs

No response

Risks

No risk, this API would emulate GetSystemTimeZones() without incurring the cost of populating DisplayName and sorting.

Author: mdh1418
Assignees: -
Labels:

api-suggestion, area-System.DateTime

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Jul 11, 2023

@mdh1418 can support this with a config switch instead of introducing a new API. If the API is necessary, it will be better to have overload TimeZoneInfo.GetSystemTimeZones(bool sortWithIds). Doing the switch or the environment variable be faster to get this done soon.

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

mdh1418 commented Jul 11, 2023

Good point! If we leveraged a config switch would it be something like

Object? shouldNotSort = AppContext.GetData("System.TimeZoneInfo.DontSortSystemTimeZones");

Or does it look like something else?

@tarekgh
Copy link
Member

tarekgh commented Jul 11, 2023

Look at https://github.com/dotnet/runtime/blob/eabea90040d359e9552e2821d6f7ab3e249a9a4d/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs#L15C170-L15C170 as an example. I would use a name like System.TimeZoneInfo.SortSystemTimeZonesUsingIds or similar.

@svick
Copy link
Contributor

svick commented Jul 12, 2023

Wouldn't it be acceptable to simply stop sorting by DisplayName in the existing method?

@teo-tsirpanis
Copy link
Contributor

@svick no, it's documented that they are sorted. dotnet/android#8050 (comment)

@tarekgh
Copy link
Member

tarekgh commented Jul 12, 2023

Wouldn't it be acceptable to simply stop sorting by DisplayName in the existing method?

As I indicated before, usually users will use the list to display it in the UI. Sorting it with the display name is the correct thing to do here. @mdh1418 already logged a new issue to give the option sorting with Ids. This will make it safer not to make any breaking change.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Jul 12, 2023
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Jul 13, 2023
@JeroenBer
Copy link

Any news on this ? Because we really need a solution for this. Since people seem to be worried about breaking the sorting backwards compatibility the best option seems to be adding an extra overloaded function to be able to retrieve them sorted by Id's instead of displayname with more performance, our application doesn't need the displayname. @tarekgh said: @mdh1418 already logged a new issue to give the option sorting with Ids. I don't see a new issue logged for this ?

@tarekgh tarekgh modified the milestones: Future, 9.0.0 Jul 28, 2023
@tarekgh
Copy link
Member

tarekgh commented Jul 28, 2023

@JeroenBer this issue you are replying here is tracking adding the overload. Unfortunately, this is not going to be available before the next release. We may look at any other workaround meanwhile till we expose the new overload. My first thought is in your app startup launch a background task to enumerate the zones so, will be ready when you want to use it later. Or enumerate the names of the zones from the device manually and then call FindSystemTimeZoneById when need to create the zone object.

@JeroenBer
Copy link

JeroenBer commented Jul 28, 2023

@tarekgh I don't think that's acceptable. We need this resolved for migrating from Xamarin to .NET8, see the original issue dotnet/android#8050, this API call has become an absolute performance killer when migrating. On slow mobile devices this API call can now take well over a second and in our app we need this at startup. I didn't even benchmark on slow Android devices but I wouldn't be surprised if it would take 5 seconds.
Spending an extra second or more on startup is not acceptable. Also people not aware of these performance problems will experience major hickups when accidentally using this on the GUI thread.
In that case it would be better to just break compatibility and change it for .NET 8 release. Just update the doc with new sorting criteria not based on the display name, but on ID and offset. Anyone who really needs that can sort it afterwards on DisplayName.
Just leaving this issue to wait for milestone 9 at the end of 2024 is the worst option.

@tarekgh
Copy link
Member

tarekgh commented Jul 28, 2023

@JeroenBer I'll look more to see if we can do something sooner. I'll let you know.

@JeroenBer
Copy link

@tarekgh Thanks, please find out whatever solution you can do.

@tarekgh tarekgh modified the milestones: 9.0.0, 8.0.0 Jul 31, 2023
@tarekgh tarekgh added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 31, 2023
@tarekgh tarekgh self-assigned this Jul 31, 2023
@bartonjs
Copy link
Member

bartonjs commented Aug 3, 2023

Video

  • We discussed boolean vs an enumeration, and feel confident that the bool will last us a while.
  • We then spent a long time on naming, and renamed unsorted to skipSorting
namespace System
{
    public sealed partial class TimeZoneInfo : IEquatable<TimeZoneInfo?>, ISerializable, IDeserializationCallback
    {
        // Existing
        //public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZones();
        
        // New
        public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZones(bool skipSorting);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 3, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2023
@tarekgh
Copy link
Member

tarekgh commented Aug 4, 2023

@JeroenBer we have introduced a new API TimeZoneInfo.GetSystemTimeZones(true) which you can call and should be faster.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.