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

fix(vm) Fix memory leak of InstanceHandle #1822

Merged
merged 6 commits into from
Nov 19, 2020
Merged

Commits on Nov 17, 2020

  1. chore(compiler) Rename ModuleInfoTranslation.module_translation to …

    …`.module_translation_state`.
    
    The idea is to avoid confusion with th e`ModuleInfoTranslation` type itself.
    Hywan committed Nov 17, 2020
    Configuration menu
    Copy the full SHA
    49ea2bf View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    e48c0ee View commit details
    Browse the repository at this point in the history
  3. fix(vm) Fix memory leak of InstanceHandle.

    In `wasmer_engine::Artifact::instantiate`, the engine artifact
    allocates a new instance by calling
    `wasmer_vm::InstanceHandle::allocate_instance`. To make it short, this
    function calculates the [`Layout`] of an `wasmer_vm::Instance`, and
    allocates space for it by calling [`alloc`]. This last part is
    important because it means we are responsible to deallocate it with
    [`dealloc`].
    
    The pointer to this `wasmer_vm::Instance` is stored in the
    `wasmer_vm::InstanceHandle` structure. That's the handle that is
    returned by the engine artifact `Artifact::instantiate` method.
    
    This instance handle is then stored in `wasmer::Instance` (through the
    intervention of `wasmer::Module`).
    
    How is it freed? It wasn't. That's the leak. [`dealloc`] was never
    called.
    
    How to free it? We must call [`dealloc`]. There is even a
    `wasmer_vm::InstanceHandle::deallocate` helper to do that
    properly. Neat!
    
    When to free it? That's the tricky part. A `wasmer::Instance` can be
    clonable. To do so, `wasmer_vm::InstanceHandle` must be clonable
    too. There was a `Clone` implementation, that was constructing a new
    `wasmer_vm:InstanceHandle` by using the same pointer to
    `wasmer_vm::Instance`. That's annoying because it's the best way to
    get many pointers that point to the same `wasmer_vm::Instance` in the
    nature, and it's difficult to track them.
    
    This patch changes the paradigm. There is only one and unique
    `wasmer_vm::InstanceHandle` per `wasmer::Instance`, including its
    clones. The handle is now stored inside a
    `Arc<Mutex<wasmer_vm::InstanceHandle>>`. Consequently, when a
    `wasmer::Instance` is cloned, it uses the same
    `wasmer_vm::InstanceHandle`, not a clone of it.
    
    Bonus: `wasmer::Instance` continues to be `Send` + `Sync`.
    
    So. Let's back to our question. When to free
    `wasmer_vm::InstanceHandle`? Response: When `wasmer::Instance` is
    dropped. Right? There is a unique path from `wasmer::Instance`, to
    `wasmer_vm::InstanceHandle`, to `wasmer_vm::Instance` now. So we just
    need to call `wasmer_vm::InstanceHandle::dealloc` in a specific `Drop`
    implementation for `wasmer_vm::InstanceHandle`, and the Rust borrow
    checker does the rest.
    
    Yes. … No. There is another use case: It is possible to create a
    `wasmer_vm::InstanceHandle` with `InstanceHandle::from_vmctx`. Indeed,
    a `wasmer_vm::VMContext` also stores a pointer to
    `wasmer_vm::Instance`. In this, we consider `wasmer_vm::VMContext`
    owns the instance pointer, somehow, and is responsible to free it
    properly.
    
    Consequently, we need another flag inside `wasmer_vm::InstanceHandle`
    to know whether this structure owns the pointer to
    `wasmer_vm::Instance` or not.
    
    So. Let's back to our question. When to free
    `wasmer_vm::InstanceHandle`? Response: Inside the `Drop`
    implementation of `wasmer_vm::InstanceHandle` with its `Self::dealloc`
    method if and only if the handle owns the pointer to
    `wasmer_vm::Instance`.
    
    Testing with Valgrind shows that the leak has been removed.
    
    [`Layout`]: https://doc.rust-lang.org/std/alloc/struct.Layout.html
    [`alloc`]: https://doc.rust-lang.org/std/alloc/fn.alloc.html
    [`dealloc`]: https://doc.rust-lang.org/std/alloc/fn.dealloc.html
    Hywan committed Nov 17, 2020
    Configuration menu
    Copy the full SHA
    8016a34 View commit details
    Browse the repository at this point in the history

Commits on Nov 19, 2020

  1. Configuration menu
    Copy the full SHA
    de78a95 View commit details
    Browse the repository at this point in the history
  2. doc(engine-jit) Fix a typo.

    Hywan committed Nov 19, 2020
    Configuration menu
    Copy the full SHA
    d211ebe View commit details
    Browse the repository at this point in the history
  3. feat(vm) Remove the owned_instance argument of InstanceHandle::new.

    As discussed with @syrusakbary, it's OK to assume
    `InstanceHandle::new` will always own `instance_ptr`.
    Hywan committed Nov 19, 2020
    Configuration menu
    Copy the full SHA
    8e7d7ef View commit details
    Browse the repository at this point in the history