WASIp3 Cooperative Threading#799
Conversation
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
An initial pass over things.
One major piece remaining in terms of synchronization is locks around the descriptor_table.h and derivative subsystems. None of that is locking-aware right now and would probably cause lots of problems if two threads tried to print to stdout for example. That's fine for an initial PR but we'll want to avoid broadcasting that threads are generally available until those issues are sorted out
| if (at) { | ||
| errno = ENOSYS; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Would it make sense to thread at into __waitlist_wait_on and have the error here generated by that function? That way there's sort of a centralized "this isn't supported" as opposed to having to make sure to write this down in all functions with a timeout parameter
| // TODO(wasip3) timed waits | ||
| if (ts) { | ||
| return ETIMEDOUT; | ||
| } |
There was a problem hiding this comment.
This one might be difficult to leave unsupported into the future as AFAIK waiting with timeouts on condvars is a relatively common operation. Leaving this as a TODO for now though is fine by me
There was a problem hiding this comment.
This I think is ok for an initial implementation, but I think we're going to want to eventually ensure that calls to __wasm_get_stack_pointer go directly to the builtin intrinsic without needing a wrapper in the middle. Otherwise there's basically no way to create a module that directly performs the call because even with LTO these won't get inlined.
| return thrd_busy; | ||
| } | ||
| } | ||
| int ret = __pthread_mutex_trylock((pthread_mutex_t *)m); |
There was a problem hiding this comment.
Would it make sense to have some sort of internal macro/header to do this cast to avoid needing to open-code it having the same representation?
|
Another high level thing I'm just now remembering -- we'll need to afford a transition period from today-without-coop-threads to tomorrow-with-coop-threads-stable. Basically I think we'll want some sort of CMake flag for "ok turn on threads" which is default off for wasip3. That way the default build of wasi-sdk, for example, will have a libc.a which has pthread symbols but the symbols don't actually do anything (return errors as they do today). Once the I'd like to ideally still only have one target, Does that sound ok to you @TartanLlama? If so I was thinking |
|
Yeah, I think the additional CMake option makes sense |
2 participants
Currently a draft while the LLVM patches merge and I get all of this ready for review