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

Port C API to new Context API #2973

Merged
merged 3 commits into from
Jun 30, 2022
Merged

Port C API to new Context API #2973

merged 3 commits into from
Jun 30, 2022

Conversation

epilys
Copy link
Contributor

@epilys epilys commented Jun 24, 2022

Not done: tests

@epilys epilys added this to the v3.0 milestone Jun 24, 2022
@epilys epilys self-assigned this Jun 24, 2022
@epilys epilys added the 📦 lib-c-api About wasmer-c-api label Jun 24, 2022
@epilys epilys changed the title Context api capi Port C API to new Context API Jun 24, 2022
@epilys epilys force-pushed the context_api_capi branch from 7c5aaa2 to f41ce3c Compare June 24, 2022 12:21
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't change the signatures of wasm_global_type, wasm_global_set, ... from store to ctx since those signatures are given by the headers in the official wasm-c-api and we must preserve them.
You can check the official headers here: https://github.com/WebAssembly/wasm-c-api

I propose to attach the ctx to the store itself (when it's created on the c-api) and that way there will be no breaking changes on the API.

@epilys epilys force-pushed the context_api_capi branch from f41ce3c to d647351 Compare June 26, 2022 12:03
@epilys
Copy link
Contributor Author

epilys commented Jun 26, 2022

We shouldn't change the signatures of wasm_global_type, wasm_global_set, ... from store to ctx since those signatures are given by the headers in the official wasm-c-api and we must preserve them. You can check the official headers here: https://github.com/WebAssembly/wasm-c-api

I propose to attach the ctx to the store itself (when it's created on the c-api) and that way there will be no breaking changes on the API.

Resolved

@epilys epilys force-pushed the context_api_capi branch from d647351 to b58d1f1 Compare June 26, 2022 12:11
@epilys epilys marked this pull request as ready for review June 26, 2022 12:11
@epilys epilys requested a review from syrusakbary June 26, 2022 16:11
@epilys epilys force-pushed the context_api_capi branch from b58d1f1 to abd3e11 Compare June 28, 2022 14:48
@epilys epilys requested review from syrusakbary and removed request for syrusakbary June 28, 2022 14:49
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few comments over locking the context, but it's intended for almost any usage of the ctx.lock() in the PR.

In general, we shouldn't rely on locking the context. The Wasm C-API is designed to push that responsibility into the user code, not on the API side

@epilys epilys force-pushed the context_api_capi branch from abd3e11 to 7bc6b42 Compare June 29, 2022 13:18
@epilys epilys requested a review from syrusakbary June 29, 2022 13:18
@epilys epilys force-pushed the context_api_capi branch from 7bc6b42 to 16c27c8 Compare June 29, 2022 13:34
@epilys epilys linked an issue Jun 29, 2022 that may be closed by this pull request
@epilys epilys force-pushed the context_api_capi branch from 96e7e90 to 53d7219 Compare June 30, 2022 13:49
@syrusakbary syrusakbary merged commit 2a589ae into context_api Jun 30, 2022
@bors bors bot deleted the context_api_capi branch June 30, 2022 19:38
bors bot added a commit that referenced this pull request 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
Labels
📦 lib-c-api About wasmer-c-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify Wasm C API implementation
2 participants