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 through FuncDataRegistry when repeatedly instantiating module #2638

Closed
matklad opened this issue Oct 29, 2021 · 2 comments
Closed
Assignees
Labels
bug Something isn't working priority-high High priority issue
Milestone

Comments

@matklad
Copy link
Contributor

matklad commented Oct 29, 2021

Describe the bug

The following code:

let module = wasmer::Module::new(&store, &wasm).unwrap();
loop {
  let imports = ...;
  let instance = wasmer::Instance::new(&module, &imports).unwrap();
}

uses unbounded amount of memory

Steps to reproduce

Full repro is here: https://github.com/matklad/repros/blob/9f98e5cda5a54a7242ba2b3c39b1a36bee5d4404/wasmer-leak/src/main.rs

Expected behavior

Bounded memory usage

Actual behavior

Memory usage increases without bound

Additional context

Reproduces this with both wasmer2 and current master. The culprit is this data structure:

func_data_registry: Arc<FuncDataRegistry>,

This is a hashmap keyed by addresses of all imported functions. It is a part of Store, so it is shared across all Instances we create. The hashmap is append only and never shrinks, so it accumulates imports from long-dead instances.

@matklad matklad added the bug Something isn't working label Oct 29, 2021
@Amanieu Amanieu added the priority-high High priority issue label Oct 29, 2021
@syrusakbary
Copy link
Member

Related PR: #2437

wasmerbot pushed a commit that referenced this issue Oct 29, 2021
matklad pushed a commit to matklad/wasmer that referenced this issue Nov 1, 2021
This backports
  wasmerio/wasmer#2437
to fix
  wasmerio/wasmer#2638

On a high-level, Store (global state for the whole wasmer runtime) used
to store pointers to host functions. This was a partial implementation
of the module linking proposal: for that, we need to be able to use
functions *across* several instances, so we need to keep such functions
in some sort of global state.

This commit moves the data to the Instance. This breaks some module
linking related tests, but we are not using that, and the support wasn't
complete anyway.

Note that the actual memory management remains the same:
`Instance::imported_function_envs` is the thing that was and still is
responsible for dropping data related to external functions.
matklad pushed a commit to matklad/wasmer that referenced this issue Nov 1, 2021
This backports
  wasmerio/wasmer#2437
to fix
  wasmerio/wasmer#2638

On a high-level, Store (global state for the whole wasmer runtime) used
to store pointers to host functions. This was a partial implementation
of the module linking proposal: for that, we need to be able to use
functions *across* several instances, so we need to keep such functions
in some sort of global state.

This commit moves the data to the Instance. This breaks some module
linking related tests, but we are not using that, and the support wasn't
complete anyway.

Note that the actual memory management remains the same:
`Instance::imported_function_envs` is the thing that was and still is
responsible for dropping data related to external functions.
@syrusakbary syrusakbary added this to the v2.1 milestone Nov 3, 2021
@Amanieu Amanieu self-assigned this Nov 3, 2021
@syrusakbary
Copy link
Member

Thanks to #2699 we now don't leak the function references by default to the store.

This means that if the Wasm is not using reference types, the functions are not stored in the store, and therefore not leaked anywhere.
However, if the funcrefs are used this will be registered in the store until the store drops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-high High priority issue
Projects
None yet
Development

No branches or pull requests

3 participants