[WASM] Trim TimeZoneInfo to reduce size #50266
Conversation
|
Tagging subscribers to this area: @tarekgh, @safern Issue DetailsThis removes functions and related code from the browser wasm build target that were added recently with #48931 and #49412. These never get called at runtime because neither time zone display name strings nor IANA to Windows time zone mapping data are included in the ICU data we ship with the browser. Wasm (releae) output sizes on my local machine: Before change:
After change:
I think this should resolve #50210
|
|
@lewing want just to confirm would be ok excluding this function from the browser? |
|
cc @eerhardt |
|
Thanks for the fast turn around! This makes sense and my only thought is that we might be able to break the linkage with a new linker substitution rather than at build time. But that is more of a feature than an issue with this pr. |
marek-safar
added
the
size-reduction
|
Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @tannergooding, @CoffeeFlux Issue DetailsThis removes functions and related code from the browser wasm build target that were added recently with #48931 and #49412. These never get called at runtime because neither time zone display name strings nor IANA to Windows time zone mapping data are included in the ICU data we ship with the browser. Wasm (release) output sizes on my local machine: Before change:
After change:
I think this should resolve #50210
|
| @@ -22,9 +22,22 @@ public sealed partial class TimeZoneInfo | |||
| private const string ZoneTabFileName = "zone.tab"; | |||
| private const string TimeZoneEnvironmentVariable = "TZ"; | |||
| private const string TimeZoneDirectoryEnvironmentVariable = "TZDIR"; | |||
|
|
|||
| #if !TARGET_BROWSER | |||
There was a problem hiding this comment.
Why browser only? We use the same data on other platforms like tvOS
There was a problem hiding this comment.
Is there a list of targets that use the filtered ICU data? Or a different flag I should use?
These can all be trimmed out, anywhere that the full ICU with time zone data is not available.
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
|
Some of the checks failed for unrelated reasons. Would someone with permissions pleas click the button to re-run failed checks? Thanks. I believe this is ready, otherwise. |
|
Restarted the lanes for you. Thanks for your work here! |
There was a problem hiding this comment.
This LGTM. Thanks for the quick turnaround @mattjohnsonpint.
@tarekgh - any thoughts?
|
Merging with Tarek's approval. |
|
We are seeing this gain back in the performance lab as shown here, DrewScoggins/performance-2#4858 |
|
@DrewScoggins - I'm not seeing it on https://aka.ms/dotnetperfstatus:
|
|
I'm a little confused. We see the regression in the image that you posted between Mar 21-28, and then the fix went in and we went back down between Mar 28-Apr 04. Am I missing something? |
|
I'm the one that was confused. By I get it now - you were just confirming that the regression is fixed. |
ghost
locked as resolved and limited conversation to collaborators
This removes functions and related code from the browser wasm build target that were added recently with #48931 and #49412.
These never get called at runtime because neither time zone display name strings nor IANA to Windows time zone mapping data are included in the ICU data we ship with the browser.
Wasm (release) output sizes on my local machine:
Before change:
dotnet.wasm: 2,345 KBdotnet.wasm.gz: 994 KBdotnet.wasm.br: 806 KBSystem.Private.CoreLib.dll: 1,149 KBSystem.Private.CoreLib.dll.gz: 467 KBSystem.Private.CoreLib.dll.br: 392 KBAfter change:
dotnet.wasm: 2,222 KBdotnet.wasm.gz: 940 KBdotnet.wasm.br: 764 KBSystem.Private.CoreLib.dll: 1,147 KBSystem.Private.CoreLib.dll.gz: 466 KBSystem.Private.CoreLib.dll.br: 391 KBI think this should resolve #50210
@eerhardt @lewing