-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Support for WASI threads #98120
Support for WASI threads #98120
Conversation
@dotnet-policy-service agree company="SimRail S.A." |
message ("CMAKE_SYSTEM_NAME=${CMAKE_SYSTEM_NAME}") | ||
message ("CMAKE_SYSTEM_VARIANT=${CMAKE_SYSTEM_VARIANT}") | ||
|
||
set(CLR_CMAKE_HOST_OS ${CMAKE_SYSTEM_NAME}) | ||
string(TOLOWER ${CLR_CMAKE_HOST_OS} CLR_CMAKE_HOST_OS) | ||
set(CLR_CMAKE_HOST_VARIANT ${CMAKE_SYSTEM_VARIANT}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not super familiar with cmake, but it seems weird that the host variant is either threads or not threads depending on the target you're building for. don't all cmake build hosts have threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is referring not to build host but mono runtime host. (as opposed to target system for which runtime will do codegen)
Still, I'm not sure how that interacts with DISABLE_THREADS
(shouldn't there be TARGET/HOST_DISABLE_THREADS?), but in any case it was used this way for wasm before.
Tagging subscribers to 'arch-wasm': @lewing Issue Detailswasi-libc provides pthread implementation, so use that. This way it behaves almost like POSIX-like platform (it does not support signals). Unfortunately macrology does get pretty complicated, because it is WASM, but does not need to use most existing WASM support code but generic POSIX instead. wasi-sdk is updated to version 21 for WebAssembly/wasi-sdk@c891cd2 and WebAssembly/wasi-libc@ec4566b
Conditionals in csproj are patched to work correctly with WASI threads, but I'm not really sure whether When testing on wasmtime it needs bytecodealliance/wasmtime#7884
|
This is WASIX extensions, right ? What would happen when we upgrade to WASI preview 2 ? |
#elif defined(USE_PTHREAD_WASM_BACKEND) | ||
|
||
struct __pthread { | ||
struct pthread *self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty brittle, what if this structure changes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to get stack bounds otherwise, because wasi-libc doesn't have pthread_getattr_np.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses pthreads from wasi-libc, which is currently implemented using wasi-threads proposal (https://github.com/WebAssembly/wasi-threads) |
@Milek7 we appreciate your efforts! I would like to share some context to set your and our expectations. As you know, the wasm threading is still developing and the final implementation may be very different from what you are trying to achieve in this PR. Our view is that we want to have dotnet based on the standard component based WASI, rather than POSIX/WASIX extensions. On the other hand, as you pointed out, threads are not ready. We are willing to explore this, given that we all know this API would eventually go away. As you probably noticed, our big focus at the moment is to enable threads in the browser (with emscripten). Browser limitations as well as interop with JS requires that we work-around many concepts. Historically we only had Mono on emscripten and that combination was referred to as For context, in the browser major features which influence threads are:
We have following combinations ahead
What need to be done for WASI
Right now, we are not actively working on any of the above. Are you still interested ? |
My motivation is running existing C# codebase in wasm runtime. As you noted, single-threaded requires yielding to caller, which requires changes to both library code and applications (which may assume that threads exist, use locks, and blocking functions). Multi-threading though makes it fit into posix-like model, which enables library and application code to work without changes.
This PR uses wasi-threads proposal (as implemented in eg. wasmtime) and does not use wasmer WASIX extensions.
Indeed that's why I'm pursuing this approach. Admittedly I didn't do extensive testing yet, but this (Thread, ThreadPool, Task, etc.) should already work with this PR. Given that it seems component model multithreading won't be available for quite some time (I have seen statements that "The Component Model does not have support for threads at this time and that's not expected to be supported until the time scale of years in the future"), I think it is reasonable to merge this approach for time being. When it comes I think it is expected that wasi-libc will still provide pthread implementation requiring no changes here. Targeting component model directly bypassing libc would likely require some changes, though I'm not exactly sure why you would do this, considering that in practice it would probably mean copy-pasting musl code. (unless it would also bring Wasm GC, but that would be complete architecture change anyway). I can look into threading bugs if some will come up with more testing, and potentially untangle that |
I'm interested in more details, if you are willing to share them.
We can help enable WASI full library test suite for you on this PR. Is it ok for us to push into your branch ? There are also some failing CI legs on this PR, because we will need to bump SDK in the Helix VM images too.
We are discussing that with the team. TBH we don't know exactly what we want yet. Will keep you posted. |
Maybe the bump of the wasi sdk should be done in a separate PR ? |
Thanks, feel free to do it.
It's probably rather unconventional use-case :) Actually we are embedding it in desktop application, for various reasons it is convenient to have it compiled as WASM module (no extra processes, easy state cleanup for restart). PS: For testing with wasmtime it will need fresh build (for bytecodealliance/wasmtime#7884) |
Is there feed with nighty build ? runtime/eng/testing/wasi-provisioning.targets Lines 21 to 23 in 954ead3
|
The wasi sdk bump was merged. |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
I didn't forget about it and it's still on my TODO list. I plan to cleanup the |
In the meantime I encountered problem with WasmBuildNative, added fix for that. |
We don't have bandwidth to keep WASIp1 at the same time as we want to switch to WASIp2. It would require even more complex and conditional build scripts. I'm sorry. |
wasi-libc provides pthread implementation, so use that. This way it behaves almost like POSIX-like platform (it does not support signals). Unfortunately macrology does get pretty complicated, because it is WASM, but does not need to use most existing WASM support code but generic POSIX instead.
wasi-sdk is updated to version 21 for WebAssembly/wasi-sdk@c891cd2 and WebAssembly/wasi-libc@ec4566b
pthread_exit
does not exist, but returning fromstart_wrapper
seems to work fine, not sure when other ways of exiting a thread are invoked.Conditionals in csproj are patched to work correctly with WASI threads, but I'm not really sure whether
FeatureWasmManagedThreads
should be defined at all for this configuration. It's not a browser, has POSIX threads, so this whole complication with ifdefing unsupported browser attributes and adding special assemblies forWebAssembly.Threading
seems completely unnecessary. I don't know how to fix this cleanly, though.When testing on wasmtime it needs bytecodealliance/wasmtime#7884