details: noexcept specification for transform sender#396
Conversation
| struct transform_sender_for<Kokkos::Experimental::parallel_for_t, Env> { | ||
| template <typename Data, execution_space_completing_sender<Env> Sndr> | ||
| auto operator()(Kokkos::Experimental::parallel_for_t, Data&& data, Sndr&& sndr) const noexcept { | ||
| auto operator()(Kokkos::Experimental::parallel_for_t, Data&& data, Sndr&& sndr) const |
There was a problem hiding this comment.
What about:
template <typename Env>
struct transform_sender_for<Kokkos::Experimental::parallel_for_t, Env> {
private:
template <typename Data, typename Sndr>
auto impl(Data&& data, Sndr&& sndr) const {
auto [label, functor, policy] = std::forward<Data>(data);
using functor_t = decltype(functor);
using policy_t = decltype(policy);
auto schd = stdexec::get_completion_scheduler<stdexec::set_value_t>(
stdexec::get_env(sndr), env_);
policy_t policy_updated(Kokkos::Impl::PolicyUpdate{}, std::move(policy), schd.state->exec);
return ParallelForSender<Sndr, functor_t, policy_t>{
{{std::move(label), std::move(functor), std::move(policy_updated)}},
std::forward<Sndr>(sndr)};
}
public:
template <typename Data, execution_space_completing_sender<Env> Sndr>
auto operator()(Kokkos::Experimental::parallel_for_t, Data&& data, Sndr&& sndr) const
noexcept(noexcept(impl(std::forward<Data>(data), std::forward<Sndr>(sndr)))) {
return impl(std::forward<Data>(data), std::forward<Sndr>(sndr));
}
const Env& env_; // NOLINT(cppcoreguidelines-avoid-const-or-ref-data-members)
};c3486ed to
9e95eeb
Compare
| struct transform_sender_for<Kokkos::Experimental::parallel_for_t> { | ||
| template <typename Env, typename Data, execution_space_completing_sender<Env> Sndr> | ||
| auto operator()(const Env& env, Kokkos::Experimental::parallel_for_t, Data&& data, Sndr&& sndr) const noexcept { | ||
| auto operator()(const Env& env, Kokkos::Experimental::parallel_for_t, Data&& data, Sndr&& sndr) const |
There was a problem hiding this comment.
I think this version still isn't bringing anything new on the table.
I find it ever worse in terms of:
- readability
- contract
I think you should either drop the noexcept, or make it unconditional with static_assert inside. Any other attempt will make the noexcept look weird w.r.t. the body, so any develop would look at it suspiciously and want to audit it.
How about:
template <>
struct transform_sender_for<Kokkos::Experimental::parallel_for_t> {
template <typename Env, typename Data, execution_space_completing_sender<Env> Sndr>
auto operator()(const Env& env, Kokkos::Experimental::parallel_for_t, Data&& data, Sndr&& sndr) const noexcept {
auto [label, functor, policy] = std::forward<Data>(data);
using policy_t = decltype(policy);
using functor_t = decltype(functor);
using closure_t = ParallelForClosure<functor_t, policy_t>;
using sender_t = ParallelForSender<Sndr, functor_t, policy_t>;
static_assert(std::is_nothrow_move_constructible_v<policy_t>,
"parallel_for_t transform: policy must be nothrow move constructible");
static_assert(std::is_nothrow_constructible_v<closure_t, std::string, functor_t, policy_t>,
"parallel_for_t transform: closure must be nothrow constructible from its members");
static_assert(std::is_nothrow_constructible_v<sender_t, closure_t, Sndr>,
"parallel_for_t transform: ParallelForSender must be nothrow constructible");
auto schd = stdexec::get_completion_scheduler<stdexec::set_value_t>(stdexec::get_env(sndr), env);
auto new_policy = policy_t(Kokkos::Impl::PolicyUpdate{}, std::move(policy), schd.state->exec);
auto clsr = closure_t{{std::move(label), std::move(functor), std::move(new_policy)}};
return sender_t{std::move(clsr), std::forward<Sndr>(sndr)};
}
};Basically, you mandate that everything you build must not throw, and you add stat_assert in the body to ensure that contract is respected.
Side question: what's the benefit of nothrow connectability ? Worth added to the documentation somewhere probably.
72e70ab to
863a6d3
Compare
f582cdb to
d8e085b
Compare
61d597c to
9bf7d32
Compare
Extracted from: * #396 Signed-off-by: romintomasetti <romin.tomasetti@gmail.com>
bf491c4 to
d2be6a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive noexcept specifications to transform_sender and connect functions for sender/receiver operations, ensuring proper exception handling guarantees in the stdexec framework integration. The changes align with standard C++ practices for expressing exception specifications and enable compile-time detection of potential exceptions.
Changes:
- Added noexcept specifications to
transform_senderandconnectfunctions using stdexec traits (__nothrow_applicable,__nothrow_connectable,__nothrow_decay_copyable) - Refactored code to use type aliases for improved readability and correct noexcept expression evaluation
- Added tests to verify noexcept behavior with different view types (Kokkos::View vs std::span)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/stdexec/inline_scheduler_with_domain.hpp | Added noexcept specifications to transform_sender and connect; fixed requires clause to use Sndr&& instead of Sndr |
| tests/kokkos_ext/execution_space/test_parallel_for.cpp | Added compile-time tests for noexcept transformability with different view types |
| kokkos_ext/impl/execution_space/scoped_region.hpp | Added type aliases and noexcept specification to connect; updated get_completion_scheduler call to include receiver environment |
| kokkos_ext/impl/execution_space/schedule_from.hpp | Added type alias and noexcept specification to connect |
| kokkos_ext/impl/execution_space/parallel_for.hpp | Added noexcept specification to transform_sender operator; refactored with type aliases and helper function for policy update |
| kokkos_ext/impl/execution_space/continues_on.hpp | Added type alias and noexcept specification to connect; moved member declaration |
| kokkos_ext/impl/execution_space/bulk.hpp | Added noexcept specification to transform_sender operator; refactored with type aliases and helper function for policy construction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| template <stdexec::receiver Rcvr> | ||
| stdexec::operation_state auto connect(Rcvr rcvr) && noexcept( | ||
| std::is_nothrow_constructible_v< | ||
| rcvr_t<schd_t<::stdexec::env_of_t<Rcvr>>, Rcvr>, |
There was a problem hiding this comment.
How about you make schd_t take the receiver instead of the env ? It would shorten the whole thing.
There was a problem hiding this comment.
And then rcvr_t could also just take the receiver, and deduce the scheduler. Makes the whole thing really shorter.
| * despite not having a @c noexcept specification. | ||
| */ | ||
| template <Kokkos::ExecutionPolicy ExecPolicy> | ||
| static auto impl_policy_construct(const auto& exec, auto shape) noexcept { |
There was a problem hiding this comment.
Well, not all policies are no throw constructible. I'm thinking of the range polices on GPUs that need to check for block/grid size.
But I guess a failure in the construction of an execution policy is a hard failure (not sure it's easy or worth to recover from it), so marking noexcept will make it terminate and that's probably fine.
It's worth opening an issue on the Kokkos side and enhance a bit your docstring with my comment ;)
0cf16ca to
0944a14
Compare
0944a14 to
c644195
Compare
eb923cd to
d4959f4
Compare
| //! See https://github.com/NVIDIA/stdexec/blob/16076a81efa4477513e6ede9c2741fd034ecef99/include/stdexec/__detail/__bulk.hpp#L100. | ||
| template <typename Data> | ||
| concept has_parallel_policy = requires(const Data& data) { | ||
| concept has_parallel_policy = requires(const std::remove_cvref_t<Data>& data) { |
There was a problem hiding this comment.
Not sure about this one?
| return sndr_t<Sndr, Functor, Env>{ | ||
| {{std::string(std::format("{}: then", Kokkos::Impl::TypeInfo<execution_space<Sndr, Env>>::name())), | ||
| ThenWrapper{std::forward<Functor>(functor)}, | ||
| ThenWrapper<std::remove_cvref_t<Functor>>{.functor = std::forward<Functor>(functor)}, |
There was a problem hiding this comment.
Let's have a then_wrapper_t so that you have a single, centralized place where you can document why we need to remove the reference. BTW I'd do remove_ref_t only (?).
There was a problem hiding this comment.
Added a static assert to the then wrapper itself with a comment.
5891510 to
f347b79
Compare
Signed-off-by: Maarten Arnst <maarten.arnst@uliege.be>
5b26fe1 to
5a526e1
Compare
Extracted from: * #396 Signed-off-by: romintomasetti <romin.tomasetti@gmail.com>
Extracted from: * uliegecsm/graph-dispatching#396 Signed-off-by: romintomasetti <romin.tomasetti@gmail.com>
Extracted from
OpState#304