Rewrite wait/notify with wasm threads #7629
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit rewrites and refactors the `ParkingSpot` implementation in Wasmtime. This is motivated by bytecodealliance#7623 primarily which is something I haven't been able to reproduce but it doesn't look like a spurious issue. Additionally in reviewing the previous implementation I'm not sure if it was technically spec-compliant. Previously each parking spot was modeled with a single condition variable. All threads blocking on the same address would block on the same condition variable. When waking up N threads the condition variable would either use `notify_all` or `notify_one` N times. The part I wasn't so sure about is that each thread, when waking up, would "consume" a wakeup notification on the way out, going back to sleep if a notification wasn't available. This was intended to handle spurious wakeups from the OS condition variable in use. This could mean, however, that a spurious wakeup of one thread could consume a notification from another thread. This was documented already as a possibility and "probably ok" but my gut is that this behavior led to bytecodealliance#7623, although I haven't been able to construct a trace that would lead the test here to deadlock. Out of caution, however, this commit rewrites the implementation to be similar to what V8 and SpiderMonkey are doing. Both of those implementations are using a linked list of waiters for threads that are blocked and then notifying N threads dequeues N threads to wake them up. This makes the semantics of knowing which thread is waken up easier to understand from an implementation point of view since each wakeup notification deterministically goes to a specified thread. The tricky part about this implementation is that a doubly-linked-list needs to be maintained within `ParkingSpot` to handle this. While I was here I additionally refactored the interface of `ParkingSpot` to more closely match the raw interface of WebAssembly. This is intended to scope the problem being solved more narrowly to what wasm needs rather than trying to solve a more general problem at the same time. Closes bytecodealliance#7623
pchickey
approved these changes
Dec 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit rewrites and refactors the
ParkingSpotimplementation in Wasmtime. This is motivated by #7623 primarily which is something I haven't been able to reproduce but it doesn't look like a spurious issue. Additionally in reviewing the previous implementation I'm not sure if it was technically spec-compliant.Previously each parking spot was modeled with a single condition variable. All threads blocking on the same address would block on the same condition variable. When waking up N threads the condition variable would either use
notify_allornotify_oneN times. The part I wasn't so sure about is that each thread, when waking up, would "consume" a wakeup notification on the way out, going back to sleep if a notification wasn't available. This was intended to handle spurious wakeups from the OS condition variable in use. This could mean, however, that a spurious wakeup of one thread could consume a notification from another thread. This was documented already as a possibility and "probably ok" but my gut is that this behavior led to #7623, although I haven't been able to construct a trace that would lead the test here to deadlock.Out of caution, however, this commit rewrites the implementation to be similar to what V8 and SpiderMonkey are doing. Both of those implementations are using a linked list of waiters for threads that are blocked and then notifying N threads dequeues N threads to wake them up. This makes the semantics of knowing which thread is waken up easier to understand from an implementation point of view since each wakeup notification deterministically goes to a specified thread. The tricky part about this implementation is that a doubly-linked-list needs to be maintained within
ParkingSpotto handle this.While I was here I additionally refactored the interface of
ParkingSpotto more closely match the raw interface of WebAssembly. This is intended to scope the problem being solved more narrowly to what wasm needs rather than trying to solve a more general problem at the same time.Closes #7623