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 creating new Store, Module and Instance #1714

Closed
evq opened this issue Oct 12, 2020 · 4 comments
Closed

Memory leak when creating new Store, Module and Instance #1714

evq opened this issue Oct 12, 2020 · 4 comments
Assignees
Labels
1.0 Wasmer at 1.0 bug Something isn't working

Comments

@evq
Copy link

evq commented Oct 12, 2020

Describe the bug

Memory leak

Steps to reproduce

Reproduces using the following variation of https://github.com/wasmerio/wasmer-python/blob/master/examples/appendices/simple.py

from wasmer import Store, Module, Instance
import os

__dir__ = os.path.dirname(os.path.realpath(__file__))

while True:
    module = Module(Store(), open(__dir__ + '/simple.wasm', 'rb').read())

    instance = Instance(module)
    result = instance.exports.sum(1, 2)

    print(result) # 3!

Expected behavior

The program should not leak memory.

Actual behavior

Memory is leaked eventually leading to:

Traceback (most recent call last):
  File "simple.py", line 9, in <module>
    instance = Instance(module)
RuntimeError: Insufficient resources: Failed to create memory: Error when allocating memory: Cannot allocate memory (os error 12)
@Hywan Hywan self-assigned this Oct 13, 2020
@Hywan
Copy link
Contributor

Hywan commented Oct 13, 2020

Thank you for the bug report.

What version are you using? Is it from the master branch?

@evq
Copy link
Author

evq commented Oct 13, 2020 via email

@syrusakbary
Copy link
Member

@Hywan I think the memory leak could be in the Wasmer Rust implementation itself (not in the Python integration).
We should investigate probably in the main repo :)

@Hywan Hywan transferred this issue from wasmerio/wasmer-python Oct 13, 2020
@MarkMcCaskey
Copy link
Contributor

Related #1584 #1667 , I think all these are referring to the same bug

@jubianchi jubianchi added 1.0 Wasmer at 1.0 bug Something isn't working labels Nov 20, 2020
@jubianchi jubianchi self-assigned this Dec 1, 2020
bors bot added a commit that referenced this issue Dec 15, 2020
1865: Fix memory leak in host function envs r=MarkMcCaskey a=MarkMcCaskey

TODO: link to issue

This PR contains a number of changes:

1. Make `WasmerEnv: Clone`
2. Store a pointer to the `clone` function when creating a host function (Notably this is a feature that wouldn't work even if we _could_ use a proper trait object because you can't have a `Sized` trait object and `Clone: Sized`).
3. Store a pointer to the `drop` function when creating a host function.
4. Clone the env via pointer every time an `Instance` is made. Therefore each `Instance` gets its own, unique `Env` per host function with `Env`.
5. Add reference counting and drop logic to a sub-field of `wasmer_export::ExportFunction` which frees the original version of the `Env` (the thing that gets cloned each time an `Instance` is made) with the `drop` function pointer.
6. Change some logic in `vm::Instance` from SoA (struct of arrays) to AoS (array of structs): this uses more memory but is a bit less error prone and can be easily changed later.
7. Add logic on this new struct (`vm::ImportEnv`) that contains the function pointers for each import in `Instance` to drop (with the `drop` fn pointer) when the `vm::Instance` is being dropped. This fixes the original memory leak.
8. Add wrapper functions inside the host function creation functions which makes the layout of the user supplied env-pointer the responsibility of each function.  Thus, rather than `drop` being `Env::drop`, it's a function which frees all wrapper types, traverses indirections and frees the internal `Env` with `Env::drop`.  This simplifies code at the cost of making the `host_env` pointer (`vmctx`) not consistent in terms of what it actually points to.  This change fixes another memory leak related to the creation of host functions.

tl;dr: we're leaning into manually doing virtual method dispatch on `WasmerEnv`s and it actually works great! The biggest issue I have with the PR as-is is that the code isn't as clean/readable/robust as I'd ideally like it to be.

Edit (by @Hywan): This PR fixes #1584, #1714, #1865, #1667.

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
bors bot added a commit that referenced this issue Dec 15, 2020
1865: Fix memory leak in host function envs r=MarkMcCaskey a=MarkMcCaskey

TODO: link to issue

This PR contains a number of changes:

1. Make `WasmerEnv: Clone`
2. Store a pointer to the `clone` function when creating a host function (Notably this is a feature that wouldn't work even if we _could_ use a proper trait object because you can't have a `Sized` trait object and `Clone: Sized`).
3. Store a pointer to the `drop` function when creating a host function.
4. Clone the env via pointer every time an `Instance` is made. Therefore each `Instance` gets its own, unique `Env` per host function with `Env`.
5. Add reference counting and drop logic to a sub-field of `wasmer_export::ExportFunction` which frees the original version of the `Env` (the thing that gets cloned each time an `Instance` is made) with the `drop` function pointer.
6. Change some logic in `vm::Instance` from SoA (struct of arrays) to AoS (array of structs): this uses more memory but is a bit less error prone and can be easily changed later.
7. Add logic on this new struct (`vm::ImportEnv`) that contains the function pointers for each import in `Instance` to drop (with the `drop` fn pointer) when the `vm::Instance` is being dropped. This fixes the original memory leak.
8. Add wrapper functions inside the host function creation functions which makes the layout of the user supplied env-pointer the responsibility of each function.  Thus, rather than `drop` being `Env::drop`, it's a function which frees all wrapper types, traverses indirections and frees the internal `Env` with `Env::drop`.  This simplifies code at the cost of making the `host_env` pointer (`vmctx`) not consistent in terms of what it actually points to.  This change fixes another memory leak related to the creation of host functions.

tl;dr: we're leaning into manually doing virtual method dispatch on `WasmerEnv`s and it actually works great! The biggest issue I have with the PR as-is is that the code isn't as clean/readable/robust as I'd ideally like it to be.

Edit (by @Hywan): This PR fixes #1584, #1714, #1865, #1667.

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Wasmer at 1.0 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants