Skip to content

[wasm] Size regression #74506

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

Closed
radekdoulik opened this issue Aug 24, 2022 · 8 comments · Fixed by #74828
Closed

[wasm] Size regression #74506

radekdoulik opened this issue Aug 24, 2022 · 8 comments · Fixed by #74828
Labels
Milestone

Comments

@radekdoulik
Copy link
Member

We are now tracing browser-bench sample AppBundle size in daily measurements https://github.com/radekdoulik/WasmPerformanceMeasurements/blob/main/measurements/index.zip and there was quite large size regression around 8/12/2022. The AppBundle size increased by cca 1.1MB.

By bisecting the commits I got to the commit, which causes that, c55d76d from PR #72051.

I am not yet sure, what in that commit caused the regression.

@radekdoulik radekdoulik added this to the 8.0.0 milestone Aug 24, 2022
@ghost
Copy link

ghost commented Aug 24, 2022

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

Issue Details

We are now tracing browser-bench sample AppBundle size in daily measurements https://github.com/radekdoulik/WasmPerformanceMeasurements/blob/main/measurements/index.zip and there was quite large size regression around 8/12/2022. The AppBundle size increased by cca 1.1MB.

By bisecting the commits I got to the commit, which causes that, c55d76d from PR #72051.

I am not yet sure, what in that commit caused the regression.

Author: radekdoulik
Assignees: -
Labels:

arch-wasm, area-System.Transactions

Milestone: 8.0.0

@AaronRobinsonMSFT
Copy link
Member

@radekdoulik It is entirely possible something was missed to prune out the transaction code for WASM. Is there something that was missed?

/cc @roji

@radekdoulik
Copy link
Member Author

Not sure yet. It looks like something pulls in System.Transactions.Local and few other assemblies.

ls .\before\managed\

    Directory: C:\Users\rodo\git\size-regression\before\managed

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---           8/30/2022  4:02 PM          16896 System.Collections.Concurrent.dll
-a---           8/30/2022  4:02 PM           8704 System.Collections.dll
-a---           8/30/2022  4:02 PM          14848 System.Console.dll
-a---           8/30/2022  4:02 PM          12288 System.Linq.dll
-a---           8/30/2022  4:02 PM          14336 System.Memory.dll
-a---           8/30/2022  4:02 PM           6144 System.Net.Http.dll
-a---           8/30/2022  4:02 PM          22528 System.Net.WebSockets.Client.dll
-a---           8/30/2022  4:02 PM           9216 System.Net.WebSockets.dll
-a---           8/30/2022  4:02 PM        1088512 System.Private.CoreLib.dll
-a---           8/30/2022  4:02 PM          57856 System.Private.Uri.dll
-a---           8/30/2022  4:02 PM           7168 System.Runtime.dll
-a---           8/30/2022  4:02 PM          33792 System.Runtime.InteropServices.JavaScript.dll
-a---           8/30/2022  4:02 PM           5120 System.Runtime.Intrinsics.dll
-a---           8/30/2022  4:02 PM          30208 System.Text.Encodings.Web.dll
-a---           8/30/2022  4:02 PM         307712 System.Text.Json.dll
-a---           8/30/2022  4:02 PM         125952 System.Text.RegularExpressions.dll
-a---           8/30/2022  4:02 PM          83968 Wasm.Browser.Bench.Sample.dll

