Integrate NVIDIA's S/R implementation into HPX #6431
Draft
+5,621
−871
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
hkaiser
reviewed
Feb 3, 2024
isidorostsa
force-pushed
the
include_stdexec
branch
from
8826f04
to
634cb7b
Compare
isidorostsa
force-pushed
the
include_stdexec
branch
from
634cb7b
to
1798cbc
Compare
hkaiser
reviewed
Feb 12, 2024
isidorostsa
force-pushed
the
include_stdexec
branch
from
6ec03fa
to
5b5ac33
Compare
isidorostsa
force-pushed
the
include_stdexec
branch
from
5b5ac33
to
f34103e
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more |
- Completion signatures are a specialization of stdexec::completion_signatures<...> - Senders/Receivers need the is_sender/is_receiver tag - Sender_of<snd, tag> looks for completion signatues (e.g. tag = set_error(std::exception_ptr)) instead of looking for possible set_value(...) specializations
make_env<tag>(val, old_env)
The new calling convention for make_env is:
make_env(old_env, with(tag, val))
…not working. The rest of the tests pass.
isidorostsa
force-pushed
the
include_stdexec
branch
from
9c0318f
to
59b5170
Compare
isidorostsa
force-pushed
the
include_stdexec
branch
from
36bfaf7
to
39cee7b
Compare
hkaiser
reviewed
May 18, 2024
Comment on lines
+65
to
+74
| #ifdef HPX_HAVE_STDEXEC | ||
| auto scheduler = | ||
| hpx::execution::experimental::get_completion_scheduler< | ||
| hpx::execution::experimental::set_value_t>( | ||
| hpx::execution::experimental::get_env(u)); | ||
| #else | ||
| auto scheduler = | ||
| hpx::execution::experimental::get_completion_scheduler< | ||
| hpx::execution::experimental::set_value_t>(u); | ||
| #endif |
There was a problem hiding this comment.
I think we could factor out this difference allowing for the diferences to be very localized.
There was a problem hiding this comment.
We would have to alter our existing senders to provide environments, currently most of the times they do not.
There was a problem hiding this comment.
Really? What about:
inline constexpr struct get_completion_scheduler_ex_t
{
template <typename U>
decltype(auto) operator()(U&& u) const
{
#ifdef HPX_HAVE_STDEXEC
return hpx::execution::experimental::get_completion_scheduler<
hpx::execution::experimental::set_value_t>(
hpx::execution::experimental::get_env(HPX_FORWARD(U, u)));
#else
return hpx::execution::experimental::get_completion_scheduler<
hpx::execution::experimental::set_value_t>(HPX_FORWARD(U, u));
#endif
}
} get_completion_scheduler_ex{};
There was a problem hiding this comment.
Ah yes, good point. I will get on that, and I will migrate to using it after sorting out the build system.
libs/core/execution/include/hpx/execution/algorithms/as_sender.hpp
Outdated
Show resolved
Hide resolved
| static_assert(ex::is_sender_v<decltype(s), ex::empty_env>); | ||
| #endif |
There was a problem hiding this comment.
Shouldn't we rather implement is_sender_in_v for our code base?
There was a problem hiding this comment.
right, leaving that as a TODO to remove much of the duplication from the tests.
isidorostsa
force-pushed
the
include_stdexec
branch
from
1f90be4
to
210ddea
Compare
isidorostsa
force-pushed
the
include_stdexec
branch
from
1948e56
to
3dbcee6
Compare
|
Fetching STDEXEC by default on C++20, so the CI is testing both with and without it. |
isidorostsa
changed the title
hkaiser
reviewed
May 31, 2024
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
isidorostsa
force-pushed
the
include_stdexec
branch
from
2dd79a1
to
1a188f2
Compare
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 PR serves to replace the existing senders/receivers implementation in hpx with the stdexec implementation maintained by NVIDIA.
Big credits to the @pika-org team for their work when solving almost the same problem. This PR is built on top of much of that work.