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

Extend C API with interfaces needed to use threads #7940

Merged
merged 1 commit into from Mar 8, 2024

Conversation

Milek7
Copy link
Contributor

@Milek7 Milek7 commented Feb 14, 2024

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 Milek7 requested a review from a team as a code owner February 14, 2024 18:53
@Milek7 Milek7 requested review from alexcrichton and removed request for a team February 14, 2024 18:53
@pchickey
Copy link
Collaborator

pchickey commented Feb 14, 2024

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.

@alexcrichton
Copy link
Member

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)

@Milek7
Copy link
Contributor Author

Milek7 commented Feb 14, 2024

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

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Feb 14, 2024
Copy link

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.

@abrown
Copy link
Collaborator

abrown commented Feb 15, 2024

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

@alexcrichton
Copy link
Member

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.

@Milek7
Copy link
Contributor Author

Milek7 commented Feb 15, 2024

Dropped wasictx functions.

@alexcrichton
Copy link
Member

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.

@Milek7
Copy link
Contributor Author

Milek7 commented Feb 16, 2024

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.

@alexcrichton
Copy link
Member

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

@Milek7 Milek7 force-pushed the mt-c-api branch 3 times, most recently from ad4379d to d951a8e Compare March 2, 2024 01:33
@Milek7
Copy link
Contributor Author

Milek7 commented Mar 2, 2024

Done

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexcrichton alexcrichton added this pull request to the merge queue Mar 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 7, 2024
@abrown abrown mentioned this pull request Mar 7, 2024
10 tasks
@alexcrichton alexcrichton added this pull request to the merge queue Mar 8, 2024
Merged via the queue into bytecodealliance:main with commit 55bd797 Mar 8, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants