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.

Sign up for GitHub

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

Jump to bottom

[WASM] Add ILStrip task to wasm app build process #88926

Merged
merged 21 commits into from
Aug 10, 2023
Merged

[WASM] Add ILStrip task to wasm app build process #88926

merged 21 commits into from
Aug 10, 2023

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Jul 14, 2023 •
edited
Loading

Fixes #88696

With this PR, the customer is able to create an WASM app and enabling IL trimming for AOT compiled methods. By default, this is disabled. To enable it, build the app with WasmStripILAfterAOT =true. This change help producing a smaller WASM app.

Impact of this change is displayed as follows. I measured the size of _framework after compression using brotli for wasm apps with AOT enabled:

  • Wasm app under src/mono/sample/wasm/browser
    • 6.0M -> 5.8M (-p:WasmEnableWebcil=false) (3.3% smaller)
    • 5.9M -> 5.8M (-p:WasmEnableWebcil=true) (1.7% smaller)
  • Wasm app under src/mono/sample/wasm/browser-bench
    • 7.1M -> 6.8M (-p:WasmEnableWebcil=false) (4.2% smaller)
    • 7.1M -> 6.8M (-p:WasmEnableWebcil=true) (4.2% smaller)

@ghost ghost assigned fanyang-mono Jul 14, 2023
@lewing lewing requested a review from vargaz July 14, 2023 19:25
@lewing
Copy link
Member

lewing commented Jul 14, 2023 •
edited
Loading

Are we confident that this handles all the cases where wasm might transition to the interpreter appropriately?

@radical
Copy link
Member

radical commented Jul 14, 2023

Also, test needed in src/mono/wasm/Wasm.Build.Tests.

@radical
Copy link
Member

radical commented Jul 14, 2023

And could you please add some detail about what the PR is doing, like what the default is for the property, and what it will do, and/or affect?

@radical radical added the arch-wasm WebAssembly architecture label Jul 14, 2023
@ghost
Copy link

ghost commented Jul 14, 2023

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

Issue Details

Fixes #88696

Author: fanyang-mono
Assignees: fanyang-mono
Labels:

arch-wasm, area-Build-mono
Milestone: -

@radical radical added this to the 8.0.0 milestone Jul 14, 2023
This was referenced Jul 16, 2023
Closed
Closed
@fanyang-mono fanyang-mono changed the title [WASM] Add ILStrip task to wasm app build process [WASM][WIP] Add ILStrip task to wasm app build process Jul 16, 2023
@vargaz
[mono][aot] Avoid adding some methods to the compiled method file.
074abf0 
* Methods which have 'deopt' set can enter the interpreter during EH.
* Methods which have 'interp_entry_only' set are AOTed, but the AOT
  code is only used to enter the interpreter.
@vargaz vargaz requested a review from lambdageek as a code owner July 17, 2023 16:05
@build-analysis build-analysis bot mentioned this pull request Jul 17, 2023
Closed
@lewing
Copy link
Member

lewing commented Jul 26, 2023

cc @kg

@fanyang-mono fanyang-mono marked this pull request as draft July 26, 2023 20:20
@fanyang-mono fanyang-mono mentioned this pull request Jul 27, 2023
Open
@fanyang-mono
Copy link
Member Author

Are we confident that this handles all the cases where wasm might transition to the interpreter appropriately?

I am trying to test it with as many apps as I could get a hold on before merging it. TBH, I can't guarantee that this feature is going to work 100% for all wasm apps, that's why it is off by default.

@build-analysis build-analysis bot mentioned this pull request Aug 7, 2023
Open
3 tasks
This was referenced Aug 9, 2023
Closed
Closed
Closed
@vargaz
Copy link
Contributor

vargaz commented Aug 9, 2023

The mono changes look ok to me.

@build-analysis build-analysis bot mentioned this pull request Aug 9, 2023
Closed
lambdageek
lambdageek approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM. Consider changing WASMStripIL to WasmStripIL.

Is it worth calling out that this only makes sense with AOT right in the property name? Maybe WasmStripILAfterAOT ?

src/mono/wasm/build/WasmApp.targets Outdated Show resolved Hide resolved
@fanyang-mono
Copy link
Member Author

Will enable WASM AOT tests with WasmStripILAfterAOT =true in a follow-up PR.

@fanyang-mono fanyang-mono merged commit 867e185 into dotnet:main Aug 10, 2023
165 checks passed
This was referenced Aug 11, 2023
Merged
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radical radical radical left review comments

@vargaz vargaz vargaz left review comments

@BrzVlad BrzVlad BrzVlad left review comments

@kg kg kg approved these changes

@lambdageek lambdageek lambdageek approved these changes

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@kotlarmilos kotlarmilos Awaiting requested review from kotlarmilos kotlarmilos is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@fanyang-mono fanyang-mono

Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

[WASM] Enable new IL trim feature for WASM apps
7 participants
@fanyang-mono @lewing @radical @vargaz @kg @lambdageek @BrzVlad