Improve usabiliy of WasmerEnv trait #2838
Labels
Milestone
Comments
|
We are rethinking the API for Wasmer 3.0, this issue should be fixed by those changes. Thanks for the ticket! |
8 tasks
|
This one should be fixed by #2924 |
|
Closing this in favor of #2924 |
bors bot
added a commit
that referenced
this issue
Jul 19, 2022
2892: Implement new Context API for Wasmer 3.0 r=epilys a=Amanieu ### Overview This PR reworks the way Wasmer manages objects used by instances, specifically: `Instance`, `Memory`, `Table`, `Function`, `Global` and `ExternRef`. Previously these were tracked using reference counting, but this cannot work fully because Wasm code can manipulate references to functions (and by implication the instance that function is from) using tables. The new approach uses a `Context` type which owns all of the objects listed above: objects are only freed when the `Context` is dropped. The `Instance`, `Memory`, `Table`, `Function`, `Global` and `ExternRef` types are now simply wrappers around an integer which indexes a vector in the `Context`. As a result, any function on those "handle" types now need to take a `&Context` or `&mut Context` as a parameter to access the actual data in the context. This also vastly simplifies Wasmer's thread-safety story: since all mutating operations require a `&mut Context`, no synchronization or internal locks are needed. Additionally, the way host functions are represented is also changed. The `WasmerEnv` trait is removed since it was unergonomic and hard to use correctly. Instead, the state for host functions is stored directly as the `T` in `Context<T>`. This state is initialized when the context is created, and can be accessed through the `data` and `data_mut` methods on `Context<T>`. All host functions receive a `ContextMut<'_, T>` as their first argument, which gives them access to the host state of the context. This is much better than the old approach since it provides mutable access without locks and allows all host functions to share the same state. This design is heavily inspired by [this RFC](https://github.com/bytecodealliance/rfcs/blob/main/accepted/new-api.md) from Wasmtime. ### TODO - [x] #2985 - [x] #2908 - [x] #2973 Upgrade `c-api` to use the new API - [x] Update wasmer-wasi to use the new API - [x] #2909 - [x] #2913 - [x] #2912 - #2911 (Non blocking) - [ ] #2910 Fixes #2893 Fixes #2873 Fixes #2866 Fixes #1840 Fixes #1734 Fixes #1632 Fixes #1522 Fixes #1630 Fixes #2838 Fixes #2856 Fixes #2544 Fixes #2816 Co-authored-by: Amanieu d'Antras <[email protected]> Co-authored-by: ptitSeb <[email protected]> Co-authored-by: Manos Pitsidianakis <[email protected]> Co-authored-by: Syrus Akbary <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Motivation
init_with_instanceprovides the host env with a&Instance, whereInstanceisClone, making it very tempting for people to simply clone the instance and store it on the host env for later use, which seems to create a memory leak.Proposed solution
init_with_instancecould take an argument that would enforce the implicit constraints of the method.For example a
&InitialiazedInstanceProxy, which would not beClone, and would have a method likeget_export_with_generics_weak, wich would proxy internally toinstance.exports.get_with_generics_weak.Alternative
Clarify this issue in the documentation. Good idea anyway, since people can still easily store an
Instanceon their host env outside ofinit_with_instance.Additional context
The temptation to store
Instanceon the host env is especially tempting in the light of usingwasmer_middlewares::metering, since helper functions likeget_remaining_pointstake an&Instanceas argument, and it's not obvious how to get access to it from host functions unless stored on the env.Example of mistake made in the wild and fix: massalabs/massa-sc-runtime#121
The text was updated successfully, but these errors were encountered: