Stream: git-wasmtime

Topic: wasmtime / issue #5235 wasi: provide thread-safe implemen...

Wasmtime GitHub notifications bot (Nov 09 2022 at 19:26):

abrown opened issue #5235:

With the presence of shared memories and, eventually, with wasi-threads providing a way to spawn multiple Wasm threads, access to the WASI state will no longer be single-threaded. This state (e.g., WasiCtx for wasi-common) must be appropriately protected from concurrent access, e.g., by locking as @haraldh has attempted here.

What complicates this issue is that I expect that not all users of Wasmtime will want to pay the performance penalty for locking access to WASI. This means that we may need two implementations for each WASI state object, ideally factored so that all the meaningful bits are kept in one place. Also, there may be some complications when wiring up either the single-threaded or thread-safe implementations of each WASI context during linking; presumably the thread-safe version should be used when the threads feature is enabled, but it is unclear if all of the Host types will be agreeable with this.

Wasmtime GitHub notifications bot (Nov 09 2022 at 20:10):

alexcrichton labeled issue #5235:

With the presence of shared memories and, eventually, with wasi-threads providing a way to spawn multiple Wasm threads, access to the WASI state will no longer be single-threaded. This state (e.g., WasiCtx for wasi-common) must be appropriately protected from concurrent access, e.g., by locking as @haraldh has attempted here.

What complicates this issue is that I expect that not all users of Wasmtime will want to pay the performance penalty for locking access to WASI. This means that we may need two implementations for each WASI state object, ideally factored so that all the meaningful bits are kept in one place. Also, there may be some complications when wiring up either the single-threaded or thread-safe implementations of each WASI context during linking; presumably the thread-safe version should be used when the threads feature is enabled, but it is unclear if all of the Host types will be agreeable with this.

Wasmtime GitHub notifications bot (Nov 15 2022 at 02:30):

alexcrichton commented on issue #5235:

One improvement that I think would be good to have is to have a method on GuestPtr<'_, str> to go to:

enum GuestCowStr<'a> {
    Borrowed(GuestStr<'a>),
    Owned(String),
 }

impl Deref for GuestString<'_> {
    type Target = str;
    // ...
}

And that way all bindings in all places could create this GuestCowStr<'a> and it would work with shared and unshared memories. For unshared memories this will retain borrows and for shared memories it's required to copy bytes out no matter what and this helps encapsulate that.

Lists are harder, however, and I believe will require case-by-case handling.

Wasmtime GitHub notifications bot (Nov 15 2022 at 17:20):

alexcrichton edited a comment on issue #5235:

One improvement that I think would be good to have is to have a method on GuestPtr<'_, str> to go to:

enum GuestCowStr<'a> {
    Borrowed(GuestStr<'a>),
    Owned(String),
 }

impl Deref for GuestCowStr<'_> {
    type Target = str;
    // ...
}

And that way all bindings in all places could create this GuestCowStr<'a> and it would work with shared and unshared memories. For unshared memories this will retain borrows and for shared memories it's required to copy bytes out no matter what and this helps encapsulate that.

Lists are harder, however, and I believe will require case-by-case handling.

Wasmtime GitHub notifications bot (Nov 21 2022 at 22:15):

abrown commented on issue #5235:

The last time we talked about this, there were two "roadblocks" to starting to work on this issue:

1. _Benchmarking_: adding RwLocks (e.g.) to various fields of the WASI context structures could add some locking overhead for each WASI call. @alexcrichton hypothesized that this overhead would be minimal compared to the overhead already imposed by Wiggle. We decided to benchmark a locking implementation of wasi-common in order to decide if we can apply locking unconditionally or if we have to (undesirably) maintain two implementations of each WASI context. Now that #5274 and #5309 are merged, we can actually measure this: use cargo bench wasi/ to run these new benchmarks first with the current HEAD and subsequently with a locking implementation.
2. _Async_: wasi-common is designed to support both asynchronous and synchronous calls; we decided when we met that we shouldn't allow async and wasi-threads to mix. In my mind, this involved some (perhaps major) refactoring to wasi-common; @haraldh had some questions along this lines IIRC. We had discussed a separate issue to discuss how the design of this refactoring to wasi-common might look, but now I think it could be discussed here, as a part of this issue. I would summarize this as: "wasi-threads and async cannot be enabled at the same time." This has ramifications for what is allowed in the embedding API (e.g., should an async call fail with an error when the threads feature is enabled?).

Wasmtime GitHub notifications bot (Nov 21 2022 at 22:16):

abrown edited a comment on issue #5235:

The last time we talked about this, there were two "roadblocks" to starting to work on this issue:

1. _Benchmarking_: adding RwLocks (e.g.) to various fields of the WASI context structures could add some locking overhead for each WASI call. @alexcrichton hypothesized that this overhead would be minimal compared to the overhead already imposed by Wiggle. We decided to benchmark a locking implementation of wasi-common in order to decide if we can apply locking unconditionally or if we have to (undesirably) maintain two implementations of each WASI context. Now that #5274 and #5309 are merged, we can actually measure this: use cargo bench wasi/ to run these new benchmarks first with the current HEAD and subsequently with a locking implementation. This roadblock should be gone now.
2. _Async_: wasi-common is designed to support both asynchronous and synchronous calls; we decided when we met that we shouldn't allow async and wasi-threads to mix. In my mind, this involved some (perhaps major) refactoring to wasi-common; @haraldh had some questions along this lines IIRC. We had discussed a separate issue to talk about what the design of this refactoring to wasi-common might look like, but now I think it could be discussed here, as a part of this issue. I would summarize this remaining roadblock as: "wasi-threads and async cannot be enabled at the same time." This has ramifications for what is allowed in the embedding API (e.g., should an async call fail with an error when the threads feature is enabled?).

Wasmtime GitHub notifications bot (Nov 21 2022 at 22:55):

alexcrichton commented on issue #5235:

To clarify what I meant a bit about async/threads, my intention is to avoid the need for any large refactoring ideally. At a Config level we can implement a simplistic check that verifies wasm_threads and async_support are not both enabled. At the runtime layer for wiggle/wasi-common the goal would be "what's the smallest delta to support threads while not disturbing everything that's already there too much". I don't know the scale of the changes necessary, though, and I imagine that the implementation of RwLock and etc could have a large impact. In any case though I mostly wanted to outline that my reasoning is to avoid the need for refactorings unless necessary, I'm not trying to add undue work -- but at the same time recognizing that wiggle/wasi-common are used in production today and shouldn't regress from where they are.

Wasmtime GitHub notifications bot (Feb 14 2023 at 21:44):

alexcrichton commented on issue #5235:

I think this is done now, right @abrown?

Wasmtime GitHub notifications bot (Feb 14 2023 at 23:02):

abrown commented on issue #5235:

It's done for wasi-common but I haven't yet propagated it to wasi-nn and wasi-crypto. Should I create separate issues for those and close this one?

Wasmtime GitHub notifications bot (Feb 15 2023 at 15:08):

alexcrichton commented on issue #5235:

Ah good point! Nah keeping this is fine.

Last updated: Nov 22 2024 at 16:03 UTC