wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

Extend C API with interfaces needed to use threads

Open Milek7 opened this issue 1 year ago • 10 comments

I'm not sure how to handle SharedMemory here. Possibly it could use Extern interfaces, but I had some trouble with it. Extern seem to assume that it's associated with Store, but shared memories aren't. Instead I added dedicated wasmtime_linker_define_sharedmemory. Is this acceptable or I need to fight with Extern more to get it to work?

Milek7 avatar Feb 14 '24 18:02 Milek7

From the context here I'm not sure why the wasmtime_wasictx methods are required to use threads. I'm planning a renovation which will swap out the c api's wasi implementation from wasi-common to wasmtime-wasi::preview2, which will shortly be promoted to the root of wasmtime-wasi, and which is not compatible with wasmtime-wasi-threads.

pchickey avatar Feb 14 '24 21:02 pchickey

I think adding bits and functions to manage shared memories in the C API would be great to have, but I would agree with @pchickey that the WASI bits may want to be left out for now. I would echo his same sentiments in terms of it's not clear what to do with the implementation of WASI and threads in the wasmtime repo into the future and I think it would be best to not add new dependencies on something we're hoping to remove (aka wasi-common)

alexcrichton avatar Feb 14 '24 22:02 alexcrichton

wasictx constructor is needed to attach the same WASI context to multiple instances (threads). If preview2 does not support threads that's somewhat of a problem for my use-case :( I implement thread spawn function on C side instead of using wasmtime-wasi-threads, but I don't think that makes any difference

Milek7 avatar Feb 14 '24 22:02 Milek7

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions[bot] avatar Feb 15 '24 01:02 github-actions[bot]

@Milek7, you may be interested in https://github.com/WebAssembly/component-model/pull/291. Let's talk more tomorrow on Zulip; maybe you want to help out with implementing some of those bits?

abrown avatar Feb 15 '24 02:02 abrown

That makes sense that wasi contexts can't be shared today, but this API being added is exposing a contract where a wasi context can be attached to multiple stores which we may not wish to guarantee. For example thisi enables having one long-lived wasi context which is shared over its lifetime by many instances. While a perhaps interesting use case that's not necessarily something I think we want to commit to just yet.

alexcrichton avatar Feb 15 '24 19:02 alexcrichton

Dropped wasictx functions.

Milek7 avatar Feb 15 '24 23:02 Milek7

Ok, thanks! Can you detail more how come this couldn't be folded into wasmtime_extern_t? For example with this support you can't extract a shared memory export or inspect the import/export types for example.

alexcrichton avatar Feb 16 '24 16:02 alexcrichton

I was having some trouble with it, but looking at it now I might have confused wasmtime_extern_t with wasm_extern_t, ouch. I will take another look next week.

Milek7 avatar Feb 16 '24 16:02 Milek7

Ah ok makes sense! In that case yeah I'd recommend trying to get this integrated with the wasmtime_* variant. If that has issues though I can try to help work through them as they come up

alexcrichton avatar Feb 16 '24 16:02 alexcrichton

Done

Milek7 avatar Mar 02 '24 02:03 Milek7