[do not review] chore: add WasmRegistry and WasmModule #3044
Conversation
src/server/wasm/wasm_registry.cc
Outdated
| // The following line is problematic, it causes a SEGFAULT when we switch fibers | ||
| // after the command gets executed | ||
|
|
||
| // auto error = wasmtime_module_new(engine_, (uint8_t*)wasm.data, wasm.size, &module); |
There was a problem hiding this comment.
if we comment this out we crash when we switch fibers @dranikpg
There was a problem hiding this comment.
how can we switch fibers if there are no preemption points? The mutex below? Let's make everything thread-local or use a spinlock
There was a problem hiding this comment.
I am not sure, I had also commented out the locks all together. I will play around and ping
src/server/wasm/wasm_registry.cc
Outdated
| wasi_config_ = wasi_config_new(); | ||
| CHECK(wasi_config_); | ||
| wasi_config_inherit_argv(wasi_config_); | ||
| wasi_config_inherit_env(wasi_config_); | ||
| wasi_config_inherit_stdin(wasi_config_); | ||
| wasi_config_inherit_stdout(wasi_config_); | ||
| wasi_config_inherit_stderr(wasi_config_); |
There was a problem hiding this comment.
is there a way to start it without wasi? We don't need it, it should be a pure sandbox
There was a problem hiding this comment.
wasi is the portable abstraction of a system interface (for example reading from a socket read). Some of these are implemented while others have limited functionality. Either way, I think it's fine to have those?
src/server/wasm/wasm_registry.cc
Outdated
| // The following line is problematic, it causes a SEGFAULT when we switch fibers | ||
| // after the command gets executed | ||
|
|
||
| // auto error = wasmtime_module_new(engine_, (uint8_t*)wasm.data, wasm.size, &module); |
There was a problem hiding this comment.
how can we switch fibers if there are no preemption points? The mutex below? Let's make everything thread-local or use a spinlock
| // Very light-weight. Each Module is compiled *once* but each UDF call, e,g, `CALLWASM` | ||
| // will spawn its own instantiation of the wasm module. This is fine, because the former | ||
| // is the expensive operation while the later is used as the context upon the function | ||
| // will execute (effectively allowing concurrent calls to the same wasm module) | ||
| class WasmModuleInstance { |
There was a problem hiding this comment.
Better leave a comment what the difference between engine/store/module are 🙂
| @@ -0,0 +1,3268 @@ | |||
| /** | |||
There was a problem hiding this comment.
@dranikpg taken as is
| wasmtime::Store store_; | ||
| static wasmtime::Config GetConfig() { | ||
| wasmtime::Config config; | ||
| config.epoch_interruption(false); |
There was a problem hiding this comment.
We still have the same issue after we switch fiber we crash. But loading and executing wasm works fine, so for example when I merge the code in WasmFamily::Load with WasmFamily::Call it properly loads and executes the wasm binary. However it still crashes after it exits just like when we split this in two steps as described above.
| } | ||
| std::unique_lock<util::fb2::SharedMutex> lock(mu_); | ||
| modules_.emplace(name, std::move(result.ok())); | ||
|
|
There was a problem hiding this comment.
We will move registering modules here with a static flag that contains the paths
No description provided.