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

[browser][non-icu] HybridGlobalization calendar data #89255

Merged
merged 41 commits into from
Aug 7, 2023

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Jul 20, 2023

Contributes to #79989.

This PR adds ~6k tests to System.Globalization.Hybrid.WASM.Tests.csproj.
Size of icu_hybrid.dat without the calendar data:

  • 500 kB uncompressed (132 kB compressed)

with the calendar data but without collations (hybrid state before this PR):

  • 812 kB (202 kB compressed).

while the original non-hybrid file is:

  • 1 526 kB (329 kB compressed).

The size saving using Hybrid mode will be ~60% after merging this PR (and a corresponding one in icu repo that reduces icudt_hybrid.dat file size).

Public APIs aftected by the change:

  • DateTimeFormatInfo.AbbreviatedDayNames
  • DateTimeFormatInfo.AbbreviatedMonthGenitiveNames
  • DateTimeFormatInfo.AbbreviatedMonthNames
  • DateTimeFormatInfo.CalendarWeekRule
  • DateTimeFormatInfo.DayNames
  • DateTimeFormatInfo.FirstDayOfWeek
  • DateTimeFormatInfo.FullDateTimePattern
  • DateTimeFormatInfo.LongDatePattern
  • DateTimeFormatInfo.LongTimePattern
  • DateTimeFormatInfo.MonthDayPattern
  • DateTimeFormatInfo.MonthGenitiveNames
  • DateTimeFormatInfo.MonthNames
  • DateTimeFormatInfo.NativeCalendarName
  • DateTimeFormatInfo.ShortDatePattern
  • DateTimeFormatInfo.ShortestDayNames
  • DateTimeFormatInfo.ShortTimePattern
  • DateTimeFormatInfo.YearMonthPattern

cc @SamMonoRT

@ilonatommy ilonatommy requested a review from mkhamoyan July 20, 2023 15:15
@ilonatommy ilonatommy self-assigned this Jul 20, 2023
@ghost
Copy link

ghost commented Jul 20, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #79989.

This PR adds ~7k tests to System.Globalization.Hybrid.WASM.Tests.csproj.

ToDo:

  • Add documentation

Public APIs aftected by the change:

  • DateTimeFormatInfo.AbbreviatedDayNames
  • DateTimeFormatInfo.AbbreviatedMonthGenitiveNames
  • DateTimeFormatInfo.AbbreviatedMonthNames
  • DateTimeFormatInfo.CalendarWeekRule
  • DateTimeFormatInfo.DayNames
  • DateTimeFormatInfo.FirstDayOfWeek
  • DateTimeFormatInfo.FullDateTimePattern
  • DateTimeFormatInfo.LongDatePattern
  • DateTimeFormatInfo.LongTimePattern
  • DateTimeFormatInfo.MonthDayPattern
  • DateTimeFormatInfo.MonthGenitiveNames
  • DateTimeFormatInfo.MonthNames
  • DateTimeFormatInfo.NativeCalendarName
  • DateTimeFormatInfo.ShortDatePattern
  • DateTimeFormatInfo.ShortestDayNames
  • DateTimeFormatInfo.ShortTimePattern
  • DateTimeFormatInfo.YearMonthPattern

cc @SamMonoRT

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization

Milestone: -

@ilonatommy ilonatommy marked this pull request as ready for review July 25, 2023 06:20
@radical
Copy link
Member

radical commented Jul 25, 2023

Any suggestions for reviewing this? Anything to look for, or what kinda things might break?

@ilonatommy
Copy link
Member Author

Any suggestions for reviewing this? Anything to look for, or what kinda things might break?

Reviewing in the terms of performance. It might break the System.Globalization lib functionalities (the public APIs that I listed above). I covered it in tests as much as I can, thought. Also, there are test cases that don't work 100% same as dotnet and then they have a comment directly in the same line as test data with the "correct" output that dotnet gives, e.g.
image

if you happen to know any language that has such comment but the new version of output does not make sense to you (regexes might not work well in such cases and I have limited means of checking if JS's answer is legit) then let me know.

@ilonatommy ilonatommy requested a review from mkhamoyan August 2, 2023 15:48
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Big patch!! Great work!

I don't know anything about globalization, so I didn't really review the juicy details of the implementation. I reviewed what I could.

How is the hybrid case being tested? Is that enabled by default? Don't we need two separate test runs with hybrid on, and off?

@ilonatommy
Copy link
Member Author

ilonatommy commented Aug 3, 2023

How is the hybrid case being tested? Is that enabled by default? Don't we need two separate test runs with hybrid on, and off?

There are two test projects: for non-Hybrid:

  • src\libraries\System.Globalization\tests\System.Globalization.Tests.csproj,
  • src\libraries\System.Globalization.Calendars\tests\System.Globalization.Calendars.Tests.csproj

and for Hybrid:

  • src\libraries\System.Globalization\tests\Hybrid\System.Globalization.Hybrid.WASM.Tests.csproj,
  • src\libraries\System.Globalization.Calendars\tests\Hybrid\System.Globalization.Calendars.Hybrid.WASM.Tests.csproj.

@pavelsavara
Copy link
Member

How much larger is dotnet.runtime.js ?

@ilonatommy
Copy link
Member Author

ilonatommy commented Aug 7, 2023

dotnet.runtime.js

~8,7kB
213877 (4415472)-> 222632 (this PR)

@pavelsavara
Copy link
Member

~8,7kB 213877 (4415472)-> 222632 (this PR)

I guess we should make it possible to link it out. Because that cost (not just this PR but all JS in the hybrid) is paid by everybody, not just somebody who enabled hybrid.

@ilonatommy ilonatommy merged commit a9363bf into dotnet:main Aug 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants