-
Notifications
You must be signed in to change notification settings - Fork 5k
[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
Comments
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsWe 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 It is entirely possible something was missed to prune out the transaction code for WASM. Is there something that was missed? /cc @roji |
Not sure yet. It looks like something pulls in
|
I think c55d76d#diff-197d1d73f09b5e9ce1e832442f7dccd63df0be0b7b8a47f6c682099ba84e5452R8 is the issue here. If I understand it correctly, @roji why do we need |
@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 |
You can do that for Windows only. It would fix the wasm regression. |
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)
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. |
Yep, makes sense. |
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)
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)
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]>
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.
The text was updated successfully, but these errors were encountered: