Stream: git-wasmtime

Topic: wasmtime / issue #7551 Reconciling the removal of wasi-co...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2023 at 17:17):

alexcrichton opened issue #7551:

As part of the effort to implement the upcoming 0.2.0 version of WASI the previous implementation of WASI in Wasmtime, the wasi-common crate, was rewritten and is now located in the wasmtime-wasi crate in the preview2 submodule. This means that Wasmtime (and the wasmtime CLI) now has two entirely separate implementations of the preview1 snapshot of WASI. This is not something I think we should retain indefinitely, so I think it's worthwhile to plan for the removal of the old wasi-common crate.

The only technical blocker I'm aware of for this is that the preview2-based implementation of wasi_snapshot_preview1 does not support WASI threads. Effectively if a thread is spawned then all WASI functions will stop working because the WASI context is no longer uniquely owned. More-or-less this boils down to this panic which would be dynamically triggered. There is no way right now to share the WASI context amongst threads and have functions continue to work (e.g. poll working concurrently on multiple threads).

There's definitely a lot of issues on the surface that look like incompatibilities, such as liberal usage of &mut Table and &mut self, but I think much of this can be updated with relative ease to work with &Table that has some internal synchronization which gives temporary access to &mut self on various objects. The main thing I don't know how to solve at this time, however, is how blocking works.

Currently all blocking computations in Wasmtime's implementation of WASI are represented as a Future. This is chiefly done through the Subscribe trait:

#[async_trait::async_trait]
pub trait Subscribe: Send + Sync + 'static {
    async fn ready(&mut self);
}

The usage of &mut self here is problematic because the same I/O object could be subscribe to from multiple threads which means &mut exclusive access is not possible. More-or-less what theoretically needs to happen is to push the &self into this method to enable waiting for readiness on objects on multiple threads simultaenously. Each thread would create its own future in poll and then it'd get arbitrated internally. This is, however, a very large refactoring and departure from the current design, one that I don't think can easily be done and I think may have unknown consequences (e.g. more-than-expected overhead in the single-threaded use case).

This conclusion brings me to opening this issue for more discussion.

cc @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2023 at 00:34):

pchickey commented on issue #7551:

My opinion is that, since wasi-threads is not compatible with the component model and is currently being set aside in order to investigate core wasm primitives, we should not port wasi-threads to the new wasmtime-wasi implementation, and unship support for wasi-threads when we retire wasi-common.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2023 at 01:20):

abrown commented on issue #7551:

I think we should keep wasi-threads around at least until the next thing is available: it would be great to be able to compare and contrast, e.g., for measuring spawn performance or for troubleshooting bugs. I am hoping to work on whatever is next soon but am waiting for some things to be sorted out on the spec side of things; so it's not clear to me when that switch over will be.

Some thoughts:

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2023 at 02:01):

pchickey commented on issue #7551:

do we have to remove wasi-threads? I'm not sure I completely understand why removing wasi-common requires removing wasi-threads. Is it just the dependency? Can't wasi-threads still work for non-component modules?

wasmtime-wasi::preview2 contains a complete replacement for wasi-common in every regard except for supporting wasi-threads, so we'd like to retire wasi-common and remove it from the tree.

aren't we going to have to deal with &mut self to &self issues at some point with the next thing? The next thing will still need to access these futures from multiple threads, no? Maybe I just need to dig into this more to understand better, but I'm wondering out loud if this problem won't go away so we should just deal with it now.

I don't want to make architectural decisions about wasmtime-wasi::preview2 based on an expectation of how some engineering work in the future might work out, but instead based on how it does work out, because that architectural change could be very costly in terms of both engineering time, complexity of this new implementation, and performance of the single-threaded use case.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2023 at 02:02):

pchickey edited a comment on issue #7551:

do we have to remove wasi-threads? I'm not sure I completely understand why removing wasi-common requires removing wasi-threads. Is it just the dependency? Can't wasi-threads still work for non-component modules?

wasmtime-wasi::preview2 contains a complete replacement for wasi-common in every regard except for supporting wasi-threads, so we'd like to retire wasi-common and remove it from the tree.

aren't we going to have to deal with &mut self to &self issues at some point with the next thing? The next thing will still need to access these futures from multiple threads, no? Maybe I just need to dig into this more to understand better, but I'm wondering out loud if this problem won't go away so we should just deal with it now.

I don't want to make architectural decisions about wasmtime-wasi::preview2 based on an expectation of how some engineering work in the future might work out, but instead based on how it does work out, because that architectural change could be very costly in terms of both engineering time, complexity of this new implementation, and performance of the single-threaded use case. I do not believe we have enough information now to make this choice.

Last updated: Nov 22 2024 at 17:03 UTC