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

Environment passed to Function::new_native_with_env is never dropped and leaks memory #1667

Closed
bkolobara opened this issue Oct 2, 2020 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@bkolobara
Copy link

bkolobara commented Oct 2, 2020

I'm running wasmer 1.0.0-alpha.3 and recently run into a problem that I'm not sure how to work around. For every instance I crate a new ImportObject capturing an environment with some functions.

The issue is that this environment is never dropped and I can't figure out a way of releasing the memory. My environment is a bit heavier and after running the app for some time I start running out of memory.

I created a minimal example just to illustrate the issue:

use wasmer::{Store, Instance, Module, Exports, ImportObject, Function};

struct Env {
    multiplier: i32,
}

impl Drop for Env {
    fn drop(&mut self) {
        println!("Dropping: {}", self.multiplier);
    }
}

fn main() {
    let store = Store::default();
    let module = Module::new(&store, "(module (import \"env\" \"sum_and_multiply\" (func (param i32 i32) (result i32))))").unwrap();

    let env_dropped = Env { multiplier: 1 };
    let env_never_dropped = Env { multiplier: 2 };
    let f = Function::new_native_with_env(&store, env_never_dropped, sum_and_multiply);

    let mut exports = Exports::new();
    exports.insert("sum_and_multiply", f);

    let mut import_object = ImportObject::new();
    import_object.register("env", exports);

    let instance = Instance::new(&module, &import_object).unwrap();
}

fn sum_and_multiply(env: &mut Env, a: i32, b: i32) -> i32 {
    (a + b) * env.multiplier
}

The output of this app is:

Dropping: 1

As you can see env_dropped is never moved and drops, but env_never_dropped doesn't get it's drop method called. Even after the instance, module and store are all gone.

Ideally I would expect that once there are no more Instances referencing the ImportObject that the resources get released.

It can also be that I structured my app in an unconventional way. Is there maybe a better approach to this?

@bkolobara bkolobara added the bug Something isn't working label Oct 2, 2020
@bkolobara
Copy link
Author

I think this is related to #1584.

@MarkMcCaskey
Copy link
Contributor

Thanks for the issue, I think this is the same as #1584 too;

I believe once either #1663 or #1625 lands this will be easier to fix as the general ownership of Env is clarified a bit more in those PRs.

We can probably make a solution that's compatible with both of those PRs though

@MarkMcCaskey MarkMcCaskey self-assigned this Oct 2, 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]>
@jubianchi
Copy link
Contributor

Closing this as #1865 got merged and it fixes the issue. Here is the original snippet, adapted with new requirements:

use wasmer::{Store, Instance, Module, Exports, ImportObject, Function, WasmerEnv};

#[derive(WasmerEnv, Clone)]
struct Env {
    multiplier: i32,
}

impl Drop for Env {
    fn drop(&mut self) {
        println!("Dropping: {:?}", self.multiplier);
    }
}

fn main() {
    let store = Store::default();
    let module = Module::new(&store, "(module (import \"env\" \"sum_and_multiply\" (func (param i32 i32) (result i32))))").unwrap();

    let env_dropped = Env { multiplier: 1 };
    let env_never_dropped = Env { multiplier: 2 };

    let f = Function::new_native_with_env(&store, env_never_dropped, sum_and_multiply);

    let mut exports = Exports::new();
    exports.insert("sum_and_multiply", f);

    let mut import_object = ImportObject::new();
    import_object.register("env", exports);

    let instance = Instance::new(&module, &import_object).unwrap();
}

fn sum_and_multiply(env: &Env, a: i32, b: i32) -> i32 {
    (a + b) * env.multiplier
}

The output is now:

Dropping: 2
Dropping: 2
Dropping: 1

What did I change in the snippet:

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

No branches or pull requests

3 participants