1826: feat(api) Faster `Exports::from_iter` r=Hywan a=Hywan
# Description
Instead of creating a new `Exports` with `Exports::new` and inserting
all pair `(String, Extern)` one after the other with
`Exports::insert`, this patch updates the code to create an `Exports`
directly with its `map` field built with `IndexMap::from_iter`, so
that we avoid calling `Arc::get_mut(…).unwrap().insert(…)` for every
pair.
# Review
- ~[ ] Add a short description of the the change to the CHANGELOG.md file~ (not sure it's necessary)
Co-authored-by: Ivan Enderlin <ivan@mnt.io>
1822: fix(vm) Fix memory leak of `InstanceHandle` r=syrusakbary a=Hywan
# Description
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
# Review
- [ ] Add a short description of the the change to the CHANGELOG.md file
Co-authored-by: Ivan Enderlin <ivan@mnt.io>
Instead of creating a new `Exports` with `Exports::new` and inserting
all pair `(String, Extern)` one after the other with
`Exports::insert`, this patch updates the code to create an `Exports`
directly with its `map` field built with `IndexMap::from_iter`, so
that we avoid calling `Arc::get_mut(…).unwrap().insert(…)` for every
pair.
I hope this little change will clarify a little bit that the `Export`
passed to `Extern::from_vm_export` is not a `wasmer::Export` but a
`wasmer_vm::Export`.
