Fix std::future status query #596
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
1 similar comment
|
/ok to test |
|
/ok to test |
|
Discussing further in #593 rather than here. |
aff60d5 to
caa1e6a
Compare
|
/ok to test |
|
/ok to test |
1 similar comment
|
/ok to test |
There was a problem hiding this comment.
Looks good @kingcrimsontianyu. The only question is if the overhead of creating the extra gather task is an issue. I suspect not, but it would be good to get it confirmed.
cc. @GregoryKimball, @vuule.
| /** | ||
| * @brief Submit the move-only task callable to the underlying thread pool. | ||
| * | ||
| * @tparam F Callable type. F shall be move-only and have no argument. |
There was a problem hiding this comment.
It is tempting to add the "can't copy" part to the code to improve type safety:
template <typename F, std::enable_if_t<!std::is_copy_constructible_v<F>, bool> = true>
std::future<std::size_t> submit_move_only_task(F op_move_only)But this does not work.
For the non-copyable gather_tasks below, trying to copy construct results in compile error, but the type trait gives unexpected result.
auto gather_tasks_copy = gather_tasks; // Compile error as expected
static_assert(std::is_copy_constructible_v<decltype(gather_tasks)>); // No compile error. Surprise.A simpler example:
std::vector<std::future<int>> move_only;
static_assert(std::is_copy_constructible_v<decltype(move_only)>); // No compile error. Surprise.There was a problem hiding this comment.
This is a good writeup: https://quuxplusone.github.io/blog/2020/02/05/vector-is-copyable-except-when-its-not/
41afba6 to
94c266e
Compare
|
I think this looks good overall to me now. Is it ready for a thorough review? I'm not sure if it is intentionally still in draft or not. |
|
Thanks for the reminder. Other than pending performance assessment, this PR should be ready for review. |
|
I'll use cuDF bench for performance assessment, and post the results here when ready. |
cpp/include/kvikio/utils.hpp
Outdated
| @@ -152,6 +153,7 @@ std::tuple<void*, std::size_t, std::size_t> get_alloc_info(void const* devPtr, | |||
| template <typename T> | |||
| bool is_future_done(T const& future) | |||
| { | |||
| assert(future.valid()); | |||
There was a problem hiding this comment.
Since this is a public API, do we want to throw if this condition is violated or are we happy with a debug assertion?
There was a problem hiding this comment.
Thanks! I think we do need to check the pre-condition to avoid UB. Done.
| return detail::posix_device_read(_fd_direct_off, buf, size, file_offset, 0); | ||
| }; | ||
| return std::async(std::launch::deferred, task); | ||
| PushAndPopContext c(ctx); |
There was a problem hiding this comment.
Maybe add a note that this execution model provides API compatibility with the rest of the future-based APIs in the library while also ensuring that the future is actually immediately available since we don't want this call to be asynchronous.
There was a problem hiding this comment.
Thanks. Done.
I realized that the C++ concurrency TS has a utility function meant to do the exact same thing, so I put a simplistic implementation there in utils.hpp:
kvikio/cpp/include/kvikio/utils.hpp
Line 165 in b519cc2
| /** | ||
| * @brief Submit the move-only task callable to the underlying thread pool. | ||
| * | ||
| * @tparam F Callable type. F shall be move-only and have no argument. |
There was a problem hiding this comment.
This is a good writeup: https://quuxplusone.github.io/blog/2020/02/05/vector-is-copyable-except-when-its-not/
94c266e to
b519cc2
Compare
b519cc2 to
1691bd0
Compare
Pending performance regression check!!
This PR fixes the
std::futurestatus query according to the discussion in #593 .std::asyncwith the deferred launch policy. Doing so would cause the future status to be falsely reported as done. Instead, as suggested, they are created by the underlying thread pool (more specifically by the promise objects). This enables non-blocking future status query.std::asyncwith the deferred launch policy.