Do not move arrival_token in barrier::try_wait_* #283
Conversation
Take a mutable reference instead. Moving the token into try_wait makes no sense. The caller has to be able to retry the try wait with the same token. For more details and C++ spec, see: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2643r1.html
ahendriksen
requested review from
ericniebler and
griwes
and removed request for
a team
|
Pull requests from external contributors require approval from a |
|
This PR is a redo of NVIDIA/libcudacxx#499 Related issue: #280 Also: I changed the signature of |
| bool try_wait_for(arrival_token & __token, const _CUDA_VSTD::chrono::duration<_Rep, _Period>& __dur) { | ||
| auto __nanosec = _CUDA_VSTD::chrono::duration_cast<_CUDA_VSTD::chrono::nanoseconds>(__dur); | ||
|
|
||
| return __try_wait(_CUDA_VSTD::move(__token), __nanosec); |
There was a problem hiding this comment.
You are still moving here, is that intentional?
Applies below
There was a problem hiding this comment.
It is intentional in the sense that I tried to make as few changes as possible. I did not change the internal APIs from && to &. One thing I had not considered is that arrival_token might actually invalidate the original location on move. Does it?
There was a problem hiding this comment.
I think this needs to update the internal APIs as well.
| template<class _Rep, class _Period> | ||
| _LIBCUDACXX_NODISCARD_ATTRIBUTE _LIBCUDACXX_INLINE_VISIBILITY | ||
| bool try_wait_for(arrival_token && __token, const _CUDA_VSTD::chrono::duration<_Rep, _Period>& __dur) { | ||
| bool try_wait_for(arrival_token & __token, const _CUDA_VSTD::chrono::duration<_Rep, _Period>& __dur) { |
There was a problem hiding this comment.
Can we add a test that would have caught this? e.g., a try_wait_for that fails and retries.
There was a problem hiding this comment.
The current tests for this are a little janky:
It's moving from a moved-from object in a loop.
There was a problem hiding this comment.
This may not actually be possible to do without adding some runtime state tracking to track a moved-from token. So it may not be worth it.
There was a problem hiding this comment.
Main thing is to just remove the std::move which would catch not needing to pass an r-value ref.
|
/ok to test |
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
This needs to update the internal APIs we should not require arrival_token to be a trivial type
4 participants
Take a mutable reference instead.
Moving the token into try_wait makes no sense. The caller has to be able to retry the try_wait with the same token.
For more details and C++ spec, see:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2643r1.html