golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.02k stars 17.68k forks source link

proposal: cmd/link: add wasminitmem and wasmmaxmem params #69018

Open gggrafff opened 2 months ago

gggrafff commented 2 months ago

Proposal Details

Hello!

The problem

I am compiling my application to GOOS=wasip1 GOARCH=wasm and I want to control the wasm memory more accurate. wasm-ld provides the following parameters for this:

--initial-memory=<value>    Initial size of the linear memory
--max-memory=<value>        Maximum size of the linear memory

But go tool link doesn't allow me to set these values. And I cannot use the wasm-ld because -linkmode=external requires CGO but CGO doesn't support wasm.

Proposal

I propose to add the initial-memory and max-memory cli-parameters to go tool link

Solving

I think that a solving is not too hard and I wish I could help. I expect that the edit should be to pass a couple of constants from the cli-parameters to the module that is responsible for initial memory and max memory. But I looked into the linker code and didn't find where these values are set. Maybe you can tell me how best to solve this task.

cherrymui commented 2 months ago

If we do this, it would probably be -wasminitmem and -wasmmaxmem. The flag does not apply to other platforms, so it would have wasm in the name to be clear. Also, it can also apply to js/wasm, not just limited to wasip1 (so wasm, not wasi).

One question is whether these settings are better to be per-build or per-program. If it is per-program, i.e. for the same program you'd expect the same setting in most cases, maybe it can be directives in the main package, like //go:wasminitmem NNN. Per-build settings (i.e. linker flags) are more flexible, but they need to be set every time it is built.

While Wasm does allow setting max memory at build time, would it make sense to be a run time setting? (The initial memory size would have to be a build time setting.)

Could you explain more about your use cases? So we can see what is the best option here. Thanks.

cherrymui commented 2 months ago

cc @golang/wasm

gggrafff commented 2 months ago

@cherrymui thank you

The flag does not apply to other platforms, so it would have wasm in the name to be clear

good point

are better to be per-build or per-program

I agree with you that per-build settings (i.e. linker flags) are more flexible, I would be prefer this

would it make sense to be a run time setting?

You are right, max memory can be set in runtime, so this setting in the linker is not so important, but initial memory is necessary

Could you explain more about your use cases?

My users write small functions for data processing, my service compiles it into wasi and executes it. These are often very small functions that only need 2MB of memory, but go link makes initial memory much larger by default, so I get memory overruns dozens of times. I need the functions to start with a minimum amount of memory and increase it only if necessary. I use wasmtime-go to execute these.

johanbrandhorst commented 2 months ago

Is there an opportunity to just adjust the initial memory down? I don't know why it's high by default if we can allocate more as we go.

cherrymui commented 2 months ago

Currently it is set to the size of all global data + 16 MB. From the comment it is enough for the runtime to initialize without growing memory. I suppose we can lower it, though I'm not sure if we can grow memory at very early stage of runtime initialization.

If we lower the default, I think we still need the flag, so users who want the old behavior can change it back.

If the initial memory is set to be smaller than the global data size, it probably won't work. So we probably require it to be bigger than data size.

rsc commented 2 months ago

If we need max maybe the flags should be wasmminmem and wasmmaxmem.

rsc commented 2 months ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

rsc commented 2 months ago

Retitled to be -wasminitmem and -wasmmaxmem. Any other comments?

aclements commented 1 month ago

What's the use case for setting the maximum memory?

What's the downside of the linker just defaulting to a lower initial memory?

One thing I'm a bit confused about: simply initializing the runtime requires some heap allocation, which I thought would force the wasm linear memory to immediately grow to some non-trivial size. Is that not the case?

bvisness commented 1 month ago

Setting a maximum size on memory is a thing you can do to control the memory footprint, especially since engines can use different allocation strategies when they know the memory can't grow beyond a certain point. For example, an engine may be able to reserve the maximum size up front so that memory.grow can be implemented without a realloc.

Also, shared WebAssembly memory from the widely-available threads proposal requires a maximum because the memory may be accessible from multiple threads, and therefore the engine must avoid a realloc on grow.

aclements commented 1 month ago

Thanks for the additional context.

I'm still a bit unconvinced (or perhaps just unclear) about concrete use cases for this. Not needing the engine to grow the memory reservation seems nice, I guess, but doesn't seem very important. For shared, I assume you're referring to this specifically, which does require a maximum but also says growing is still allowed (up to the max). I also didn't think there was any way to set up wasm threads or shared linear memory for Go wasm programs today, though maybe that's not the case.

cherrymui commented 3 weeks ago

What's the downside of the linker just defaulting to a lower initial memory?

Defaulting to a lower initial memory size is probably fine, as long as it can finish runtime initialization without growth. The only downside I can think of is that there may be more growMemory calls compared to before with the higher default. Maybe we can change to a lower default, and provide the flag so users who want the old size can set the flag.

gopherbot commented 3 weeks ago

Change https://go.dev/cl/621636 mentions this issue: cmd/link: reduce Wasm initial memory size

gopherbot commented 3 weeks ago

Change https://go.dev/cl/621635 mentions this issue: runtime: (re)use unused linear memory on Wasm

cherrymui commented 3 weeks ago

While looking into the initial memory size, I found that surprisingly, the runtime currently doesn't use (most of) the initial memory size beyond global data. I sent CL 621635 to fix, which also fixes another case where the runtime didn't reuse memory.

So the current extra initial memory (16 MB) is mostly unused, except due to (possible another bug) that the global data calculation counts only initialized data variables, but not uninitialized bss variables. CL 621636 fixes the global data calculation, and reduces the extra to 1 MB.

The current runtime allocation pattern at startup is that it starts with two allocations, 4 pages and 1 page, for the page allocator's metadata, the an 8 MB reservation for the first heap arena, which is 4 MB size, 4 MB aligned. I chose 1 MB extra space to cover the small allocations, but let the runtime allocate the heap arena, so the linker code and the runtime's allocator are not tightly coupled.

We don't actually need the 8 MB reservation. We just need to grow the memory to be 4 MB aligned, then align a 4 MB. I'll send probably a CL for that. With that, for a small program which has global data < 4 MB and needs heap < 4 MB, it probably only needs 8 MB linear memory in total, with one growth (we could change the linker to set at least 8 MB initial memory if we're okay with more coupling between the runtime and the linker).

Given the changes above, is it still appealing to add the linker flag?

gopherbot commented 3 weeks ago

Change https://go.dev/cl/621715 mentions this issue: runtime: reserve fewer memory for aligned reservation on sbrk systems

gopherbot commented 2 weeks ago

Change https://go.dev/cl/622378 mentions this issue: test: add a test for wasm memory usage

cherrymui commented 6 days ago

Bumping this up: with the recent fixes, we've lowered the default initial memory, and a small program should just use a relatively small amount of memory (8 MB). Given this, is there still a strong need to add the linker flag?