Module instances are leaked #3930
Comments
vapourismo
changed the title
|
The same program in the equivalent Rust also leaks the WebAssembly module instance. use std::{error::Error, thread::sleep, time::Duration};
use wasmer::{Imports, Instance, Module, Store};
fn main() -> Result<(), Box<dyn Error>> {
let mut store = Store::default();
let module = Module::new(
&store,
r#"
(module
(memory 100))
"#,
)?;
let imports = Imports::new();
loop {
eprintln!("Start");
let _instance = Instance::new(&mut store, &module, &imports)?;
eprintln!("Stop");
sleep(Duration::from_secs(1));
}
}Retained allocations: |
|
Oh, so it's not c-api specific! I'll try to do some valgrind analysis, see if I can identify the source of the leak |
|
I believe this is somewhat unsurprising, but re-allocating the Store with a cloned Engine gets rid of the leak. use std::{error::Error, thread::sleep, time::Duration};
use wasmer::{Engine, Imports, Instance, Module, Store};
fn main() -> Result<(), Box<dyn Error>> {
let engine = Engine::default();
let module = Module::new(
&engine,
r#"
(module
(memory 100))
"#,
)?;
let imports = Imports::new();
loop {
{
eprintln!("Start");
let mut store = Store::new(engine.clone());
let _instance = Instance::new(&mut store, &module, &imports)?;
eprintln!("Stop");
}
sleep(Duration::from_secs(1));
}
}Purely guessing, but maybe there is a cyclic reference between the VM Store and VM Instance's objects. |
|
Actually if it were properly cyclic, it would most likely leak the memory anyway, despite re-creating the store. |
|
So, the problem here is that
In a certain sense this is by design.
So this is expected behaviour, not a leak. I do understand that this might be unexpected behaviour though, that also isn't clearly documented. There also is no way to clean up the relevant data from a store if you would want to re-use it, but offering such a cleanup would come with complications, due to |
|
So I'm leaning towards "this is expected behaviour", and thus primarily a documentation issue. This might change in the future though when we want to utilize stores to share code and internal state between instances (like in the component model), but this isn't currently the case. |
|
I figured as much. Yet, I am still surprised at this flaw. There are major problems with this, if it is really by design. C APIWhile creating a Module in Wasmer doesn't require the Store itself, the standard C API enforces this: WASM_API_EXTERN own wasm_module_t* wasm_module_new(
wasm_store_t*, const wasm_byte_vec_t* binary);From a C perspective this makes it really awkward when you need to re-create the Module for each Store while the Module hasn't actually changed. Re-creating the Store isn't always possibleImagine a scenario where I have a few "library" instances, that I need to keep around for their state hence need to stay in the store, and many short-lived instances that depend on the "library" instances. I would be paying a massive memory price if these short-lived instances allocate a few gigabytes of memory every time. I feel like there are options that would improve the situation significantly:
|
|
We have had similar discussions, but there are some design changes that would be needed and it would potentially complicate the Rust API quite a bit, or introduce some performance tradeoffs, so that's part of a larger discussion. Types like
We inherited the C API from the upstream standardized https://github.com/WebAssembly/wasm-c-api . |
|
Understand that the standardised API constraints you, but would you consider a WASM_API_EXTERN own wasm_module_t* wasmer_module_new(
wasm_engine_t*, const wasm_byte_vec_t* binary); |
|
Looks like a good idea, if you can do a Pull Request about that, we will merged it. |
|
Was merged. Closing. |
Describe the bug
Created module instances are leaked.
See culprits in Instruments:
Wasmer: 3.3.0
Rust: 1.66.0
Platform: macOS 13.4
Steps to reproduce
Minimal C program to try with:
Compile and run
Minimal Rust program:
Observe memory growing over time.
Expected behavior
Memory usage does not grow over time.
Actual behavior
Memory grows unboundedly.
The text was updated successfully, but these errors were encountered: