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

RFC: Redesign Wasmtime's API #11

Merged
merged 8 commits into from
Jun 3, 2021

Conversation

alexcrichton
Copy link
Member

Rendered


Overhaul the wasmtime crate's API to improve it along a number of vectors:

  • Greatly improve the multithreading story in all languages (Rust, C, Go, ...),
    namely enabling safe usage of wasmtime in multithreaded server runtimes
    where wasm objects can migrate between threads.
  • Leverage mutability in Rust to allow &mut access to user-defined state
    instead of requiring users to use interior mutability.
  • Simplify memory management in the C API and embeddings.

The only major cost relative to today's API is that ExternRef will move to
atomic reference counting and will require T: Send + Sync on constructors.

Overhaul the `wasmtime` crate's API to improve it along a number of vectors:

* Greatly improve the multithreading story in all languages (Rust, C, Go, ...),
  namely enabling safe usage of `wasmtime` in multithreaded server runtimes
  where wasm objects can migrate between threads.
* Leverage mutability in Rust to allow `&mut` access to user-defined state
  instead of requiring users to use interior mutability.
* Simplify memory management in the C API and embeddings.

The only major cost relative to today's API is that `ExternRef` will move to
atomic reference counting and will require `T: Send + Sync` on constructors.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 5, 2021
This commit removes a TLS lookup of the store information whenever a
trap is created. This was previously done to lookup information in the
module registry, but nowadays there's a global registry which has all
the equivalent information as well. This commit moves around some code
in the module registry and then uses this global map when creating
traps.

This is not motivated by performance, and likely regresses performance
slightly since it involves taking a global lock now. That being said the
perf of creating a `Trap` is generally not super consequential, so this
is seen as ok.

The goal of this commit is to lessen Wasmtime's reliance on TLS for
store-based information. I'm hoping to lessen this even more with the
changes for bytecodealliance/rfcs#11, where acquiring a store via TLS is
going to be much more difficult if not possible any more.
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

A few preliminary comments, but this looks great!

accepted/new-api.md Outdated Show resolved Hide resolved
accepted/new-api.md Outdated Show resolved Hide resolved

* All closures used to create a `Func` are required to be `Send + Sync`. This is
because the types are stored in `Store` and we want `Store` to be `Send +
Sync`. You still only get `Fn`, though, instead of `FnMut`. Mutability of host
Copy link
Member

Choose a reason for hiding this comment

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

Above it is suggested that having the Store be Send + Sync is optional and depends on T being Send + Sync too; so is this a strong requirement, even when the embedder doesn't need a Send + Sync Store?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct, regardless of whether the embedder needs Send and Sync host functions will always be required to be Send and Sync. This is thought to be ok, though, because most host state should be stored in T rather than in the funtion itself, and it's expected that the majority of host functions are simply 0-sized closures that are just code and no data.

