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] Trim globalization code #93473

Merged
merged 14 commits into from
Oct 26, 2023
Merged

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Oct 13, 2023

In the measurements a sample WASM app is used with added a few Globalization method calls.

Goal:

  • reduce size of Hybrid Globalization application (sample app: 6 408 kB)
  • reduce size of non-Hybrid Globalization application (sample app: 8 428 kB)

Changes:

  • correct a bug in System.Globalization.Hybrid feature switch substitution,
  • globalization-connected task _GetWasmGenerateAppBundleDependencies that was setting MsBuild properties for feature switches used to be run after IILink task. From this reason, all unset MsBuild properties that were planned to be substituted when they are "false" were ignored. Initial feature switches setup was copied from
  • Blazor trims localizable resources by default, so wasm app should do the same. Because we expect certain exception message in IcuSharding WBT, build setting for that test was changed.
  • Trimming tests used to set feature switches that do not have any effect on trimming when we add default values definitions in WasmApp.targets. They were changed to property switches.
  • Trimming tests DebuggerTypeProxyAttributeTests and DebuggerVisualizerAttributeTests need to set DebuggerSupport=true in order not to have the debugger code trimmed:
    | DebuggerSupport | System.Diagnostics.Debugger.IsSupported | Any dependency that enables better debugging experience to be trimmed when set to false |

    The test used to expect the debugger code to be preserved even though it did not set the property to true.
    Same thing for EventSourcePropertyValueTest, EventSourceManifestTest.

Results:

  • HG: 6 408 kB -> 6 096 kB (-312 kB)
  • non HG: 8 428 kB -> 8 360 kB (-68 kB)

Quoted sizes of AppBundles are not brotlied.

@ghost
Copy link

ghost commented Oct 13, 2023

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

Issue Details

In the measurements a sample WASM app is used with added a few Globalization method calls.

Goal:

  • reduce size of Hybrid Globalization application (sample app: 6 408 kB)
  • reduce size of non-Hybrid Globalization application (sample app: 8 272 kB)

Changes:

  • correct a bug in System.Globalization.Hybrid feature switch substitution,
  • globalization-connected task _GetWasmGenerateAppBundleDependencies that was setting MsBuild properties for feature switches used to be run after IILink task. From this reason, all unset MsBuild properties that were planned to be substituted when they are "false" were ignored. Initial feature switches setup was copied from

Results:

  • HG: 6 408 kB -> 6 096 kB (-312 kB)
  • non HG: 8 272 kB -> 8 360 kB (-108 kB)

Quoted sizes of AppBundles are not brotlied.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization

Milestone: -

@ilonatommy ilonatommy marked this pull request as draft October 13, 2023 14:45
@ilonatommy ilonatommy marked this pull request as ready for review October 19, 2023 08:57
@lewing lewing requested a review from maraf October 19, 2023 14:08
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@ilonatommy ilonatommy merged commit 1556b12 into dotnet:main Oct 26, 2023
184 checks passed
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
* Correct trimming errors.

* Feedback

* Fix WBT

* Allow trimming Settings class on Unix.

* Feature switches setting matter, not properties. Improve the approach to WBT fix.

* Users set properties, not features, so this is what we should test.

* Revert.

* `DebuggerSupport` is false by default

* `EventSourceSupport` is trimmed when not enabled.

* Fix `WasmTestOnBrowser` scenario lib tests.

* Fixing another batch of lib failures.

* Fix `System.Runtime` tests + partial feedback.

* @radical's feedback

* Moving common properties to manifest.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 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