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

Improve InstantiateModule concurrency performance #602

Closed
F21 opened this issue May 29, 2022 · 5 comments
Closed

Improve InstantiateModule concurrency performance #602

F21 opened this issue May 29, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@F21
Copy link
Contributor

F21 commented May 29, 2022

Is your feature request related to a problem? Please describe.
As previously referenced in #592 and #591, I am calling a WebAssembly module compiled using javy with wazero.

As modules compiled with Javy can only accept input via stdin and produce output via stdout, the current way to interact with these modules is to use a ModuleConfig with a unique name (currently generated using UUID v4) and calling InstantiateModule().

I am current doing this:

config := wazero.NewModuleConfig().WithName(uniqueModuleID).WithStdout(stdOut).WithStdin(input).WithStderr(stdErr)

module, err := runtime.InstantiateModule(ctx, code, config)

This allows safe instantiation of the module from multiple go routines. However, the problem is that calling InstantiateModule mutates the store, which is protected by a mutex and is quite expensive (https://github.com/tetratelabs/wazero/blob/029a79476b4f62741b81bc2f170cc691a86d11cf/internal/wasm/store.go#L334:17). This prevents any parallel execution and forces each call to happen serially.

Describe the solution you'd like
I'd like a more performant way of calling a module's function with stdin and stdout. The ideal solution would be to instantiate the module only once, but able to call it multiple times with a given stdin and stdout.

Describe alternatives you've considered
None at the moment.

Additional context
None at the moment.

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

Before we go down this road, we should make sure what you are doing is something supported in Javy and I expect it to not be.

As far as I understand, javy does not export functions. You currently are using the "_start" command defined by WASI, which is called during InstantiateModule, and doing I/O with stdin/stdout because "_start" is nullary.

It seems like what you are trying to do is work around that javy does not export functions, by using "_start" as a function call, using stdin/stdout as alternatives to parameters and results. You expect to WithStartFunctions(nil) to disable auto-start and then call "_start" in parallel overriding stdin/stout each time via context or something.

My concern isn't just that this is a hack around lack of function exports, but that this is not necessarily safe either. _start may be setting up state that would prevent it from being able to be safely called in parallel.

Questions:

  1. Have you asked javy to support function exports? That's so very much cleaner than doing this.
  2. If not, it seems safer to make an option to manually export a function than do something like this

More importantly, would you mind exploring on https://github.com/F21/javy-wazero instead of using the issues list here to figure out workarounds? I'm watching that repo, so you can open issues like this there and if I don't respond you can try pinging other repos like Javy or wazero for your questions about javy.

cc also @saulecabrera

@codefromthecrypt
Copy link
Contributor

ps regardless I'll work on the perf thing. We may also end up needing to do scope overrides for all things, not just filesystem (see the experimental package). just I started to decompile (via wasm2wat) what javy creates and it is a large module (>1k functions for a simple code) and would have instantiation cost. there's going to be some value thinking about the root issue which is cheaply invoking functions similar to how other wasm compilers work (via exports)

@F21
Copy link
Contributor Author

F21 commented May 30, 2022

I just want to quickly update. I switched to using https://github.com/suborbital/javy which is a fork that has support for deallocate and allocate. Using this in conjunction with call() reduced exported function execution times by half.

Also, further testing revealed that functions on modules cannot be called concurrently (at least the ones produced by javy). Attempting to call allocate concurrently produced wasm error: out of bounds memory access and calling other exported functions produced that uses the memory addresses produced wasm error: invalid table access and wasm error: out of bounds memory access errors.

Therefore, the original reasons for this feature request are no longer valid. However, I would still love to see performance improvements to InstantiateModule(). As we cannot call exported functions from wasms concurrently, my idea is to have multiple modules of the code instantiated in a given runtime. These would be "workers" that I can scale up and scale down as needed. As InstantiateModule() is cheaper to call than creating a new runtime and compiling the code then instantiating it, I think this is a worthwhile strategy.

I will update https://github.com/F21/javy-wazero over the next few days with more findings and code so explore this in more detail.

@codefromthecrypt
Copy link
Contributor

excellent update. thanks!

@mathetake
Copy link
Member

I don’t believe this is an issue now given the recent improvements, closing for now.

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

No branches or pull requests

3 participants