accepted/new-api.md Outdated Show resolved Hide resolved
```rust
impl<'a, T> Caller<'a, T> {
fn context(&self) -> StoreContext<'_, T>;
fn context_mut(&mut self) -> StoreContextMut<'_, T>;
Copy link
Member

Choose a reason for hiding this comment

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

Are these two functions doing more than what the AsContext/Mut trait methods provide?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah they're just explicitly named so if users want to call them they don't have to import the trait. I'm not sure if this makes sense but it's always an option we can have of course!

@acfoltzer
Copy link

I'm so excited about this, thank you for making it happen!

I'm a little unclear about the role of the store's type variable T from a library user perspective. I think it would help to have some examples of user code showing how the T impacts the developer experience. Is it usually going to be inferred or fixed by library code, or will the user have to reason about what this variable should be?

@alexcrichton
Copy link
Member Author

A good question! It's sort of tangentially touched on here but the general idea is that if you are a library crate then to work with a Store<T> you'll probably place some sort of trait bound on T to get access to the data you'd want. For example with WASI:

// in the wasmtime-wasi crate
fn add_to_linker<T>(funcs: &mut Linker<'_, T>)
    where T: AsMut<WasiCtx>;

or alternatively:

// in your crate
fn add_to_linker<T>(
    funcs: &mut Linker<'_, T>,
    get: impl Fn(&mut T) -> &mut MyLibraryState + Copy + Send + Sync + 'static,
);

The idea being that the owner of the store will still determine the T for you. Libraries will work generically with some T with trait bounds or accessors or similar.

In terms of application code and such I'd imagine that the T is generally inferred from a call to Store::new for you, given that you're selecting some type to be owned by the store there.

These locations may involve adding items into the `Store`, so a mutable
view is needed.
@acfoltzer
Copy link

Hmm, I think I get it. To check my understanding, if I want to have my own application-specific context type but also a WASI context, how might that look? Create my own combined context type that impls both AsMut<WasiCtx> and AsMut<MyAppCtx>?

@alexcrichton
Copy link
Member Author

@acfoltzer indeed! While it's not the prettiest this is an example in the branch I'm working on. The wasmtime CLI optionally supports some WASI proposals both at runtime and compile time, and that struct is used to manage the state where it's dynamically initialized and then used by linker-added functions through the BorrowMut traits

@acfoltzer
Copy link

Perfect, that makes tons of sense. Thank you!

- The current implementation selects a 64-bit integer with no destructor as the
implementation of C API objects like `wasmtime_func_t`. This is done to have
the type be a "pod" type without a destructor that doesn't need to be managed,
it's simply an index into the `Store`. This implementaiton detail is how the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it's simply an index into the `Store`. This implementaiton detail is how the
it's simply an index into the `Store`. This implementation detail is how the

@alexcrichton
Copy link
Member Author

I've updated with a tweak to the APIs in languages like Python, Go, and .NET. Originally I intended that "pass the store around" wouldn't be as common there, but the behavior of the GC in Go and converting a wasmtime_val_t to a wasmtime_func_t has tipped my opinion in favor of "pass the store everyhwere" like what happens in C & Rust.

@alexcrichton
Copy link
Member Author

alexcrichton commented May 24, 2021

Motion to finalize with a disposition to merge

We've reached out to various Wasmtime embedders, and have received very positive feedback on the changes proposed here. Additionally, we have full implementations of the proposal for Wasmtime's Rust and C-APIs, wasmtime-py, and wasmtime-go.

We also analyzed the impact on various existing embeddings, and found it to be very reasonable in all cases.

As such, we proposing finalizing this RFC with a disposition to merge it.

Stakeholders sign-off

Given the broad impact of these changes, we decided to also tag a broad list of contributors and Wasmtime embedders for feedback.

Arm

DFINITY

Embark Studios

Fastly

Intel

Microsoft

Mozilla

IBM

wasmCloud

Unaffiliated

@acfoltzer
Copy link

acfoltzer commented May 24, 2021

@alexcrichton
Copy link
Member Author

Entering Final Call Period

https://github.com/bytecodealliance/rfcs/blob/main/accepted/rfc-process.md#making-a-decision-merge-or-close

Once any stakeholder from a different group has signed off, the RFC will move into a 10 calendar day final comment period (FCP), long enough to ensure that other stakeholders have at least a full business week to respond.

The FCP will end on Thursday June 3.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Could you briefly discuss how this design could be extended to support a hypothetical native wasm threads proposal? Is this just a matter of using a T that has Send + Sync, or would this need additional support?

accepted/new-api.md Outdated Show resolved Hide resolved
Co-authored-by: Dan Gohman <dev@sunfishcode.online>
@alexcrichton
Copy link
Member Author

alexcrichton commented May 24, 2021

I think it may be a little too early to say how we might support a proposal like that. The discussion on that issue is largely about thread id representation which is somewhat orthogonal to the embedding API. For the embedding API though the proposal is so vague at this point I'm not sure what our design constraints are. For example with a hypothetical thread.new instruction:

  • I'm not sure how the imports of the current module are handled. For example if you imported a function, how is that fuction import satisfied on the other thread? Similarly if you have a function table in the module it's not clear how that would be "forked" into another thread. This might mean recursively instantiating a whole bunch of modules, but you'd also bottom out in host closures and be faced with a question of how to "fork" them to new threads too.
  • I'm not sure how globals would be handled. Current globals, like the stack pointer, naturally don't want to be shared amongst threads and the sub-thread would want to get a fresh copy. (but where does the initial value come from?)

I'm sure there's other questions too, but without knowing much about the proposal it's hard to say what the embedding API might look like.

My rough mental model for the current wasm threads proposal (not to be confused with that issue for a native wasm thread) is that there'd be a Store-per-thread. Here T wouldn't need Send or Sync since the embedder would simply arrange the Store to be on its own thread somewhere else. Trying to fuse this with a native wasm threads proposal sort of looks like we'd need like a "store factory" where there's a built-in ability to create a Store on another thread. That namely does indeed need to create a T, and that could presumably be done with a custom trait/helper or even just T: Clone. This would indeed require T: Send. I don't think that T: Sync would make sense since we provide mutable access to the T which means that we can't share the T across threads anyway.

Overall I don't think we're necessarily in a bad position to support a proposal like that, but it's tough to say given the current state. I do think that we're still better off than we are today where host state is littered across all sorts of host functions and such. The only state in a Store which is maybe not Send and Sync is the T itself. This means that all reasoning about thread-safety is entirely tied to T since everything else is threadsafe (e.g. host closures are all Fn() + Send + Sync).

Currently there's a bunch of pieces floating around where if you connect a few and squint hard it sort of looks like an implementation of a native threads proposal, but I think there needs to be more concrete design planning and such done first.

Copy link

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

I think this API will be very nice for consumers. The trade offs and concerns are well documented as well. Really great work here!

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Great overhaul, it makes manipulating the API much more explicit and clear to reason with in multithreaded contexts!

@repi
Copy link

repi commented Jun 3, 2021

Good stuff! 👍

@alexcrichton
Copy link
Member Author

Ok the FCP period has now finished and no objections have been raised. Time to merge then!

@alexcrichton alexcrichton merged commit 521ad34 into bytecodealliance:main Jun 3, 2021
alexcrichton added a commit to alexcrichton/wasmtime-py that referenced this pull request Jun 3, 2021
This commit updates wasmtime-py to account for [RFC 11] and the changes
in Wasmtime's own C API. Note that this is a breaking change for this
binding, notably requiring that `Store` is a uniquely-owned value now
which must be passed in as context to most methods that operate with a
store. More details are in the commit itself and updated tests and such.

[RFC 11]: bytecodealliance/rfcs#11
alexcrichton added a commit to alexcrichton/wasmtime-go that referenced this pull request Jun 3, 2021
This commit updates this repository to use Wasmtime's new C API as
specified by [RFC 11]. This is a breaking change for this API and
notably requires that a `Storelike` object is now passed in for many
functions, serving as the `Store` context for that method. The
implementation of memory management, however, has been greatly
simplified, now that objects within a `Store` no longer need
destructors. This should improve performance and also help avoid
reclaiming memory only at odd times.

[RFC 11]: bytecodealliance/rfcs#11
alexcrichton added a commit to bytecodealliance/wasmtime-py that referenced this pull request Jun 3, 2021
* Update to Wasmtime's new C API

This commit updates wasmtime-py to account for [RFC 11] and the changes
in Wasmtime's own C API. Note that this is a breaking change for this
binding, notably requiring that `Store` is a uniquely-owned value now
which must be passed in as context to most methods that operate with a
store. More details are in the commit itself and updated tests and such.

[RFC 11]: bytecodealliance/rfcs#11

* Fix compat with Python 3.6

* Test Python 3.9 on CI

* Cancel in-flight jobs on CI
alexcrichton added a commit to bytecodealliance/wasmtime-go that referenced this pull request Jun 3, 2021
* Update with Wasmtime's new C API

This commit updates this repository to use Wasmtime's new C API as
specified by [RFC 11]. This is a breaking change for this API and
notably requires that a `Storelike` object is now passed in for many
functions, serving as the `Store` context for that method. The
implementation of memory management, however, has been greatly
simplified, now that objects within a `Store` no longer need
destructors. This should improve performance and also help avoid
reclaiming memory only at odd times.

[RFC 11]: bytecodealliance/rfcs#11

* Try fixing bazel build

* Cancel in-flight jobs on CI
olivierlemasle added a commit to olivierlemasle/opa that referenced this pull request Jun 15, 2021
This version of wasmtime-go reflects new Wasmtime C API specified
by [RFC 11], and requires some changes in the way it is called.

Also, the imports that were present only to include the C headers and static
lib in `go vendor`ed dependencies are no longer required, because these paths
are now imported by wasmtime-go itself in [includebuild.go].

[RFC 11]: bytecodealliance/rfcs#11
[includebuild.go]: https://github.com/bytecodealliance/wasmtime-go/blob/2f1b8a9d0/includebuild.go#L13-L17

Signed-off-by: Olivier Lemasle <o.lemasle@gmail.com>
srenatus pushed a commit to open-policy-agent/opa that referenced this pull request Jun 21, 2021
This version of wasmtime-go reflects new Wasmtime C API specified
by [RFC 11], and requires some changes in the way it is called.

Also, the imports that were present only to include the C headers and static
lib in `go vendor`ed dependencies are no longer required, because these paths
are now imported by wasmtime-go itself in [includebuild.go].

[RFC 11]: bytecodealliance/rfcs#11
[includebuild.go]: https://github.com/bytecodealliance/wasmtime-go/blob/2f1b8a9d0/includebuild.go#L13-L17

Signed-off-by: Olivier Lemasle <o.lemasle@gmail.com>
jeremywiebe added a commit to Khan/fastlike that referenced this pull request Aug 3, 2021
This upgrade includes a wasmtime-go fix for a linker bug on Linux where the pthread library wasn't linked and caused builds to fail on Linux.
The upgrade to wasmtime-go v0.29 also includes a migration to wasmtime's new C api (bytecodealliance/rfcs#11)
jeremywiebe added a commit to Khan/fastlike that referenced this pull request Aug 4, 2021
## Summary:

A[ linker issue](bytecodealliance/wasmtime-go#94) in wasmtime-go caused build failures in Linux environments. This was [fixed](bytecodealliance/wasmtime-go#95) and shipped in [v0.29](https://github.com/bytecodealliance/wasmtime-go/releases/tag/v0.29.0) and so we need to upgrade.

This upgrade includes a wasmtime-go fix for a linker bug on Linux where the pthread library wasn't linked and caused builds to fail on Linux.
The upgrade to wasmtime-go v0.29 also includes a migration to wasmtime's new C api (bytecodealliance/rfcs#11)

The changes were largely guided by the RFC and the [example code](https://github.com/bytecodealliance/wasmtime-go/blob/main/doc_test.go).

Issue: LP-10085

## Test plan:

`make test`

Author: jeremywiebe

Reviewers: jaredly

Required Reviewers: 

Approved by: jaredly

Checks: ✅ Run tests, ✅ Build wasm files for tests

Pull request URL: #4
X-leaf7 added a commit to X-leaf7/wasmtime-go that referenced this pull request Jun 24, 2024
* Update with Wasmtime's new C API

This commit updates this repository to use Wasmtime's new C API as
specified by [RFC 11]. This is a breaking change for this API and
notably requires that a `Storelike` object is now passed in for many
functions, serving as the `Store` context for that method. The
implementation of memory management, however, has been greatly
simplified, now that objects within a `Store` no longer need
destructors. This should improve performance and also help avoid
reclaiming memory only at odd times.

[RFC 11]: bytecodealliance/rfcs#11

* Try fixing bazel build

* Cancel in-flight jobs on CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.