PS C:\Users\rodo\git\size-regression> ls .\after\managed\

    Directory: C:\Users\rodo\git\size-regression\after\managed

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---           8/30/2022  4:07 PM          16896 System.Collections.Concurrent.dll
-a---           8/30/2022  4:07 PM           8704 System.Collections.dll
-a---           8/30/2022  4:07 PM          15360 System.Console.dll
-a---           8/30/2022  4:07 PM           5120 System.Diagnostics.Tracing.dll
-a---           8/30/2022  4:07 PM          12288 System.Linq.dll
-a---           8/30/2022  4:07 PM          14336 System.Memory.dll
-a---           8/30/2022  4:07 PM           6144 System.Net.Http.dll
-a---           8/30/2022  4:07 PM          22528 System.Net.WebSockets.Client.dll
-a---           8/30/2022  4:07 PM           9728 System.Net.WebSockets.dll
-a---           8/30/2022  4:07 PM        1183744 System.Private.CoreLib.dll
-a---           8/30/2022  4:07 PM          58368 System.Private.Uri.dll
-a---           8/30/2022  4:07 PM           8704 System.Runtime.dll
-a---           8/30/2022  4:07 PM           5632 System.Runtime.InteropServices.dll
-a---           8/30/2022  4:07 PM          33792 System.Runtime.InteropServices.JavaScript.dll
-a---           8/30/2022  4:07 PM           5120 System.Runtime.Intrinsics.dll
-a---           8/30/2022  4:07 PM          30208 System.Text.Encodings.Web.dll
-a---           8/30/2022  4:07 PM         308224 System.Text.Json.dll
-a---           8/30/2022  4:07 PM         126464 System.Text.RegularExpressions.dll
-a---           8/30/2022  4:07 PM           5632 System.Threading.dll
-a---           8/30/2022  4:07 PM           5120 System.Threading.Thread.dll
-a---           8/30/2022  4:07 PM           5120 System.Threading.ThreadPool.dll
-a---           8/30/2022  4:07 PM         163840 System.Transactions.Local.dll
-a---           8/30/2022  4:07 PM          83968 Wasm.Browser.Bench.Sample.dll

@radekdoulik
Copy link
Member Author

radekdoulik commented Aug 30, 2022

I think c55d76d#diff-197d1d73f09b5e9ce1e832442f7dccd63df0be0b7b8a47f6c682099ba84e5452R8 is the issue here.

If I understand it correctly, System.Transactions.Local is part of NetCoreAppLibrary property in src/libraries/NetCoreAppLibrary.props and it has IsTrimmable set to false now. Thus it survives trimming and brings itself and dependencies into every netcore app.

@roji why do we need IsTrimmable be set to false for System.Transactions.Local?

@roji
Copy link
Member

roji commented Aug 30, 2022

@radekdoulik with the porting of Windows-only distributed transaction support into System.Transactions.Local (#72051), this library now does COM interop, which is currently incompatible with trimming; so it was recommended to set IsTrimmable to false (see conversation here). It seems there's a desire to have a source generator for COM (in .NET 8.0), at which point we can mark the assembly as trimmable again...

/cc @ajcvickers

@jkotas
Copy link
Member

jkotas commented Aug 30, 2022

why do we need IsTrimmable be set to false for System.Transactions.Local?

You can do that for Windows only. It would fix the wasm regression.

radekdoulik added a commit to radekdoulik/runtime that referenced this issue Aug 30, 2022
On other target platforms than windows.

This fixes dotnet#74506

The reason to make it non-trimmable is that it uses COM interop
on windows. So we can make it trimmable again on other target platforms.
dotnet#74506 (comment)
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 30, 2022
@radekdoulik
Copy link
Member Author

Thanks for clarifying it. That should apply only for windows then, right? If so, we can make the property conditional to improve linking on other target platforms, like WebAssembly and mobile platforms. I opened #74828 for that.

@roji
Copy link
Member

roji commented Aug 30, 2022

Yep, makes sense.

radekdoulik added a commit that referenced this issue Aug 31, 2022
On other target platforms than windows.

This fixes #74506

The reason to make it non-trimmable is that it uses COM interop
on windows. So we can make it trimmable again on other target platforms.
#74506 (comment)
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 31, 2022
github-actions bot pushed a commit that referenced this issue Sep 2, 2022
On other target platforms than windows.

This fixes #74506

The reason to make it non-trimmable is that it uses COM interop
on windows. So we can make it trimmable again on other target platforms.
#74506 (comment)
carlossanlop pushed a commit that referenced this issue Sep 2, 2022
On other target platforms than windows.

This fixes #74506

The reason to make it non-trimmable is that it uses COM interop
on windows. So we can make it trimmable again on other target platforms.
#74506 (comment)

Co-authored-by: Radek Doulik <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants