Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove operator| overloads for sync_wait and start_detached #346

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

msimberg
Copy link
Contributor

@msimberg msimberg commented Aug 4, 2022

Fixes #329 .

@msimberg msimberg added this to the 0.8.0 milestone Aug 4, 2022
@msimberg
Copy link
Contributor Author

msimberg commented Aug 4, 2022

bors try

bors bot added a commit that referenced this pull request Aug 4, 2022
@msimberg msimberg self-assigned this Aug 4, 2022
@bors
Copy link
Contributor

bors bot commented Aug 4, 2022

try

Build failed:

@msimberg msimberg force-pushed the no-pipe-sync_wait-start_detached branch from 44ff7d5 to df67152 Compare August 4, 2022 14:40
@pika-bot
Copy link
Collaborator

pika-bot commented Aug 4, 2022

Performance test report

pika Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)(=)+

Info

PropertyBeforeAfter
pika Datetime2021-12-21T15:01:46+00:002022-08-04T14:25:33+00:00
pika Commit01e4980e2184ca113b853ae3a36869f713fcc4b96185de8
Datetime2021-12-21T16:09:15.238666+01:002022-08-04T16:40:52.880564+02:00
Envfile
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00932nid00408
Clusternamedaintdaint

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch+++

Info

PropertyBeforeAfter
pika Datetime2021-12-10T13:50:04+00:002022-08-04T14:25:33+00:00
pika Commitf499a2233385060b8a2612ab88163e62b08818886185de8
Datetime2021-12-10T15:19:42.442217+01:002022-08-04T16:41:07.625837+02:00
Envfile
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00243nid00408
Clusternamedaintdaint

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
Stream Benchmark - Add(=)(=)-
Stream Benchmark - Scale-----
Stream Benchmark - Triad(=)--
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
pika Datetime2021-11-12T11:29:27+00:002022-08-04T14:25:33+00:00
pika Commitf64fbd02165a132a6276cedd14c586910abb79e46185de8
Datetime2021-11-12T12:57:50.824026+01:002022-08-04T16:41:22.962542+02:00
Envfile
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00007nid00408
Clusternamedaintdaint

Explanation of Symbols

SymbolMEANING
=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 (>10%)
++/--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…

@msimberg
Copy link
Contributor Author

msimberg commented Aug 4, 2022

bors try

bors bot added a commit that referenced this pull request Aug 4, 2022
@pika-bot
Copy link
Collaborator

pika-bot commented Aug 4, 2022

Performance test report

pika Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)(=)+

Info

PropertyBeforeAfter
pika Datetime2021-12-21T15:01:46+00:002022-08-04T14:40:25+00:00
pika Commit01e4980e2184ca113b853ae3a36869f713fcc4b98265b8b
Datetime2021-12-21T16:09:15.238666+01:002022-08-04T16:49:42.613703+02:00
Clusternamedaintdaint
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00932nid00075
Envfile

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch+++

Info

PropertyBeforeAfter
pika Datetime2021-12-10T13:50:04+00:002022-08-04T14:40:25+00:00
pika Commitf499a2233385060b8a2612ab88163e62b08818888265b8b
Datetime2021-12-10T15:19:42.442217+01:002022-08-04T16:49:59.735722+02:00
Clusternamedaintdaint
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00243nid00075
Envfile

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
Stream Benchmark - Add(=)(=)--
Stream Benchmark - Scale-----
Stream Benchmark - Triad(=)+---
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
pika Datetime2021-11-12T11:29:27+00:002022-08-04T14:40:25+00:00
pika Commitf64fbd02165a132a6276cedd14c586910abb79e48265b8b
Datetime2021-11-12T12:57:50.824026+01:002022-08-04T16:50:17.250560+02:00
Clusternamedaintdaint
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00007nid00075
Envfile

Explanation of Symbols

SymbolMEANING
=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 (>10%)
++/--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…

@bors
Copy link
Contributor

bors bot commented Aug 4, 2022

try

Build failed:

@msimberg msimberg force-pushed the no-pipe-sync_wait-start_detached branch from df67152 to dab083a Compare August 4, 2022 15:34
@msimberg
Copy link
Contributor Author

msimberg commented Aug 4, 2022

bors try

bors bot added a commit that referenced this pull request Aug 4, 2022
@bors
Copy link
Contributor

bors bot commented Aug 4, 2022

try

Build failed:

@pika-bot
Copy link
Collaborator

pika-bot commented Aug 4, 2022

Performance test report

pika Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)(=)+

Info

PropertyBeforeAfter
pika Datetime2021-12-21T15:01:46+00:002022-08-04T15:34:52+00:00
pika Commit01e4980e2184ca113b853ae3a36869f713fcc4b9478a341
Clusternamedaintdaint
Hostnamenid00932nid01193
Envfile
Datetime2021-12-21T16:09:15.238666+01:002022-08-04T17:59:08.983525+02:00
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch+++

Info

PropertyBeforeAfter
pika Datetime2021-12-10T13:50:04+00:002022-08-04T15:34:52+00:00
pika Commitf499a2233385060b8a2612ab88163e62b0881888478a341
Clusternamedaintdaint
Hostnamenid00243nid01193
Envfile
Datetime2021-12-10T15:19:42.442217+01:002022-08-04T17:59:23.576279+02:00
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
Stream Benchmark - Add-=-
Stream Benchmark - Scale------
Stream Benchmark - Triad-(=)(=)
Stream Benchmark - Copy(=)(=)=

Info

PropertyBeforeAfter
pika Datetime2021-11-12T11:29:27+00:002022-08-04T15:34:52+00:00
pika Commitf64fbd02165a132a6276cedd14c586910abb79e4478a341
Clusternamedaintdaint
Hostnamenid00007nid01193
Envfile
Datetime2021-11-12T12:57:50.824026+01:002022-08-04T17:59:38.253963+02:00
Compiler/apps/daint/SSL/pika/packages/llvm-11.0.0/bin/clang++ 11.0.0/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1

Explanation of Symbols

SymbolMEANING
=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 (>10%)
++/--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…

Copy link
Contributor

@aurianer aurianer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@msimberg
Copy link
Contributor Author

msimberg commented Aug 8, 2022

bors merge

bors bot added a commit that referenced this pull request Aug 8, 2022
330: Update CUDA sender functionality to work with P2300 reference implementation r=msimberg a=msimberg

Continuation of #315. Part 2/n to fix #302.

This has now been tested with clang 14 and CUDA. ~but a CI configuration will follow in a separate PR (the llvm build requires this patch: spack/spack#31661

- Makes `get_env` customizations `noexcept` as required (and enforced) by the P2300 reference implementation.
- Add a missing `range` header to `cuda_scheduler_bulk.hpp`.
- ADL-isolate sender and receiver types in `async_cuda` module.
- Fixes the `completion_signatures` for senders in `async_cuda`. Note that the helpers for computing the types have to be guarded with a `requires is_invocable_v...` because `make_completion_signatures` instantiates the helper with non-`set_value_t` signatures as well. I have to investigate whether this is a bug or expected behaviour.
- Because of the above, a number of helper "kernels" have received trailing `decltype`s to enable SFINAE with bogus arguments.
- Removes the SFINAE for `then_with_cuda_stream` etc. since the HIP branch has been merged into DLA-Future. The SFINAE is no longer needed for choosing a `then_with_*` overload and adds unnecessary complexity.
- The reference implementation's `split` does not yet support move-only senders, so some additional tests are disabled with CUDA and the reference implementation.
- Change the `clang-14` Jenkins configuration to use CUDA and the P2300 reference implementation.

Note, this is not for 0.7.0 as it needs more work. Reviews are, however, already welcome.

346: Remove `operator|` overloads for `sync_wait` and `start_detached` r=msimberg a=msimberg

Fixes #329 .

347: Add Codacy coverage report r=msimberg a=msimberg

Part of #4. Not fully tested, but I got some reporting already to Codacy so there is hope that this works...

To do:
- [x] badge

352: Increase timeout for container algorithms tests on macOS CI configuration r=msimberg a=msimberg

I'm increasing the timeout to check if these timeouts https://github.com/pika-org/pika/runs/7692289838?check_suite_focus=true#step:6:237 are simply due to the test taking long to complete or if it really hangs. `partial_sort_copy_range` is one of the longer running tests so it's possible that it indeed just doesn't have enough time to finish (the timeout is 120 seconds and one of the later runs finished in 101 seconds: https://github.com/pika-org/pika/runs/7691710341?check_suite_focus=true#step:6:237).

Co-authored-by: Mikael Simberg <simberg@cscs.ch>
Co-authored-by: Mikael Simberg <mikael.simberg@iki.fi>
@bors
Copy link
Contributor

bors bot commented Aug 8, 2022

Build failed (retrying...):

bors bot added a commit that referenced this pull request Aug 8, 2022
330: Update CUDA sender functionality to work with P2300 reference implementation r=msimberg a=msimberg

Continuation of #315. Part 2/n to fix #302.

This has now been tested with clang 14 and CUDA. ~but a CI configuration will follow in a separate PR (the llvm build requires this patch: spack/spack#31661

- Makes `get_env` customizations `noexcept` as required (and enforced) by the P2300 reference implementation.
- Add a missing `range` header to `cuda_scheduler_bulk.hpp`.
- ADL-isolate sender and receiver types in `async_cuda` module.
- Fixes the `completion_signatures` for senders in `async_cuda`. Note that the helpers for computing the types have to be guarded with a `requires is_invocable_v...` because `make_completion_signatures` instantiates the helper with non-`set_value_t` signatures as well. I have to investigate whether this is a bug or expected behaviour.
- Because of the above, a number of helper "kernels" have received trailing `decltype`s to enable SFINAE with bogus arguments.
- Removes the SFINAE for `then_with_cuda_stream` etc. since the HIP branch has been merged into DLA-Future. The SFINAE is no longer needed for choosing a `then_with_*` overload and adds unnecessary complexity.
- The reference implementation's `split` does not yet support move-only senders, so some additional tests are disabled with CUDA and the reference implementation.
- Change the `clang-14` Jenkins configuration to use CUDA and the P2300 reference implementation.

Note, this is not for 0.7.0 as it needs more work. Reviews are, however, already welcome.

346: Remove `operator|` overloads for `sync_wait` and `start_detached` r=msimberg a=msimberg

Fixes #329 .

Co-authored-by: Mikael Simberg <simberg@cscs.ch>
Co-authored-by: Mikael Simberg <mikael.simberg@iki.fi>
@bors
Copy link
Contributor

bors bot commented Aug 8, 2022

Build failed (retrying...):

@bors bors bot merged commit 311cb4f into pika-org:main Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove operator| overload for pika::this_thread::experimental::{sync_wait,start_detached}
3 participants