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.
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
1753: Use a union for VMContext for functions r=MarkMcCaskey a=MarkMcCaskey
This makes the code more self documenting and correctly uses `unsafe`
to communicate that it's the caller's responsibility to ensure that the
code paths that can lead to the access can only write the value they
expect to be there.
spun off of #1739
- [x] There's still some inconsistent use of `vmctx` and `extra_data` might be good to unify this more.
- [x] Improve docs on the new type
Co-authored-by: Mark McCaskey <mark@wasmer.io>
1730: Create example for limiting memory with Tunables r=syrusakbary a=webmaster128
Closes#1588
# Description
This is an attempt for solving #1360 with the new Tunables API. It could be developed further to address #1588 if you like.
I'd appreciate a code review to ensure I'm on the right track.
Open questions:
1. Is it expected that I had to add `wasmer-vm`?
2. Should I really create a new `Target` for this use case when engine already has one set?
3. I used `BaseTunables` for the base implementation and sticked with `Tunables` for the trait name, because it makes most sense to me. However, in `lib/api/src/tunables.rs` it is done the other way round. Any thoughts on the naming issue?
4. The import collision between `wamer::Memory` and `wasmer_vm::Memory` is inconvenient. Can it be avoided or do I do it correctly here?
# Review
- [ ] Add a short description of the the change to the CHANGELOG.md file
Co-authored-by: Simon Warta <simon@warta.it>
1772: Remove lifetime parameter from `NativeFunc` r=MarkMcCaskey a=MarkMcCaskey
Ran into this as an annoyance on #1739 ; will need to account for this when we fix the memory leak in `InstanceHandle`.
Note: the lifetime wasn't doing anything useful for us, this change doesn't make anything new possible (other than not having to deal with the lifetime parameter)
# Review
- [x] Add a short description of the the change to the CHANGELOG.md file
Co-authored-by: Mark McCaskey <mark@wasmer.io>
1770: Enable these tests now that they pass. r=nlewycky a=nlewycky
Also fix a syntax error, use assert_eq! instead of assert! to compare two values for equality.
Also in passing, convert a comment about something not being done into a TODO.
Co-authored-by: Nick Lewycky <nick@wasmer.io>
Also fix a syntax error, use assert_eq! instead of assert! to compare two values for equality.
Also in passing, convert a comment about something not being done into a TODO.
This makes the code more self documenting and correctly uses `unsafe`
to communicate that it's the user's responsibility to ensure that the
code paths that can lead to the access can only write the value they
expect to be there.
