feat(plugins): Use wazero instead of wasmtime #3042
Conversation
anuraaga
left a comment
anuraaga
left a comment
There was a problem hiding this comment.
I was thinking of sending this, great to see it :D
internal/ext/wasm/wasm.go
Outdated
| wasiConfig.SetStdinFile(stdinPath) | ||
| wasiConfig.SetStdoutFile(stdoutPath) | ||
| wasiConfig.SetStderrFile(stderrPath) | ||
| // TODO: Handle error |
There was a problem hiding this comment.
There is Instantiate if you'd like to return the error. Though I think any failure here would be a programming bug, not non-determinstic
internal/ext/wasm/wasm.go
Outdated
| keys = append(keys, key) | ||
| vals = append(vals, os.Getenv(key)) | ||
| // Compile the Wasm binary once so that we can skip the entire compilation time during instantiation. | ||
| mod, err := rt.CompileModule(ctx, wasmBytes) |
There was a problem hiding this comment.
If it's possible, it would be nice to rejigger to scope this to Runner, possibly with some map[/* wasm url */ string]wazero.CompiledModule. The compilation cache is good to reuse across executions of the sqlc process itself, but it's also good to only compile once per wasm within a process if possible, the cache key computation isn't trivial. Though if the latter doesn't happen that much maybe it doesn't matter
internal/ext/wasm/wasm.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("define wasi: %w", err) | ||
| conf := wazero.NewModuleConfig() | ||
| conf = conf.WithArgs("plugin.wasm", method) |
There was a problem hiding this comment.
Nit, consider chaining, it's arguably idiomatic for wazero users
conf := wazero.NewModuleConfig().
WithArgs().
WithStdin().
WithStdout().
Co-authored-by: Anuraag Agrawal <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
|
Oops sorry looks like need to update the type argument to |
|
Hi @kyleconroy - sorry if this steps on any toes and feel free to ignore it, being interested in this PR I was hoping to help continue it. I sent #3082 as a PR to this branch with the suggestions. Thanks. |
* feat(plugins): Use wazero instead of wasmtime * Remove wasm build tags * Update internal/ext/wasm/wasm.go Co-authored-by: Anuraag Agrawal <[email protected]> * Update internal/ext/wasm/wasm.go Co-authored-by: Anuraag Agrawal <[email protected]> * Fix build * Suggestions for PR sqlc-dev#3042 (sqlc-dev#3082) * Only compile wasm once per process * Remove unused * Store runtime in flightgroup as well * Handle error from instantiate --------- Co-authored-by: Anuraag Agrawal <[email protected]>
After merging #3027 and #3040, wasmtime is our only cgo dependency left. This change replaces with wazero, a pure-Go WASM runtime. I've played around with it locally and everything appears to work, but I'm going to need to understand how the caching works before this is merged.