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

[release/8.0][wasm] Fix regressed file sizes for blazor #92627

Merged
merged 4 commits into from Sep 27, 2023
Merged

[release/8.0][wasm] Fix regressed file sizes for blazor #92627

merged 4 commits into from Sep 27, 2023

Conversation

radical
Copy link
Member

@radical radical commented Sep 26, 2023 •
edited

[wasm] build: Revert to older behavior for WasmNativeStrip

The earlier change was done in 26ae097,
which changed to pass -g to the link step also. But that resulted in
increased native file sizes.

Changed sizes for the minimum blazor template - publish scenario:

--- run from rc2 HEAD (KiB) With the change (KiB) % change
SOD - Minimum Blazor Template - Publish 7918 7705 -2.69
Total Uncompressed _framework 4254 4104 -3.53
Aggregate - .js 540 437 -19.07
Aggregate - .wasm 3711 3663 -1.29
pub/wwwroot/_framework/dotnet.js 35 35 0.00
pub/wwwroot/_framework/dotnet.native.8.0.0-VERSION.js 234 131 -44.02
pub/wwwroot/_framework/dotnet.native.wasm 1170 1122 -4.10
pub/wwwroot/_framework/dotnet.runtime.8.0.0-VERSION.js 216 217 0.46

Customer impact

Improved download size for the blazor app.

Testing

Sizes confirmed with a dotnet-runtime-perf run for blazor_scenarios. Correctness verified by unit tests.

Risk

Low. This is reverting to older behavior to change the symbols being included in the output.

Details

  • [wasm] Add getters for __indirect_function_table, and wasmMemory
  • [wasm] cleanup corresponding tests
  • [wasm] Remove WasmNativeStrip from wasm.proj as it will have no effect

Issue: dotnet/perf-autofiling-issues#20642

@radical
[wasm] Add getters for __indirect_function_table, and wasmMemory
5eefacf 
.. so that can work even if they get renamed during minimization.
@radical
[wasm] build: Revert to older behavior for WasmNativeStrip
ae93e81 
The earlier change was done in 678fd6a,
which changed to pass `-g` to the link step also. But that resulted in
increased native file sizes.

Changed sizes for the `minimum blazor template - publish` scenario:

```
                                                          | Last rc1 run     | With the change
----------------------------------------------------------|----------------- |------------------
SOD - Minimum Blazor Template - Publish                   |8590723.000 bytes |7889806.000 bytes
Total Uncompressed _framework                             |4304274.000 bytes |4202273.000 bytes
pub/wwwroot/_framework/dotnet.js                          |35722.000 bytes   |35838.000 bytes
pub/wwwroot/_framework/dotnet.native.8.0.0-VERSION.js     |239307.000 bytes  |134566.000 bytes
pub/wwwroot/_framework/dotnet.native.wasm                 |1174394.000 bytes |1148841.000 bytes
pub/wwwroot/_framework/dotnet.runtime.8.0.0-VERSION.js    |221356.000 bytes  |221712.000 bytes
```
@radical
[wasm] cleanup corresponding tests
0207d60
@radical
[wasm] Remove WasmNativeStrip from wasm.proj as it will have no effect
91379dd
@radical radical added the arch-wasm WebAssembly architecture label Sep 26, 2023
@ghost ghost assigned radical Sep 26, 2023
@radical radical changed the title wasm symbols 8 [release/8.0][wasm] Fix regressed file sizes for blazor Sep 26, 2023
@ghost
Copy link

ghost commented Sep 26, 2023

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

Issue Details
  • [wasm] Add getters for __indirect_function_table, and wasmMemory
  • [wasm] build: Revert to older behavior for WasmNativeStrip
  • [wasm] cleanup corresponding tests
  • [wasm] Remove WasmNativeStrip from wasm.proj as it will have no effect
Author: radical
Assignees: -
Labels:

arch-wasm
Milestone: -

@radical
Copy link
Member Author

radical commented Sep 26, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@radical radical requested a review from maraf September 26, 2023 07:51
@pavelsavara
Copy link
Member

pavelsavara commented Sep 26, 2023 •
edited

pub/wwwroot/_framework/dotnet.js 35722.000 bytes 35838.000 bytes

This is size change bit suspicious, because your changes should have no way how to influence that.
Most likely it means you are not comparing the same base commit. Do the other size changes have the same problem ?

Edit: oh, your table heading actually said rc1

@radical
Copy link
Member Author

radical commented Sep 26, 2023

pub/wwwroot/_framework/dotnet.js 35722.000 bytes 35838.000 bytes

This is size change bit suspicious, because your changes should have no way how to influence that. Most likely it means you are not comparing the same base commit. Do the other size changes have the same problem ?

Yeah, it's not comparing exactly the same base commit, because we don't have any runs for 8.0 or 8.0-rc2 branches, so I have to compare against the last run that we have from rc1.

Essentially, the difference is that we don't pass -g to link step now.

@radical
Copy link
Member Author

radical commented Sep 26, 2023

I can do a manual run on the current HEAD, and then we could compare.

@radical
Copy link
Member Author

radical commented Sep 26, 2023

@pavelsavara @lewing I have updated the results.

@radical radical added the Servicing-consider Issue for next servicing release review label Sep 26, 2023
@radical
Copy link
Member Author

radical commented Sep 26, 2023

@pavelsavara good catch, I fixed the percentages now.

@radical radical added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 27, 2023
@radical
Copy link
Member Author

radical commented Sep 27, 2023

@carlossanlop this is ready for merge.

@carlossanlop carlossanlop merged commit 0933e30 into dotnet:release/8.0 Sep 27, 2023
36 of 50 checks passed
@radical radical deleted the wasm-symbols-8 branch September 27, 2023 03:37
@radical radical mentioned this pull request Oct 3, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing approved these changes

@pavelsavara pavelsavara pavelsavara approved these changes

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

@maraf maraf Awaiting requested review from maraf

Assignees

@radical radical

Labels
arch-wasm WebAssembly architecture area-Build-mono Servicing-approved Approved for servicing release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@radical @pavelsavara @lewing @carlossanlop