Stream: git-wasmtime

Topic: wasmtime / PR #7940 Extend C API with interfaces needed t...


view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 18:53):

Milek7 opened PR #7940 from Milek7:mt-c-api to bytecodealliance:main:

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 define_sharedmemory in linker. Is this acceptable or I need to fight with Extern more to get it to work?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 18:53):

Milek7 requested alexcrichton for a review on PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 18:53):

Milek7 requested wasmtime-core-reviewers for a review on PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 18:59):

Milek7 updated PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 19:03):

Milek7 edited PR #7940:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 21:43):

pchickey commented on PR #7940:

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)(https://github.com/bytecodealliance/wasmtime/pull/7933), and which is not compatible with wasmtime-wasi-threads.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 21:43):

pchickey edited a comment on PR #7940:

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)(https://github.com/bytecodealliance/wasmtime/pull/7933), and which is not compatible with wasmtime-wasi-threads.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 21:56):

pchickey edited a comment on PR #7940:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 22:37):

alexcrichton commented on PR #7940:

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)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 14 2024 at 22:39):

Milek7 commented on PR #7940:

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 15 2024 at 01:03):

github-actions[bot] commented on PR #7940:

Subscribe to Label Action

cc @peterhuene

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

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Feb 15 2024 at 02:36):

abrown commented on PR #7940:

@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?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 15 2024 at 19:01):

alexcrichton commented on PR #7940:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 15 2024 at 19:56):

Milek7 updated PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 15 2024 at 23:58):

Milek7 commented on PR #7940:

Dropped wasictx functions.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 16:09):

alexcrichton commented on PR #7940:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 16:34):

Milek7 commented on PR #7940:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 16:57):

alexcrichton commented on PR #7940:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 00:48):

Milek7 updated PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 01:01):

Milek7 updated PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 01:33):

Milek7 updated PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 02:28):

Milek7 commented on PR #7940:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2024 at 17:34):

alexcrichton submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 01:35):

Milek7 updated PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 01:39):

alexcrichton has enabled auto merge for PR #7940.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 02:20):

alexcrichton merged PR #7940.


Last updated: Nov 22 2024 at 16:03 UTC