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] Set -sMAXIMUM_MEMORY to 256 MiB by default #84638

Closed
lambdageek opened this issue Apr 11, 2023 · 13 comments
Closed

[wasm] Set -sMAXIMUM_MEMORY to 256 MiB by default #84638

lambdageek opened this issue Apr 11, 2023 · 13 comments
Assignees
Labels
arch-wasm WebAssembly architecture area-Build-mono
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Apr 11, 2023

Mobile Safari (on ios 16.4 and probably earlier) doesn't like Emscripten's default MAXIMUM_MEMORY of 2 GiB:

wasmMemory=new WebAssembly.Memory({"initial":INITIAL_MEMORY/65536,"maximum":2147483648/65536,"shared":true})

We get a rejected promise during startup with RangeError: out of memory with the default maximum memory size.

As a workaround I can add this in my .csproj, but we should set a good default:

        <PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'">
            <!-- 256 MiB -->
            <EmccFlags>-sMAXIMUM_MEMORY=268435456</EmccFlags>
        </PropertyGroup>

Note: we should double-check that 256 is a good default for Blazor

Note2: demo https://lambdageek.dev/dotnet-wasm-raytracer/net8/

Screenshot 2023-04-11 at 13 01 05

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 11, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 11, 2023
@lewing lewing added this to the 8.0.0 milestone Apr 11, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 11, 2023
@lewing
Copy link
Member

lewing commented Apr 11, 2023

Interesting, we definitely need to think about what a good default for this is.

@lambdageek lambdageek added arch-wasm WebAssembly architecture area-Build-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 2023

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

Issue Details

Mobile Safari (on ios 16.4 and probably earlier) doesn't like

wasmMemory=new WebAssembly.Memory({"initial":INITIAL_MEMORY/65536,"maximum":2147483648/65536,"shared":true})

That emscripten has by default, where 2147483648 is the default MAXIMUM_MEMORY.

We get a rejected promise during startup with RangeError: out of memory with the default maximum memory size.

As a workaround I can add this in my .csproj, but we should set a good default:

        <PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'">
            <!-- 256 MiB -->
            <EmccFlags>-sMAXIMUM_MEMORY=268435456</EmccFlags>
        </PropertyGroup>

Note: we should double-check that 256 is a good default for Blazor

Note2: demo https://lambdageek.dev/dotnet-wasm-raytracer/net8/

Screenshot 2023-04-11 at 13 01 05
Author: lambdageek
Assignees: maraf
Labels:

arch-wasm, area-Build-mono, needs-area-label

Milestone: 8.0.0

@maraf
Copy link
Member

maraf commented Jul 26, 2023

I think it's too late to change defaults and would prefer to start with in early .NET 9

@maraf maraf modified the milestones: 8.0.0, 9.0.0 Jul 26, 2023
@pavelsavara
Copy link
Member

I can't find new WebAssembly.Memory statement in the dotnet.native.js anymore.

I can see Memory section in wa-info

Reading section:    Memory size:            7 count: 1
  memory: 0 limits: 256, 32768 has max: True

@lambdageek
Copy link
Member Author

@maraf Can we think about adding an msbuild property for .net8 to set the maximum in a nice way instead of relying on EmccFlags. That way we don't change the default, but people who are interested in supporting iOS can set a maximum that it can use.

@maraf
Copy link
Member

maraf commented Aug 28, 2023

I can't find new WebAssembly.Memory statement in the dotnet.native.js anymore.

The new WebAssembly.Memory statement is there only in MT version. when wasm memory is marked as import. Anyway the passed max memory is used in getHeapMax

@pavelsavara
Copy link
Member

Are we saying that setting max heap lower will make GC run earlier, and the browser would not have to allocate more than he can (on limited device) ?

Or we actually need to set initial memory ?

@pavelsavara
Copy link
Member

we could also detect that we are on mobile safari and try to do it on runtime

const memory = new WebAssembly.Memory({
  initial: 10,
  maximum: 100,
});

@pavelsavara
Copy link
Member

Also if this is only about Mono GC pressure I think we could have other means how to set it lower, without needing to make it compile time constant.

@radekdoulik I remember that you somehow detect available memory for GC purpose ?

@lambdageek
Copy link
Member Author

I'm not sure what this has to do with Mono. Mobile Safari refuses to create the wasm memory instance if the maximum is more than 256MiB. Doesn't matter how Mono will use it. This happens much earlier

@ilonatommy
Copy link
Member

Wasn't #91256 a fix for this? Do we need to keep the discussion open?

@lambdageek
Copy link
Member Author

Wasn't #91256 a fix for this? Do we need to keep the discussion open?

#91256 made it easier for app authors to set the maximum initial memory. But we didn't change the default.

Maybe we don't need to change the default? Maybe we can just add something to the docs? or write something to the console if we detect mobile safari and the runtime fails to start?

@mkhamoyan
Copy link
Member

Error message was added by #104963

@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 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

No branches or pull requests

6 participants