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

[browser] improve default initial memory size #80507

Merged
merged 20 commits into from
Jan 24, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jan 11, 2023

In both normal and re-link with wasm workload

Make default initial memory size:

  • size of loaded DLLs x1.2
  • plus size of AOT DATA segments
  • rounded to 64KB page
  • 16MB if they are smaller

Contributes to #61925
Contributes to #66313
Contributes to #79909
Contributes to #73949

Simplified backport to Net7 is #80849

@pavelsavara pavelsavara added this to the 8.0.0 milestone Jan 11, 2023
@pavelsavara pavelsavara self-assigned this Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

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

Issue Details

In both normal and re-link with wasm workload

Contributes to #61925

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 8.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@pavelsavara pavelsavara requested review from kg and maraf January 11, 2023 18:00
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

AOT
wasm-ld : error : initial memory too small, 20984208 bytes needed
wasm-ld : error : initial memory too small, 22961344 bytes needed

Microsoft.Extensions.Configuration.Functional.Tests Log

System.Runtime.Tests Log

I guess we need calculated value ? But what would be good way how to obtain it ?

@pavelsavara
Copy link
Member Author

But what would be good way how to obtain it ?

Here the wasm-ld would calculate minimal value it needs for stack and global variables, and segment sizes
https://github.com/llvm/llvm-project/blob/540e58af6692bf373d9be4f99d320b4b0f753e3b/lld/wasm/Writer.cpp#L352-L363

But unfortunately emscripten is sending a specific value
https://github.com/emscripten-core/emscripten/blob/8fc4ea5d3e62642664a9bb95c4413bba3d349f71/src/settings.js#L161

Why our AOT adds data segments ?

@pavelsavara
Copy link
Member Author

pavelsavara commented Jan 12, 2023

I suspect that the extra data in the data segment when AOT are coming from mapping metadata_token -> wasm func index.
It's 0.8MB difference in data segment size for src\mono\sample\wasm\browser-advanced sample (hello world).

@vargaz

  • is such size increase of data segment reasonable ?
  • how do I validate that the extra data are indeed the function mapping and not something else ?
  • could we store wasm func index in the IL metadata instead of the token ?
  • what would be good algo to estimate the extra size from AOT in order to bump initial memory size during the build time ?

@lewing
Copy link
Member

lewing commented Jan 14, 2023

#80653 is related

@pavelsavara pavelsavara changed the title [browser] make default initial memory size 16MB [browser] make default initial memory size (size of loaded DLLs)*2 or 16MB if they are smaller Jan 16, 2023
@pavelsavara pavelsavara changed the title [browser] make default initial memory size (size of loaded DLLs)*2 or 16MB if they are smaller [browser] improve default initial memory size Jan 16, 2023
@radekdoulik
Copy link
Member

One possible improvement, which doesn't need to be part of this PR, is to not run the llvm-size and the task when the memory size is already set by the user.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Jan 19, 2023
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

I like the idea of eventually using the managed path in the task but that is an implementation detail that can be prioritized separately

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Looking good. Some comments.

src/mono/wasm/build/WasmApp.Native.targets Outdated Show resolved Hide resolved
src/mono/wasm/build/WasmApp.Native.targets Show resolved Hide resolved
src/mono/wasm/build/WasmApp.Native.targets Outdated Show resolved Hide resolved
src/mono/wasm/build/WasmApp.targets Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmCalculateInitialHeapSize.cs Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmCalculateInitialHeapSize.cs Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmCalculateInitialHeapSize.cs Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmCalculateInitialHeapSize.cs Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 19, 2023
Co-authored-by: Ankit Jain <radical@gmail.com>
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 24, 2023
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@pavelsavara
Copy link
Member Author

CI failures are unrelated

@pavelsavara pavelsavara merged commit 1607ab2 into dotnet:main Jan 24, 2023
@ChristianA1992
Copy link

I tried the .net 7 backport.
I didn't really do much for loading the app. Atleast not on chromium browser which are very slow compared to firefox.
Maybe I'm stilling missing more, for my app to load faster.

@maraf
Copy link
Member

maraf commented Feb 8, 2023

I tried the .net 7 backport.
I didn't really do much for loading the app. Atleast not on chromium browser which are very slow compared to firefox.
Maybe I'm stilling missing more, for my app to load faster.

This change doesn't actually speed up loading the app. It changes the amount of memory that is allocated when the app starts.

carlossanlop pushed a commit that referenced this pull request Feb 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2023
@pavelsavara pavelsavara deleted the browser_initial_heap_size branch September 2, 2024 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants