-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Comments
Tagging subscribers to this area: @dotnet/area-system-datetime Issue DetailsBackground and motivationIt was discovered in dotnet/android#8050 that on Android, #88368 made an effort to improve the performance of 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 API Proposalnamespace 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 DesignsNo response RisksNo risk, this API would emulate
|
@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 |
Good point! If we leveraged a config switch would it be something like
Or does it look like something else? |
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 |
Wouldn't it be acceptable to simply stop sorting by |
@svick no, it's documented that they are sorted. dotnet/android#8050 (comment) |
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. |
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 ? |
@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 |
@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. |
@JeroenBer I'll look more to see if we can do something sooner. I'll let you know. |
@tarekgh Thanks, please find out whatever solution you can do. |
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);
}
} |
@JeroenBer we have introduced a new API |
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 theDisplayName
internal field accounts for approximately 60% of theGetSystemTimeZones
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 toDisplayName
,StandardName
, andDaylightName
instead of populating them within the privateTimeZoneInfo(byte[] data, string id, bool dstDisabled)
unix constructor. However, asGetSystemTimeZones
is documented to be sorted onDisplayName
(after sorting onBaseUtcOffset
), it will still take a significant (on Android and mobile) amount of time for users to grab a collection ofTimeZoneInfo
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 toGetSystemTimeZones
without the need to sort byDisplayName
that provides users a more performant option to grab all system time zones asTimeZoneInfo
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
Alternative Designs
No response
Risks
No risk, this API would emulate
GetSystemTimeZones()
without incurring the cost of populatingDisplayName
and sorting.The text was updated successfully, but these errors were encountered: