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

[wasm] Fix regressed file sizes for blazor #92664

Merged
merged 28 commits into from
Dec 6, 2023

Conversation

radical
Copy link
Member

@radical radical commented Sep 26, 2023

[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.

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

Issue: Issue: dotnet/perf-autofiling-issues#20642

.. so that can work even if it gets renamed during minimization.
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
```

(cherry picked from commit ae93e81)
(cherry picked from commit 0207d60)
@radical radical added the arch-wasm WebAssembly architecture label Sep 26, 2023
@ghost ghost assigned radical 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 getter for __indirect_function_table
  • [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 radical changed the title wasm native strip main [wasm] Fix regressed file sizes for blazor Sep 26, 2023
@radical
Copy link
Member Author

radical commented Sep 26, 2023

/azp run runtime-wasm-perf

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Sep 27, 2023

After the change to move getWasmIndirectFunctionTable to only be before the runtime is created, library all the tests are failing with:

fail: [out of order message from the browser]: http://127.0.0.1:36071/_framework/dotnet.runtime.js 2:12344 "MONO_WASM: onRuntimeInitializedAsync() failed" TypeError: t.getWasmIndirectFunctionTable is not a function
          at br (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:55253)
          at zr (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:60544)
          at http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:214955
fail: [out of order message from the browser]: http://127.0.0.1:36071/_framework/dotnet.js 2:917 "MONO_WASM: TypeError: t.getWasmIndirectFunctionTable is not a function\n    at br (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:55253)\n    at zr (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:60544)\n    at http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:214955"

log.

@kg
Copy link
Contributor

kg commented Sep 27, 2023

After the change to move getWasmIndirectFunctionTable to only be before the runtime is created, library all the tests are failing with:

fail: [out of order message from the browser]: http://127.0.0.1:36071/_framework/dotnet.runtime.js 2:12344 "MONO_WASM: onRuntimeInitializedAsync() failed" TypeError: t.getWasmIndirectFunctionTable is not a function
          at br (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:55253)
          at zr (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:60544)
          at http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:214955
fail: [out of order message from the browser]: http://127.0.0.1:36071/_framework/dotnet.js 2:917 "MONO_WASM: TypeError: t.getWasmIndirectFunctionTable is not a function\n    at br (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:55253)\n    at zr (http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:60544)\n    at http://127.0.0.1:36071/_framework/dotnet.runtime.js:3:214955"

log.

I guess I put it in both places because it was actually necessary, and I forgot :-)

@pavelsavara
Copy link
Member

need help ?

@radical
Copy link
Member Author

radical commented Nov 29, 2023

need help ?

Any suggestions on a fix? The alternative would be to disable the failing tests which are essentially for Debug+AOT, and Debug+relink. We could ignore debug+aot for now, but debug+relink is likely more common because we would relink if you had any native references too.

But this PR will help the sizes.

@pavelsavara
Copy link
Member

Any suggestions on a fix?

Still this, right ?

Wasm.Build.Tests.Blazor.SimpleRunTests.BlazorPublishRunTest(config: "Debug", aot: True) [FAIL]

 [error] MONO_WASM: onRuntimeInitializedAsync() failed Error: raw cwrap mono_jiterp_placeholder_trace not found
            at Ir (http://127.0.0.1:35275/_framework/dotnet.runtime.9.0.0-ci.uxcbg0k114.js:3:58604)
            at http://127.0.0.1:35275/_framework/dotnet.runtime.9.0.0-ci.uxcbg0k114.js:3:214838
            at http://127.0.0.1:35275/_framework/dotnet.runtime.9.0.0-ci.uxcbg0k114.js:3:215122

@radical
Copy link
Member Author

radical commented Nov 30, 2023

Any suggestions on a fix?

Still this, right ?


Wasm.Build.Tests.Blazor.SimpleRunTests.BlazorPublishRunTest(config: "Debug", aot: True) [FAIL]



 [error] MONO_WASM: onRuntimeInitializedAsync() failed Error: raw cwrap mono_jiterp_placeholder_trace not found

            at Ir (http://127.0.0.1:35275/_framework/dotnet.runtime.9.0.0-ci.uxcbg0k114.js:3:58604)

            at http://127.0.0.1:35275/_framework/dotnet.runtime.9.0.0-ci.uxcbg0k114.js:3:214838

            at http://127.0.0.1:35275/_framework/dotnet.runtime.9.0.0-ci.uxcbg0k114.js:3:215122

Yes

@pavelsavara
Copy link
Member

I have this untested theory:

I think this is not about AOT, but about WasmBuildNative being triggered with some optimization level which mangles symbols.

It fails in getRawCwrap which uses un-mangled symbol names.
mono_jiterp_placeholder_trace is just coincidence, because it's the first thing requested.

We have this un-mangling logic in createWasmImportStubsFrom and replace_linker_placeholders for wasm imports. Maybe we need same thing for wasm exports ?

@pavelsavara
Copy link
Member

Printing full list of Module["asm"] would help to debug it.

@radical
Copy link
Member Author

radical commented Nov 30, 2023

Relink/aot with an optimization level and -g . This happens only for debug config.

Dunno about the details that you mentioned.

@radical
Copy link
Member Author

radical commented Dec 6, 2023

#95613 seems to have fixed the failures! Thanks @kg, @pavelsavara!

@radical
Copy link
Member Author

radical commented Dec 6, 2023

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Dec 6, 2023

@microsoft-github-policy-service rerun

@radical
Copy link
Member Author

radical commented Dec 6, 2023

Failures are known issues.

@radical
Copy link
Member Author

radical commented Dec 6, 2023

@microsoft-github-policy-service rerun

@radical
Copy link
Member Author

radical commented Dec 6, 2023

@dotnet-github-policy-service rerun

@radical
Copy link
Member Author

radical commented Dec 6, 2023

@dotnet-policy-service rerun

@radical radical merged commit ec31705 into dotnet:main Dec 6, 2023
45 of 49 checks passed
@radical radical deleted the wasm-native-strip-main branch December 6, 2023 10:37
@radekdoulik
Copy link
Member

radekdoulik commented Dec 7, 2023

great work
image

radical added a commit to radical/runtime that referenced this pull request Dec 14, 2023
Blazor size regression was fixed by:

```
commit ec31705
Author: Ankit Jain <radical@gmail.com>
Date:   Wed Dec 6 05:36:59 2023 -0500

    [wasm] Fix regressed file sizes for blazor (dotnet#92664)
```

.. but a subsequent PR created close to that undid some of the changes:

```
commit a128c15
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Dec 11 15:45:58 2023 -0500

    [wasm/wasi] Consolidate build targets (dotnet#95775)
```

Essentially, `-g` was being passed to the link, and compile-bc steps.

Found in dotnet/perf-autofiling-issues#25891 .
radical added a commit that referenced this pull request Dec 14, 2023
* [wasm/wasi] Fix size regression

Blazor size regression was fixed by:

```
commit ec31705
Author: Ankit Jain <radical@gmail.com>
Date:   Wed Dec 6 05:36:59 2023 -0500

    [wasm] Fix regressed file sizes for blazor (#92664)
```

.. but a subsequent PR created close to that undid some of the changes:

```
commit a128c15
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Dec 11 15:45:58 2023 -0500

    [wasm/wasi] Consolidate build targets (#95775)
```

Essentially, `-g` was being passed to the link, and compile-bc steps.

Found in dotnet/perf-autofiling-issues#25891 .

* [wasm] Add flag missed in the consolidate PR
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants