[P2300] Added fundamental coroutine_traits for S/R #5945

SAtacker · 2022-07-07T20:17:36Z

Signed-off-by: Shreyas Atre shreyasatre16@gmail.com

Proposed Changes

Awaiter traits
Awaitable traits
get_awaiter

Checklist

Awaiter traits tests
awaitable traits tests
get waiter tests

libs/core/execution_base/include/hpx/execution_base/traits/coroutine_traits.hpp

hkaiser

Very nice! Thanks!

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

- Change `class` to `typename` for consistency - use `_v` for variable postfix - `inline constexpr` where ever possible - `inline constexpr bool` maintains consistency - using `HPX_FORWARD` which uses `static_cast` - `void` is the default type for `std::enable_if_t` Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

- The awaitable concept simply checks whether the type supports applying the co_await operator to avalue of that type. - If the object has either a member or non-member operator co_await() then its return value must satisfy the Awaiter concept. Otherwise, the Awaitable object must satisfy the Awaiter concept itself. Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

- Await transform concept was accepting universal value arguments Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

libs/core/futures/include/hpx/futures/traits/detail/future_await_traits.hpp

libs/core/execution_base/include/hpx/execution_base/traits/coroutine_traits.hpp

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

- `hpx::coro` is now defined under certain conditions - `__has_include` is found and either of the coroutine std headers are found Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

hkaiser · 2022-07-19T17:54:17Z

@SAtacker I think you need to wrap the new configuration header in an #if defined(...) to circumvent the build errors. There is also some inspect errors that needs fixing.

SAtacker · 2022-07-19T18:32:18Z

@SAtacker I think you need to wrap the new configuration header in an #if defined(...) to circumvent the build errors. There is also some inspect errors that needs fixing.

8e63d93#diff-c87c94ab3db419c43094f1852b046cd3200ae0bfb7a66a482f51be42bf76803bR36-R38
and 8e63d93#diff-73bb5d75ca22afdf27a16580a7a5a51d476e49c6b4357c8e591fb1485397b094R38-R40 should have taken care of it, I had thought so

SAtacker · 2022-07-20T07:36:50Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

hkaiser · 2022-07-20T15:31:07Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

What I meant is that you either have to avoid #including the new coroutine_traits.hpp if coroutines support is not detected or wrap the content of the header in a corresponding #ifdef/#endif.

hkaiser · 2022-07-20T15:39:03Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

What I meant is that you either have to avoid #including the new coroutine_traits.hpp if coroutines support is not detected or wrap the content of the header in a corresponding #ifdef/#endif.

The second option seems to be better, however.

SAtacker · 2022-07-20T15:54:05Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

What I meant is that you either have to avoid #including the new coroutine_traits.hpp if coroutines support is not detected or wrap the content of the header in a corresponding #ifdef/#endif.

The second option seems to be better, however.

Did you mean https://github.com/STEllAR-GROUP/hpx/pull/5945/files#diff-73bb5d75ca22afdf27a16580a7a5a51d476e49c6b4357c8e591fb1485397b094R38-R40 ?

hkaiser · 2022-07-20T16:06:35Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

What I meant is that you either have to avoid #including the new coroutine_traits.hpp if coroutines support is not detected or wrap the content of the header in a corresponding #ifdef/#endif.

The second option seems to be better, however.

Did you mean https://github.com/STEllAR-GROUP/hpx/pull/5945/files#diff-73bb5d75ca22afdf27a16580a7a5a51d476e49c6b4357c8e591fb1485397b094R38-R40 ?

I meant adding a wrapper here: libs/core/config/include/hpx/config/coroutines_support.hpp

SAtacker · 2022-07-20T16:57:39Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

What I meant is that you either have to avoid #including the new coroutine_traits.hpp if coroutines support is not detected or wrap the content of the header in a corresponding #ifdef/#endif.

The second option seems to be better, however.

Did you mean https://github.com/STEllAR-GROUP/hpx/pull/5945/files#diff-73bb5d75ca22afdf27a16580a7a5a51d476e49c6b4357c8e591fb1485397b094R38-R40 ?

I meant adding a wrapper here: libs/core/config/include/hpx/config/coroutines_support.hpp

Then hpx::coro is not found by it.

diff --git a/libs/core/config/include/hpx/config/coroutines_support.hpp b/libs/core/config/include/hpx/config/coroutines_support.hpp
index fb7be8d728..08b1adb20a 100644
--- a/libs/core/config/include/hpx/config/coroutines_support.hpp
+++ b/libs/core/config/include/hpx/config/coroutines_support.hpp
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#if defined(HPX_HAVE_CXX20_COROUTINES)
+
 #if defined(__has_include)
 #if __has_include(<coroutine>)
 #include <coroutine>
@@ -25,3 +27,5 @@ namespace hpx { namespace coro {
 #define HPX_COROUTINE_NAMESPACE_STD std::experimental
 #endif
 #endif
+
+#endif // HPX_HAVE_CXX20_COROUTINES
\ No newline at end of file

SAtacker · 2022-07-20T16:59:11Z

I know it is supposed to work, but building systems always make it harder to believe.

hkaiser · 2022-07-20T18:14:20Z

https://github.com/STEllAR-GROUP/hpx/runs/7414352657?check_suite_focus=true#step:6:230 the coroutine test itself is failing

What I meant is that you either have to avoid #including the new coroutine_traits.hpp if coroutines support is not detected or wrap the content of the header in a corresponding #ifdef/#endif.

The second option seems to be better, however.

Did you mean https://github.com/STEllAR-GROUP/hpx/pull/5945/files#diff-73bb5d75ca22afdf27a16580a7a5a51d476e49c6b4357c8e591fb1485397b094R38-R40 ?

I meant adding a wrapper here: libs/core/config/include/hpx/config/coroutines_support.hpp

Then hpx::coro is not found by it.
diff --git a/libs/core/config/include/hpx/config/coroutines_support.hpp b/libs/core/config/include/hpx/config/coroutines_support.hpp
index fb7be8d728..08b1adb20a 100644
--- a/libs/core/config/include/hpx/config/coroutines_support.hpp
+++ b/libs/core/config/include/hpx/config/coroutines_support.hpp
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#if defined(HPX_HAVE_CXX20_COROUTINES)
+
 #if defined(__has_include)
 #if __has_include(<coroutine>)
 #include <coroutine>
@@ -25,3 +27,5 @@ namespace hpx { namespace coro {
 #define HPX_COROUTINE_NAMESPACE_STD std::experimental
 #endif
 #endif
+
+#endif // HPX_HAVE_CXX20_COROUTINES
\ No newline at end of file

You need to add a #include <hpx/config/defines.hpp> before using the HPX_HAVE_CXX20_COROUTINES constant (it's defined in that header).

- Add #ifdef guards around the namespace include file Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

libs/core/config/include/hpx/config.hpp

libs/core/config/include/hpx/config/coroutines_support.hpp

libs/core/execution_base/include/hpx/execution_base/traits/coroutine_traits.hpp

libs/core/config/include/hpx/config/coroutines_support.hpp

libs/core/execution_base/include/hpx/execution_base/traits/coroutine_traits.hpp

libs/core/futures/include/hpx/futures/traits/detail/future_await_traits.hpp

- `#if defined` not needed - Change `#pragma` location - Include `config` header instead of including separately - Prefer to check coroutine support first and then include the required headers Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

- await_suspend() accepts an argument of type coroutine_handle<void> Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

- Remove additional parentheses - Missing EOL Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>

StellarBot · 2022-07-21T20:31:28Z

Performance test report

HPX Performance

Comparison

BENCHMARK	FORK_JOIN_EXECUTOR	PARALLEL_EXECUTOR	SCHEDULER_EXECUTOR
For Each	(=)	(=)	(=)

Info

Property	Before	After
HPX Commit	`d5655f8`	`a7fffac`
HPX Datetime	2022-05-31T12:57:29+00:00	2022-07-21T18:55:40+00:00
Clustername	daint	daint
Compiler	/apps/daint/SSL/HPX/packages/llvm-11.0.0/bin/clang++ 11.0.0	/apps/daint/SSL/HPX/packages/llvm-11.0.0/bin/clang++ 11.0.0
Hostname	nid01193	nid00907
Envfile
Datetime	2022-05-31T15:13:01.357969+02:00	2022-07-21T22:30:12.398199+02:00

Comparison

BENCHMARK	NO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch	-

Info

Property	Before	After
HPX Commit	`d5655f8`	`a7fffac`
HPX Datetime	2022-05-31T12:57:29+00:00	2022-07-21T18:55:40+00:00
Clustername	daint	daint
Compiler	/apps/daint/SSL/HPX/packages/llvm-11.0.0/bin/clang++ 11.0.0	/apps/daint/SSL/HPX/packages/llvm-11.0.0/bin/clang++ 11.0.0
Hostname	nid01193	nid00907
Envfile
Datetime	2022-05-31T15:13:18.026239+02:00	2022-07-21T22:30:29.436978+02:00

Comparison

BENCHMARK	FORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATOR	PARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATOR	SCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add	(=)	(=)	(=)
Stream Benchmark - Scale	(=)	(=)	(=)
Stream Benchmark - Triad	(=)	(=)	(=)
Stream Benchmark - Copy	(=)	(=)	(=)

Info

Property	Before	After
HPX Commit	`71d8dbe`	`a7fffac`
HPX Datetime	2021-11-10T19:14:21+00:00	2022-07-21T18:55:40+00:00
Clustername	daint	daint
Compiler	/apps/daint/SSL/HPX/packages/llvm-11.0.0/bin/clang++ 11.0.0	/apps/daint/SSL/HPX/packages/llvm-11.0.0/bin/clang++ 11.0.0
Hostname	nid00120	nid00907
Envfile
Datetime	2021-11-10T20:28:18.266961+01:00	2022-07-21T22:30:44.790145+02:00

Explanation of Symbols

Symbol	MEANING
=	No performance change (confidence interval within ±1%)
(=)	Probably no performance change (confidence interval within ±2%)
(+)/(-)	Very small performance improvement/degradation (≤1%)
+/-	Small performance improvement/degradation (≤5%)
++/--	Large performance improvement/degradation (≤10%)
+++/---	Very large performance improvement/degradation (>10%)
?	Probably no change, but quite large uncertainty (confidence interval with ±5%)
??	Unclear result, very large uncertainty (±10%)
???	Something unexpected…

hkaiser

LGTM, thanks!

hkaiser · 2022-07-25T13:58:04Z

bors merge

bors · 2022-07-25T14:14:26Z

Build succeeded:

Bors

SAtacker force-pushed the p2300_enhancements branch from c414088 to 4d5056b Compare July 7, 2022 20:18

hkaiser added type: enhancement type: compatibility issue category: senders/receivers Implementations of the p0443r14 / p2300 + p1897 proposals labels Jul 8, 2022

hkaiser added this to the 1.9.0 milestone Jul 8, 2022