[libs][TimeZoneInfo] Add switch for unsorted GetSystemTimeZones #88794
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-datetime Issue DetailsFixes #88691 This PR adds a switch to enable a more performant, yet unsorted GetSystemTimeZones() timing:
|
|
I opted to go for a completely unsorted collection, as that would be the most performant, saving even the cost of sorting based on The resulting collection is mostly sorted on `Id` anyways.UTC |
|
cc @JeroenBer |
| @@ -33,6 +33,7 @@ internal enum TimeZoneInfoOptions | |||
| [TypeForwardedFrom("System.Core, Version=3.5.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] | |||
| public sealed partial class TimeZoneInfo : IEquatable<TimeZoneInfo?>, ISerializable, IDeserializationCallback | |||
| { | |||
| internal static bool Unsorted { get; } = AppContextConfigHelper.GetBooleanConfig("System.TimeZoneInfo.SystemTimeZonesUnsorted", "DOTNET_SYSTEM_TIME_ZONE_INFO_SYSTEM_TIME_ZONES_UNSORTED"); | |||
There was a problem hiding this comment.
word UNSORTED is confusing too. maybe we can use DOTNET_SYSTEM_TIME_ZONES_SORT_USING_ID?
There was a problem hiding this comment.
From the note in the description (I realized I didnt put it in the description... just a comment, sorry), I was aiming to implement the most performant variant of GetSystemTimeZones(), which also avoids the cost of sorting completely.
| List<TimeZoneInfo> unsortedSystemTimeZones = new List<TimeZoneInfo>(cachedData._systemTimeZones.Values); | ||
| cachedData._readOnlySystemTimeZones = new ReadOnlyCollection<TimeZoneInfo>(unsortedSystemTimeZones); | ||
| } | ||
| else |
There was a problem hiding this comment.
why all this change? isn't enough only change line 924 to test Unsorted and decide to use the Id or DisplayName.
I don't think we need to return unsorted list at all. I think the perf hit coming from display name which is we try to avoid here.
There was a problem hiding this comment.
I was trying to cover the scenario in which a user grabs an unsorted collection and then changes their mind and grabs a sorted collection.
There was a problem hiding this comment.
With that in mind, for the second call, the scenarios in which cachedData._readOnlySystemTimeZones != null are
- Empty case
- Unsorted Case
- Sorted Case
And the topmost early return I was thinking if users already did the work to calculate the sorted collection, only return the sorted collection if there are future calls, ignoring calls to get an unsorted collection (I dont know what value there would be in allowing users to get the unsorted collection after already having a sorted collection as at that point they already hit the performance cost).
There was a problem hiding this comment.
Sorry, I forgot to mention that a note in the description comment was that I figured since part of the point of this version was to be the most performant version of GetSystemTimeZones that it would just avoid sorting altogether, so the users wont incur any extra performance cost. It turned out though, that the unsorted list is mostly sorted by IDs except for having UTC at the beginning.
| @@ -33,6 +33,7 @@ internal enum TimeZoneInfoOptions | |||
| [TypeForwardedFrom("System.Core, Version=3.5.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] | |||
| public sealed partial class TimeZoneInfo : IEquatable<TimeZoneInfo?>, ISerializable, IDeserializationCallback | |||
| { | |||
| internal static bool Unsorted { get; } = AppContextConfigHelper.GetBooleanConfig("System.TimeZoneInfo.SystemTimeZonesUnsorted", "DOTNET_SYSTEM_TIME_ZONE_INFO_SYSTEM_TIME_ZONES_UNSORTED"); | |||
There was a problem hiding this comment.
The API is documented to return sorted timezones. How are people going to figure out when it is safe to set this switch and that it is not going to introduce subtle breaks in their app?
There was a problem hiding this comment.
I do not think we should be introducing this config switch. The config switch has major usability issues.
If we believe that it is important to avoid the sorting for perf, we should introduce the API overload that skips the sorting as proposed in the issue.
There was a problem hiding this comment.
Because the default behavior of the switch when unset is to go with the sorted variation, I was imagining that only the users with performance in mind would look for the switch and enable it. As for when it would be safe to set, if this switch just changes the order of the timezones in the collection, wouldn't it be on the user's responsibility if they found the switch and enabled it?
There was a problem hiding this comment.
The user would need to review usage of this time zone API in all libraries used by their app and repeat this review with every update of libraries used by their app to validate that the switch is not breaking them. It is very poor experience.
There was a problem hiding this comment.
Ah I see, it would affect all instances of GetSystemTimeZones because it would either be an app config or environment variable set. Whereas if we switched to an overload, it would only affect the instances where the overload is supplied as false?
|
I am closing this PR per @jkotas comments and let's pursue the route of adding a new API. |
Fixes #88691
This PR adds a switch to enable a more performant, yet unsorted
GetSystemTimeZones.GetSystemTimeZones() timing:
505ms->171ms(Android Emulator 2020 Mac m1 mini)Opting for a completely unsorted collection over sorting by
Idinstead, as that would be the most performant, saving even the cost of sorting based onBaseUtcOffsetandId.Note: The intended usage of the alternate behavior of this API is to avoid the cost of sorting and avoid the cost of grabbing
DisplayName. As such, there is little value in returning an "unsorted" collection if the collection had already been sorted byDisplayNameearlier in the application's lifetime as the performance cost had already been incurred. So ifGetSystemTimeZonesis already sorted onDisplayName, calls to grab an "unsorted" collection are "ignored" and return the sorted collection.The resulting "unsorted" collection is mostly sorted on `Id` anyways.
UTC
Africa/Abidjan
Africa/Accra
Africa/Addis_Ababa
Africa/Algiers
Africa/Asmara
Africa/Bamako
Africa/Bangui
Africa/Banjul
Africa/Bissau
Africa/Blantyre
Africa/Brazzaville
Africa/Bujumbura
Africa/Cairo
Africa/Casablanca
Africa/Ceuta
Africa/Conakry
Africa/Dakar
Africa/Dar_es_Salaam
Africa/Djibouti
Africa/Douala
Africa/El_Aaiun
Africa/Freetown
Africa/Gaborone
Africa/Harare
Africa/Johannesburg
Africa/Juba
Africa/Kampala
Africa/Khartoum
Africa/Kigali
Africa/Kinshasa
Africa/Lagos
Africa/Libreville
Africa/Lome
Africa/Luanda
Africa/Lubumbashi
Africa/Lusaka
Africa/Malabo
Africa/Maputo
Africa/Maseru
Africa/Mbabane
Africa/Mogadishu
Africa/Monrovia
Africa/Nairobi
Africa/Ndjamena
Africa/Niamey
Africa/Nouakchott
Africa/Ouagadougou
Africa/Porto-Novo
Africa/Sao_Tome
Africa/Tripoli
Africa/Tunis
Africa/Windhoek
America/Adak
America/Anchorage
America/Anguilla
America/Antigua
America/Araguaina
America/Argentina/Buenos_Aires
America/Argentina/Catamarca
America/Argentina/Cordoba
America/Argentina/Jujuy
America/Argentina/La_Rioja
America/Argentina/Mendoza
America/Argentina/Rio_Gallegos
America/Argentina/Salta
America/Argentina/San_Juan
America/Argentina/San_Luis
America/Argentina/Tucuman
America/Argentina/Ushuaia
America/Aruba
America/Asuncion
America/Atikokan
America/Bahia
America/Bahia_Banderas
America/Barbados
America/Belem
America/Belize
America/Blanc-Sablon
America/Boa_Vista
America/Bogota
America/Boise
America/Cambridge_Bay
America/Campo_Grande
America/Cancun
America/Caracas
America/Cayenne
America/Cayman
America/Chicago
America/Chihuahua
America/Costa_Rica
America/Creston
America/Cuiaba
America/Curacao
America/Danmarkshavn
America/Dawson
America/Dawson_Creek
America/Denver
America/Detroit
America/Dominica
America/Edmonton
America/Eirunepe
America/El_Salvador
America/Fort_Nelson
America/Fortaleza
America/Glace_Bay
America/Goose_Bay
America/Grand_Turk
America/Grenada
America/Guadeloupe
America/Guatemala
America/Guayaquil
America/Guyana
America/Halifax
America/Havana
America/Hermosillo
America/Indiana/Indianapolis
America/Indiana/Knox
America/Indiana/Marengo
America/Indiana/Petersburg
America/Indiana/Tell_City
America/Indiana/Vevay
America/Indiana/Vincennes
America/Indiana/Winamac
America/Inuvik
America/Iqaluit
America/Jamaica
America/Juneau
America/Kentucky/Louisville
America/Kentucky/Monticello
America/Kralendijk
America/La_Paz
America/Lima
America/Los_Angeles
America/Lower_Princes
America/Maceio
America/Managua
America/Manaus
America/Marigot
America/Martinique
America/Matamoros
America/Mazatlan
America/Menominee
America/Merida
America/Metlakatla
America/Mexico_City
America/Miquelon
America/Moncton
America/Monterrey
America/Montevideo
America/Montserrat
America/Nassau
America/New_York
America/Nipigon
America/Nome
America/Noronha
America/North_Dakota/Beulah
America/North_Dakota/Center
America/North_Dakota/New_Salem
America/Nuuk
America/Ojinaga
America/Panama
America/Pangnirtung
America/Paramaribo
America/Phoenix
America/Port-au-Prince
America/Port_of_Spain
America/Porto_Velho
America/Puerto_Rico
America/Punta_Arenas
America/Rainy_River
America/Rankin_Inlet
America/Recife
America/Regina
America/Resolute
America/Rio_Branco
America/Santarem
America/Santiago
America/Santo_Domingo
America/Sao_Paulo
America/Scoresbysund
America/Sitka
America/St_Barthelemy
America/St_Johns
America/St_Kitts
America/St_Lucia
America/St_Thomas
America/St_Vincent
America/Swift_Current
America/Tegucigalpa
America/Thule
America/Thunder_Bay
America/Tijuana
America/Toronto
America/Tortola
America/Vancouver
America/Whitehorse
America/Winnipeg
America/Yakutat
America/Yellowknife
Antarctica/Casey
Antarctica/Davis
Antarctica/DumontDUrville
Antarctica/Macquarie
Antarctica/Mawson
Antarctica/McMurdo
Antarctica/Palmer
Antarctica/Rothera
Antarctica/Syowa
Antarctica/Troll
Antarctica/Vostok
Arctic/Longyearbyen
Asia/Aden
Asia/Almaty
Asia/Amman
Asia/Anadyr
Asia/Aqtau
Asia/Aqtobe
Asia/Ashgabat
Asia/Atyrau
Asia/Baghdad
Asia/Bahrain
Asia/Baku
Asia/Bangkok
Asia/Barnaul
Asia/Beirut
Asia/Bishkek
Asia/Brunei
Asia/Chita
Asia/Choibalsan
Asia/Colombo
Asia/Damascus
Asia/Dhaka
Asia/Dili
Asia/Dubai
Asia/Dushanbe
Asia/Famagusta
Asia/Gaza
Asia/Hebron
Asia/Ho_Chi_Minh
Asia/Hong_Kong
Asia/Hovd
Asia/Irkutsk
Asia/Jakarta
Asia/Jayapura
Asia/Jerusalem
Asia/Kabul
Asia/Kamchatka
Asia/Karachi
Asia/Kathmandu
Asia/Khandyga
Asia/Kolkata
Asia/Krasnoyarsk
Asia/Kuala_Lumpur
Asia/Kuching
Asia/Kuwait
Asia/Macau
Asia/Magadan
Asia/Makassar
Asia/Manila
Asia/Muscat
Asia/Nicosia
Asia/Novokuznetsk
Asia/Novosibirsk
Asia/Omsk
Asia/Oral
Asia/Phnom_Penh
Asia/Pontianak
Asia/Pyongyang
Asia/Qatar
Asia/Qostanay
Asia/Qyzylorda
Asia/Riyadh
Asia/Sakhalin
Asia/Samarkand
Asia/Seoul
Asia/Shanghai
Asia/Singapore
Asia/Srednekolymsk
Asia/Taipei
Asia/Tashkent
Asia/Tbilisi
Asia/Tehran
Asia/Thimphu
Asia/Tokyo
Asia/Tomsk
Asia/Ulaanbaatar
Asia/Urumqi
Asia/Ust-Nera
Asia/Vientiane
Asia/Vladivostok
Asia/Yakutsk
Asia/Yangon
Asia/Yekaterinburg
Asia/Yerevan
Atlantic/Azores
Atlantic/Bermuda
Atlantic/Canary
Atlantic/Cape_Verde
Atlantic/Faroe
Atlantic/Madeira
Atlantic/Reykjavik
Atlantic/South_Georgia
Atlantic/St_Helena
Atlantic/Stanley
Australia/Adelaide
Australia/Brisbane
Australia/Broken_Hill
Australia/Darwin
Australia/Eucla
Australia/Hobart
Australia/Lindeman
Australia/Lord_Howe
Australia/Melbourne
Australia/Perth
Australia/Sydney
Europe/Amsterdam
Europe/Andorra
Europe/Astrakhan
Europe/Athens
Europe/Belgrade
Europe/Berlin
Europe/Bratislava
Europe/Brussels
Europe/Bucharest
Europe/Budapest
Europe/Busingen
Europe/Chisinau
Europe/Copenhagen
Europe/Dublin
Europe/Gibraltar
Europe/Guernsey
Europe/Helsinki
Europe/Isle_of_Man
Europe/Istanbul
Europe/Jersey
Europe/Kaliningrad
Europe/Kiev
Europe/Kirov
Europe/Lisbon
Europe/Ljubljana
Europe/London
Europe/Luxembourg
Europe/Madrid
Europe/Malta
Europe/Mariehamn
Europe/Minsk
Europe/Monaco
Europe/Moscow
Europe/Oslo
Europe/Paris
Europe/Podgorica
Europe/Prague
Europe/Riga
Europe/Rome
Europe/Samara
Europe/San_Marino
Europe/Sarajevo
Europe/Saratov
Europe/Simferopol
Europe/Skopje
Europe/Sofia
Europe/Stockholm
Europe/Tallinn
Europe/Tirane
Europe/Ulyanovsk
Europe/Uzhgorod
Europe/Vaduz
Europe/Vatican
Europe/Vienna
Europe/Vilnius
Europe/Volgograd
Europe/Warsaw
Europe/Zagreb
Europe/Zaporozhye
Europe/Zurich
Indian/Antananarivo
Indian/Chagos
Indian/Christmas
Indian/Cocos
Indian/Comoro
Indian/Kerguelen
Indian/Mahe
Indian/Maldives
Indian/Mauritius
Indian/Mayotte
Indian/Reunion
Pacific/Apia
Pacific/Auckland
Pacific/Bougainville
Pacific/Chatham
Pacific/Chuuk
Pacific/Easter
Pacific/Efate
Pacific/Enderbury
Pacific/Fakaofo
Pacific/Fiji
Pacific/Funafuti
Pacific/Galapagos
Pacific/Gambier
Pacific/Guadalcanal
Pacific/Guam
Pacific/Honolulu
Pacific/Kiritimati
Pacific/Kosrae
Pacific/Kwajalein
Pacific/Majuro
Pacific/Marquesas
Pacific/Midway
Pacific/Nauru
Pacific/Niue
Pacific/Norfolk
Pacific/Noumea
Pacific/Pago_Pago
Pacific/Palau
Pacific/Pitcairn
Pacific/Pohnpei
Pacific/Port_Moresby
Pacific/Rarotonga
Pacific/Saipan
Pacific/Tahiti
Pacific/Tarawa
Pacific/Tongatapu
Pacific/Wake
Pacific/Wallis