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

Memory leak when repeatedly instantiating a WASM module #3130

Open
khoover opened this issue Nov 1, 2022 · 4 comments
Open

Memory leak when repeatedly instantiating a WASM module #3130

khoover opened this issue Nov 1, 2022 · 4 comments
Labels

Comments

@khoover
Copy link

khoover commented Nov 1, 2022

Describe the Bug

The heap array grows without bounds when repeatedly instantiating a WASM module and calling any function that adds a JS object to the heap.

Steps to Reproduce

  1. Build
use wasm_bindgen::prelude::*;
use std::cell::RefCell;

thread_local! {
  pub static VALUE: RefCell<Option<JsValue>> = RefCell::new(None);
}

#[wasm_bindgen]
pub fn set_value(val: JsValue) {
  VALUE.with(move |global_val| {
    *global_val.borrow_mut() = Some(val);
  });
}
  1. Repeatedly init the wasm module and call this function with a new JS object each time, e.g. while (true) { const instance = initSync(module); instance.set_value(new Object()); }.

Expected Behavior

Memory usage to stay level while running.

Actual Behavior

Memory usage continuously increases while running.

Additional Context

In the init method, even though wasm gets changed (effectively invalidating the old instance, as now the module-level functions will be referring to a different instance when using wasm), the heap isn't re-initialized. So all the values that were stored in the old instance continue to be held onto, even when they're effectively unusable by the new instance. This can be observed when, e.g., the Rust code uses a panic handler that calls throw_str, where the calling JS code will re-init the module after a panic (similar to Erlang behaviour).

@khoover khoover added the bug label Nov 1, 2022
@alexcrichton
Copy link
Contributor

Unfortunately there's not really anything that can be done about this. You'd need some sort of destructor for the entire instance akin to "program shutdown" which is not implemented. Such a destructor would be responsible for clearing this global.

If you're creating tons of wasm instances I'd recommend not storing things in globals and instead using objects which should have destructors to trigger things correctly with finalizers.

@khoover
Copy link
Author

khoover commented Nov 1, 2022

Can init/initSync not just wipe the global? Once you change the wasm variable to point to the new instance, the old one is basically unusable anyway (side note: is there any reason why wasm and heap are module globals vs. closure-captured stack variables or properties of the imports object?).

For my use-case, it's not really about creating tons of instances, more when an instance with a lot of heap values attached crashes and is restarted.

@alexcrichton
Copy link
Contributor

What you probably want is to have all the intrinsics and such that wasm-bindgen uses to be closure state in the initialization function. That way they'd all get gc'd with everything else when they're no longer referenced (e.g. after something crashes and has been replaced).

@khoover
Copy link
Author

khoover commented Nov 3, 2022

That's exactly it, yeah; as far as I can tell, there's not really any reason the various expose_x-generated functions can't take their associated global(s) as input, and have something like

export function initSync(module) {
  let globalsObj = { heap: /* do heap init */ };
  const imports = getImports(globalsObj);
  // ...
  const instance = new WebAssembly.Instance(module, imports);
  globalsObj.wasm = instance.exports;
  return finalizeInit(instance, module);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants