Remove OS Thread from Atomics Wait Async#989
Conversation
komyg
changed the title
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Generally looks good to me, but I had a few nitpicks regarding the inline comments that I'd like to fix before merging :)
|
|
||
| // SAFETY: buffer is a cloned SharedDataBlock; non-dangling. | ||
| let waiters = unsafe { self.0.data_block.get_or_init_waiters() }; | ||
| // a. Perform EnterCriticalSection(WL). |
There was a problem hiding this comment.
nitpick (but blocking): I'd like to see here a reference to where these steps are coming from. Possibly also a link to the relevant spec method.
There was a problem hiding this comment.
So this is the implementation of the Job Abstract Closure for WaitAsyncTimeoutJob, when there is no timeout.
AFAIK the spec only defines the case with a timeout, but we have separated them, so I don't think that there is a specifc spec reference that I can add.
Therefore, I removed the comments referencing the steps.
| )); | ||
| } | ||
| } | ||
| // c. Perform LeaveCriticalSection(WL). |
There was a problem hiding this comment.
nitpick (blocking): Step b is missing. I assume that'd be removing the waiter from the list, but I'd want to see it.
| } | ||
|
|
||
| // SAFETY: buffer is a cloned SharedDataBlock; non-dangling. | ||
| let waiters = unsafe { self.0.data_block.get_or_init_waiters() }; |
There was a problem hiding this comment.
nitpick: This seems like it could be a helper method.
There was a problem hiding this comment.
Since this is an unsafe operation, IMO it would be better to have this inline, so that this fact is not hidden from the user by an abstraction.
komyg
force-pushed
the
fix/remove-thread-from-wait-async
branch
from
ffa98bd to
3f07edc
Compare
|
Hey @aapoalas, I've addressed your comments. Can you review again, please? |
2 participants
No description provided.