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

Rework the Windows timezone implementation #390

Merged
merged 6 commits into from
Aug 8, 2024

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Apr 29, 2024

  • Improved memory usage during timezone database initialization: now, buffer allocations are performed outside of loops.
  • Removed the last production-facing piece of cinterop. cinterop is still used in tests.
  • Added a thorough test to check that every timezone transition that we discovered is also a valid transition from the point of view of Windows.
  • Used the test to determine the logic that Windows uses for time zone handling, fixing all inconsistencies but a couple of small ones:
    • In some cases, Windows claims that the transition happens on 23:59:59.999, whereas we can express 24:00:00.000, the value that was actually intended.
    • For a couple of time zones, for years 2030+, Windows seems to use some custom logic that doesn't seem correct anyway, so we don't reimplement it.

@dkhalanskyjb dkhalanskyjb added this to the 0.7.0 milestone May 1, 2024
@dkhalanskyjb dkhalanskyjb changed the title Reuse buffers in the Windows implementation Rework the Windows timezone implementation May 4, 2024
@dkhalanskyjb dkhalanskyjb requested a review from ilya-g May 4, 2024 15:07
@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review May 4, 2024 15:07
@dkhalanskyjb dkhalanskyjb self-assigned this Jul 19, 2024
@dkhalanskyjb dkhalanskyjb modified the milestones: 0.7.0, 0.6.1 Jul 19, 2024
@dkhalanskyjb dkhalanskyjb added the timezone The model and API of timezones label Jul 19, 2024
@@ -152,33 +152,28 @@ internal val standardToWindows: Map<String, String> = mutableMapOf(
"America/Moncton" to "Atlantic Standard Time",
"America/Monterrey" to "Central Standard Time (Mexico)",
"America/Montevideo" to "Montevideo Standard Time",
"America/Montreal" to "Eastern Standard Time",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What caused these removals? Is it no longer a valid time zone?

Copy link
Collaborator Author

@dkhalanskyjb dkhalanskyjb Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of. In the IANA tzdb, the zone.tab file is a list of valid timezones, and, for example, America/Montreal isn't there. We don't take zone.tab into account on the other platforms, and Oracle OpenJDK 22 still thinks there's an America/Montreal and lists it in the list of available time zones. At some point, the CLDR removed some of such zones that are preserved in the IANA tzdb as symlinks for backward compatibility: unicode-org/cldr#3442

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zone.tab doesn't include America/Montreal since 2013: https://mm.icann.org/pipermail/tz-announce/2013-September/000013.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List of zones on the MacOS filesystem but not in the Windows zone name mapping:

Put it under a spoiler, as there's many of them
Africa/Asmara
Africa/Timbuktu
America/Argentina/Buenos_Aires
America/Argentina/Catamarca
America/Argentina/ComodRivadavia
America/Argentina/Cordoba
America/Argentina/Jujuy
America/Argentina/Mendoza
America/Atikokan
America/Atka
America/Ensenada
America/Fort_Wayne
America/Indiana/Indianapolis
America/Kentucky/Louisville
America/Knox_IN
America/Montreal
America/Nipigon
America/Nuuk
America/Pangnirtung
America/Porto_Acre
America/Rainy_River
America/Rosario
America/Santa_Isabel
America/Shiprock
America/Thunder_Bay
America/Virgin
America/Yellowknife
Antarctica/South_Pole
Antarctica/Troll
Asia/Ashkhabad
Asia/Chongqing
Asia/Chungking
Asia/Dacca
Asia/Harbin
Asia/Ho_Chi_Minh
Asia/Istanbul
Asia/Kashgar
Asia/Kathmandu
Asia/Kolkata
Asia/Macao
Asia/Tel_Aviv
Asia/Thimbu
Asia/Ujung_Pandang
Asia/Ulan_Bator
Asia/Yangon
Atlantic/Faroe
Atlantic/Jan_Mayen
Australia/ACT
Australia/Canberra
Australia/Currie
Australia/LHI
Australia/NSW
Australia/North
Australia/Queensland
Australia/South
Australia/Tasmania
Australia/Victoria
Australia/West
Australia/Yancowinna
Brazil/Acre
Brazil/DeNoronha
Brazil/East
Brazil/West
CET
Canada/Atlantic
Canada/Central
Canada/Eastern
Canada/Mountain
Canada/Newfoundland
Canada/Pacific
Canada/Saskatchewan
Canada/Yukon
Chile
Chile/Continental
Chile/EasterIsland
EET
Eire
Etc/GMT+0
Etc/GMT-0
Etc/GMT0
Etc/Greenwich
Etc/UCT
Etc/Universal
Etc/Zulu
Europe/Belfast
Europe/Kyiv
Europe/Nicosia
Europe/Tiraspol
Europe/Uzhgorod
Europe/Zaporozhye
GB
GB-Eire
GMT+0
GMT-0
GMT0
HST
Hongkong
Iceland
Japan
MET
Mexico/BajaNorte
Mexico/BajaSur
Mexico/General
NZ
NZ-CHAT
Navajo
PRC
Pacific/Chuuk
Pacific/Johnston
Pacific/Kanton
Pacific/Pohnpei
Pacific/Samoa
Pacific/Yap
Poland
Portugal
ROC
ROK
UCT
US/Alaska
US/Aleutian
US/Arizona
US/Central
US/East-Indiana
US/Eastern
US/Hawaii
US/Indiana-Starke
US/Michigan
US/Mountain
US/Pacific
US/Samoa
Universal
W-SU
WET
Zulu

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I get right that America/Montreal for example is now an alias to some other zone name, for which we have a mapping?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so if we can get these link mappings, we could allow obtaining zones by these link names from TimeZone.of without listing them in availableZoneIds. But I think this should be a separate PR.

core/native/src/internal/TimeZoneRules.kt Show resolved Hide resolved
@dkhalanskyjb dkhalanskyjb requested a review from ilya-g July 24, 2024 07:36
@dkhalanskyjb dkhalanskyjb force-pushed the windows-without-cinterop branch from 019a06d to 744ed9d Compare August 7, 2024 09:22
@dkhalanskyjb dkhalanskyjb requested a review from ilya-g August 7, 2024 13:11
@dkhalanskyjb dkhalanskyjb merged commit 19e3160 into master Aug 8, 2024
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the windows-without-cinterop branch August 8, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timezone The model and API of timezones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants