WASI threads, implementation of wasm32-wasi-preview1-threads target - #112922
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
|
These commits modify compiler targets. |
This comment has been minimized.
This comment has been minimized.
alexcrichton
left a comment
There was a problem hiding this comment.
IIRC this won't get much testing on Rust CI (unsure if things have changed since I last looked), so it might be worth running tests locally not only for this target but also for the wasm32-wasi target if you have some time (not perhaps the ui test suite but various programs locally)
Is it too big or why not? Just checked that threads pass 51/65 from tests/ui/threads-sendsync, 5 tests doesn't compile and 9 fails. |
alexcrichton
left a comment
There was a problem hiding this comment.
Is it too big or why not?
Mostly in that I don't think it's regularly run with the WASI targets right now so it's not as simple as "run this and make sure it passes", you'd have to do the legwork of running it and figuring out if failures are relevant or not.
@alexcrichton Would it be enough of testing or do you know any open-source rust projects that are already getting compiled to |
|
Should this be blocked on #110596? (See rust-lang/compiler-team#607 for context) |
commented
Jun 30, 2023
|
Just a note regarding CI testing. Note sure if you have already found out by yourself, but a few details I found out for my own tests:
I am in a similar situation: I just added native wasm exception handling support to the rust compiler; I'd like to add a run-make test for it, but the nodejs version is too old and does not support wasm exceptions yet. I will add a PR for this, though. Maybe it goes through :) |
commented
Jun 30, 2023
Seems reasonable to me, yeah. I originally commented to this effect to recommend testing the non-threads wasm32-wasi target since the original version of this PR I think would have broken that. I'm more confident (but not 100%) that the current PR won't break existing code, but it might be good to spot-check a few things as well. |
commented
Jul 4, 2023
@mirkootter thanks |
commented
Jul 4, 2023
@alexcrichton |
commented
Jul 4, 2023
@abrown @bjorn3 @alexcrichton is there any concern if I just rename it to |
commented
Jul 5, 2023
|
As for testing, I think that's now up to whomever approves this PR. It sounds like not a lot of testing has been done, and AFAIK this won't be tested on CI. As for naming, I don't have a preference myself. I believe the name |
commented
Jul 5, 2023
|
|
commented
Jul 11, 2023
Ah okay, I thought it's gonna be you @alexcrichton, @bjorn3 or @abrown.
Sort of, I've done some testing but I haven't found a good open source Rust project compiled to wasm32-wasi to test it. I tried https://github.com/bytecodealliance/wizer/tree/main today and it was fine, but there wasn't much to test. I have an idea to compare compilation and run results for
Is |
commented
Jul 11, 2023
|
unsolicited and probably undesired 2p, but adding it to prevent this being merged without outside perspective This should not be named preview1 because wasi-threads is not a part of preview1. It isn't even phase 3. If anything, this should be clear that this is not a finished feature and not about preview1. Calling this |
|
Hey guys, so I got this fully working here already using the WASI threads ABI (not the WASIX ABI) It's quite well tested now (many users for over a year) and its been running in production - namely the big drama being how to pass through the stacks so they can unwind. Not directly needed for threading but important for other patterns (such as Note: that it uses the triple I think that we should work a bit more closely together on this, one thing that definitely makes sense is that we align the triplets as otherwise this will get messy quite quick. Also while not directly for this PR... this might be a good time to start thinking about how we can make the other POSIX WASM capabilities eventually land upstream behind appropriate Cheers |
That's not right, Footnotes
|
commented
Jul 31, 2023
|
@bors r+ |
commented
Jul 31, 2023
commented
Aug 2, 2023
commented
Aug 2, 2023
|
☀️ Test successful - checks-actions |
commented
Aug 2, 2023
|
Finished benchmarking commit (aa8462b): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
commented
Aug 8, 2023
|
@rustbot label +perf-regression-triaged This appears to be noise rather than genuine delta. |
This PR adds a target proposed in rust-lang/compiler-team#574 by @abrown and implementation of
std::thread::spawnfor the targetwasm32-wasi-preview1-threadsTier 3 Target Policy
As tier 3 targets, the new targets are required to adhere to the tier 3 target policy requirements. This section quotes each requirement in entirety and describes how they are met.
See src/doc/rustc/src/platform-support/wasm32-wasi-preview1-threads.md.
The target is using the same name for $ARCH=wasm32 and $OS=wasi as existing Rust targets. The suffix
preview1introduced to accurately set expectations because eventually this target will be deprecated and follows MCP 607. The suffixthreadsindicates that it’s an extension that enables threads to the existing target and it follows MCP 574 which describes the rationale behind introducing a separate target.This PR does not introduce any new dependency.
The new target doesn’t support building host tools.
The full standard library is available for this target as it’s an extension to an existing target that has already supported it.
Only manual test running is supported at the moment with some tweaks in the test runner codebase. For build and running tests see src/doc/rustc/src/platform-support/wasm32-wasi-preview1-threads.md.
I acknowledge these requirements and intend to ensure they are met.