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

Reduced ICU files for HybridGlobalization #300

Merged

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Dec 27, 2022

First version of reduced ICU files for HybridGlobalization, Browser. Contributes to dotnet/runtime#79989.

HybridGlobalization is an optional function that can be switched on by the developer by adding

<HybridGlobalization>true</HybridGlobalization>

to the wasm project.

In that case, instead of loading default icu files, reduced icu files would be loaded to the app memory, correspondingly:

  • icudt_wasm.dat instead of icudt.dat
  • icudt_EFIGS_wasm.dat instead of icudt_EFIGS.dat
  • icudt_CJK_wasm.dat instead of icudt_CJK.dat
  • icudt_no_CJK_wasm.dat instead of icudt_no_CJK.dat.
file original uncompressed [bytes] original compressed [bytes] new _wasm uncompressed [bytes] new _wasm compressed [bytes] reduction rate [%] (compressed)
icudt.dat 1 526 128 329 770 812 560 202 330 39%
icudt_CJK.dat 957 376 249 025 554768 146 874 41%
icudt_EFIGS.dat 548 384 143 345 539 440 142 615 0,5%
icudt_no_CJK.dat 1 107 168 222 250 778 144 194 597 12%
SUM 4 139 056 944 390 2 684 912 686 416 27%

Because savings on some shards are marginal, sharding will not be enabled in Hybrid Globalization mode. From this reason, we are creating only one new file - for full ICU: icudt_wasm.dat.
cc @directhex, @akoeplinger - it will make the runtime pack increase.
The size of runtime pack will be increased by icudt_wasm.dat size: 812kB (202kB brotli copressed).

We cannot stop shipping the original icu files and make HybridGlobalization the default and only option because HybridGlobalization does not provide 100% coverage with the current internalization behavior.

cc @SamMonoRT @tarekgh

@ilonatommy ilonatommy marked this pull request as draft April 12, 2023 08:25
@SamMonoRT
Copy link
Member

@ilonatommy - to clarify, the true property will control if we use the new .dat file correct ? So when that is set to false (default case), we will not see a size increase as it uses the only the old file ?

@ilonatommy
Copy link
Member Author

@ilonatommy - to clarify, the true property will control if we use the new .dat file correct ? So when that is set to false (default case), we will not see a size increase as it uses the only the old file ?

Yes, the property controls weather we will load only the new, icudt_browser.dat file (for true) or we will load the runtime assets as we used to before (false/null). The default option is the latter.

@SamMonoRT
Copy link
Member

Changes LGTM. @akoeplinger @directhex please take a look.
@ilonatommy if these changes aren't necessary, please don't merge prior to Preview 4 fork. (I'm uncertain when these will be pulled down for P4 branching).

@directhex
Copy link

Are we not checking in a binary copy, to avoid the "random non-deterministic builds" issue?

@ilonatommy
Copy link
Member Author

Are we not checking in a binary copy, to avoid the "random non-deterministic builds" issue?

What do you mean by checking in binary copy?

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

one comment, looks good otherwise

eng/icu.mk Show resolved Hide resolved
@akoeplinger
Copy link
Member

Are we not checking in a binary copy, to avoid the "random non-deterministic builds" issue?

What do you mean by checking in binary copy?

The prebuilt file that we check into eng/prebuilts. I do see it in the PR, but maybe we should also let it be generated by @directhex's machine since it tends to produce the smallest binaries.

@ilonatommy
Copy link
Member Author

maybe we should also let it be generated by @directhex's machine since it tends to produce the smallest binaries.

@directhex, if you could help with that, then I will update the existing prebuilt with your version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants