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

Add emscripten_notify_memory_growth import? #601

Closed
inkeliz opened this issue May 29, 2022 · 16 comments · Fixed by #678
Closed

Add emscripten_notify_memory_growth import? #601

inkeliz opened this issue May 29, 2022 · 16 comments · Fixed by #678
Labels
enhancement New feature or request

Comments

@inkeliz
Copy link
Contributor

inkeliz commented May 29, 2022

Is your feature request related to a problem? Please describe.
Compiling using emscripten requires emscripten_notify_memory_growth, defined at https://emscripten.org/docs/api_reference/emscripten.h.html#abi-functions.

This is one single function and that is useless for Wazero. That is require for malloc.

Minimal code:

# include "stdint.h"
# include "stdlib.h"
# include "stdio.h"

uint8_t *InputMemory;
uint8_t *OutputMemory;

int main() {
    OutputMemory = (uint8_t *) malloc(8000000);
    InputMemory = (uint8_t *) malloc(8000000);

    if (InputMemory == NULL || OutputMemory == NULL) {
        printf("Erro");
        return 1;
    } else {
        printf("Okay");
        return 0;
    }
}

Compile with:

emcc wasm.c -o wasm.wasm -O3 -s ALLOW_MEMORY_GROWTH

Requires emscripten_notify_memory_growth.

Describe the solution you'd like
Maybe Wazero can define such import, so any emscripten compiled wasm will work without declaring additional modules.

Describe alternatives you've considered
It's possible to fix by using:

    env, _ := host.NewModuleBuilder("env").
        ExportFunction("emscripten_notify_memory_growth", func(_ int32) {}).
        Instantiate(ctx)

That is very small footprint, but very annoying to declare it everytime.

Additional context
Maybe we can do something similar of AssemblyScript and create one new Emscripten package? However, it's very small, maybe can be declared on WASI? But, it's not WASI anyway. :\

@inkeliz inkeliz added the enhancement New feature or request label May 29, 2022
@codefromthecrypt
Copy link
Contributor

funny there's only one, and I guess it is multi-memory ready as the index is the memory index right? (always 0 at the moment)

Adding something for emscripten sounds fine as it can consolidate the docs and stop people from having to type out what you added, solely to ignore the callback. Of course, even in Go, you may want to pay attention to it, if there's an unsafe view of memory.

We can limit this to just the module part (ex similar to the /assemblyscript directory/package) and not dig into an example, yet. The default could be no-op and the builder could allow overriding via a func(context.Context, api.Module, uint32) as I expect if some callback were needed, you'd need to grab the memory. Taking care to add godoc comment of that emscripten link as it is a handy reference.

Are you keen to give this a go?

@inkeliz
Copy link
Contributor Author

inkeliz commented May 29, 2022

Of course, even in Go, you may want to pay attention to it, if there's an unsafe view of memory.

I think Wazero could provide some channel to send events when grow happens. That will work for all programs, not just compiled with Emscripten. Basically, when calling grow, Wazero could do: notify <- struct{}, and the Memory() have one option for Memory().OnGrow(). (I'm simplifying to give the idea).

@codefromthecrypt
Copy link
Contributor

indeed an event listener was on topic a month or two ago. and yeah this emscripten function would exist regardless of that. so many neat things left on the plate :D

@codefromthecrypt
Copy link
Contributor

ps we are doing some shifting around again, so I'll take this on to reduce drift (adding a small module for emscripten)

@codefromthecrypt
Copy link
Contributor

Aside as it doesn't impact doing this, but it is related...

You know this, but maybe others watching don't, that go also has this same notification API https://github.com/golang/go/blob/9839668b5619f45e293dd40339bf0ac614ea6bee/src/runtime/mem_js.go#L82

@codefromthecrypt
Copy link
Contributor

@inkeliz so re-parsing your comment having seen the pattern repeat in go. Here's my takeaway:

  • some compilers write down instructions to invoke a host-defined memory grow callback.
  • if we had a generic memory grow callback, all custom ones could be ignored with docs saying "use the generic one".

Am I getting this right?

@inkeliz
Copy link
Contributor Author

inkeliz commented Jun 4, 2022

Yes.

I think makes more sense to ignore "compiler-specific callbacks" (exports these functions as "no-op"). Expose a generic channel/callback (which will work regardless of the compiler/language): that should trigger when that Grow is called (and the buffer reallocates).

@codefromthecrypt
Copy link
Contributor

sounds good.

nuance to document:

  • if max=cap the buffer won't grow, but since the length changed it is likely watchers need to update their view.
  • memory is shared in wasm, so slow callbacks can affect multiple instantiated modules, unless they are offloaded to a goroutine.

any other thoughts?

@inkeliz
Copy link
Contributor Author

inkeliz commented Jun 6, 2022

if max=cap the buffer won't grow, but since the length changed it is likely watchers need to update their view.

Yes, that makes sense.
EDIT: If the callback is only interested to detect reallocation, it can check if it reallocates or not by checking the first memory[0] pointer, and then skip if it's the same.

memory is shared in wasm, so slow callbacks can affect multiple instantiated modules, unless they are offloaded to a goroutine.

I'm not sure if I understand "can affect multiple instantiated modules". But, I think that is quite expected, maybe just add it as an documentation.

@codefromthecrypt
Copy link
Contributor

ps I've verified this builds and ends up with the following imports

  (import "wasi_snapshot_preview1" "proc_exit" (func (;0;) (type 2)))
  (import "wasi_snapshot_preview1" "fd_write" (func (;1;) (type 7)))
  (import "env" "emscripten_notify_memory_growth" (func (;2;) (type 2)))

@codefromthecrypt
Copy link
Contributor

@inkeliz I'm probably doing something stupid, but regardless of if the memory growth param is set, when I compile the above example, it invariably results in unreachable.

ex without optimization

2022/07/05 17:45:41 module[] function[_start] failed: wasm error: unreachable
wasm stack trace:
	.[10](i32)
	.[9](i32)
	.[6]()

Were you able to successfully compile and run a main function using emcc like above?

@codefromthecrypt
Copy link
Contributor

I added the --profiling arg and looks like this is during exit.

2022/07/05 17:55:19 module[] function[_start] failed: wasm error: unreachable
wasm stack trace:
	._Exit(i32)
	.exit(i32)
	._start()

I'll look into this tomorrow unless someone has run into this before and knows what's up.

@codefromthecrypt
Copy link
Contributor

oh wow. looks like emscripten wants to not be able to return from exit!

emscripten-core/emscripten#12322 (comment)

This is a separate issue we'll need to decide to panic by default or via config cc @anuraaga @mathetake

codefromthecrypt pushed a commit that referenced this issue Jul 5, 2022
This changes the AssemblyScript abort handler and WASI proc_exit
implementation to panic the caller which eventually invoked close.

This ensures no code executes afterwards, For example, LLVM inserts
unreachable instructions after calls to exit.

See emscripten-core/emscripten#12322
See #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor

After this is in, emscripten should work naturally #673

Then, we can add a host module including a fake memory hook

@inkeliz
Copy link
Contributor Author

inkeliz commented Jul 5, 2022

oh wow. looks like emscripten wants to not be able to return from exit!

I don't remember to have this issue before, but I didn't test it recently. :S

codefromthecrypt pushed a commit that referenced this issue Jul 6, 2022
This changes the AssemblyScript abort handler and WASI proc_exit
implementation to panic the caller which eventually invoked close.

This ensures no code executes afterwards, For example, LLVM inserts
unreachable instructions after calls to exit.

See emscripten-core/emscripten#12322
See #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this issue Jul 6, 2022
This changes the AssemblyScript abort handler and WASI proc_exit
implementation to panic the caller which eventually invoked close.

This ensures no code executes afterwards, For example, LLVM inserts
unreachable instructions after calls to exit.

See emscripten-core/emscripten#12322
See #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this issue Jul 6, 2022
This changes the AssemblyScript abort handler and WASI proc_exit
implementation to panic the caller which eventually invoked close.

This ensures no code executes afterwards, For example, LLVM inserts
unreachable instructions after calls to exit.

See emscripten-core/emscripten#12322
See #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this issue Jul 17, 2022
This adds a toehold integration for emscripten users, so they don't
have to define the memory growth function.

Fixes #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this issue Jul 17, 2022
This adds a toehold integration for emscripten users, so they don't
have to define the memory growth function.

Fixes #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor

sorry took so long @inkeliz #678

codefromthecrypt added a commit that referenced this issue Jul 17, 2022
This adds a toehold integration for emscripten users, so they don't
have to define the memory growth function.

Fixes #601

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants