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.

Sign up for GitHub

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

Jump to bottom

Replace pika's invoke_result with std::invoke_result #599

Merged
merged 2 commits into from
Feb 16, 2023
Merged

Replace pika's invoke_result with std::invoke_result #599

merged 2 commits into from
Feb 16, 2023

Conversation

aurianer
Copy link
Contributor

@aurianer aurianer commented Feb 15, 2023 •
edited
Loading

Fix #543

@aurianer aurianer added this to the 0.13.0 milestone Feb 15, 2023
@aurianer aurianer self-assigned this Feb 15, 2023
@aurianer aurianer mentioned this pull request Feb 15, 2023
Open
59 tasks
@msimberg msimberg changed the title Replace pika invoke result with std Replace pika's invoke_result with std::invoke_result Feb 15, 2023
@aurianer aurianer force-pushed the replace_pika_invoke_result_with_std branch 2 times, most recently from 42c8797 to 30d48f8 Compare February 15, 2023 18:09
@aurianer
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Feb 15, 2023
@bors
Try #599:
7c36de8
@bors
Copy link
Contributor

bors bot commented Feb 15, 2023

try

Build failed:

@pika-bot
Copy link
Collaborator

Performance test report

pika Performance

Comparison

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

Info

PropertyBeforeAfter
pika Datetime2022-09-16T08:18:06+00:002023-02-15T18:09:52+00:00
pika Commit190f1890c24afe
Envfile
Datetime2022-09-16T10:25:01.976661+02:002023-02-15T19:17:45.188789+01:00
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Clusternamedaintdaint
Hostnamenid00074nid00423

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
msimberg requested changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

This looks good on a high level. Thanks @aurianer. Could you please add std::invoke_result(_v) and type_traits to the inshpect config please? I think there are a few pika/type_traits.hpp includes left that need to be removed (and type_traits added if it's not already there).

@aurianer
Copy link
Contributor Author

aurianer commented Feb 16, 2023 •
edited
Loading

Yes will add the rule to inshpect, I thought of removing the invoke_result.hpp header include and check that <type_traits> was included but I didn't check pika/type_traits.hpp, will do
EDIT: pika/type_traits.hpp is not included in my branch

@aurianer aurianer force-pushed the replace_pika_invoke_result_with_std branch from 30d48f8 to 5ef04fc Compare February 16, 2023 14:25
@pika-bot
Copy link
Collaborator

Performance test report

pika Performance

Comparison

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

Info

PropertyBeforeAfter
pika Commit190f189f918db4
pika Datetime2022-09-16T08:18:06+00:002023-02-16T14:25:45+00:00
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Envfile
Clusternamedaintdaint
Datetime2022-09-16T10:25:01.976661+02:002023-02-16T15:34:24.545375+01:00
Hostnamenid00074nid00677

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
msimberg reviewed Feb 16, 2023
View reviewed changes
.inshpect.toml Outdated Show resolved Hide resolved
@msimberg
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Feb 16, 2023
@bors
Try #599:
a645aa5
@bors
Copy link
Contributor

bors bot commented Feb 16, 2023

try

Build failed:

@aurianer aurianer force-pushed the replace_pika_invoke_result_with_std branch from 5ef04fc to be8de90 Compare February 16, 2023 15:31
@aurianer aurianer force-pushed the replace_pika_invoke_result_with_std branch from be8de90 to 8ea8d7a Compare February 16, 2023 15:33
@aurianer
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Feb 16, 2023
@bors
Try #599:
ffd3111
@pika-bot
Copy link
Collaborator

Performance test report

pika Performance

Comparison

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

Info

PropertyBeforeAfter
pika Commit190f1895924496
pika Datetime2022-09-16T08:18:06+00:002023-02-16T15:33:33+00:00
Hostnamenid00074nid00012
Clusternamedaintdaint
Envfile
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Datetime2022-09-16T10:25:01.976661+02:002023-02-16T16:39:47.235676+01:00

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 Feb 16, 2023

try

Build succeeded:

@aurianer aurianer marked this pull request as ready for review February 16, 2023 16:41
@aurianer aurianer requested a review from biddisco as a code owner February 16, 2023 16:41
@aurianer
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Feb 16, 2023
@bors @aurianer
Merge #599
2dad053 
599: Replace pika's `invoke_result` with `std::invoke_result` r=aurianer a=aurianer

Fix #543

Co-authored-by: aurianer <aurianer@cscs.ch>
@bors
Copy link
Contributor

bors bot commented Feb 16, 2023

Build failed:

@aurianer
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 16, 2023

Build succeeded:

@bors bors bot merged commit b6a7a9c into pika-org:main Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msimberg msimberg msimberg approved these changes

@biddisco biddisco Awaiting requested review from biddisco biddisco is a code owner

Assignees

@aurianer aurianer

Labels
None yet
Projects
pika planning board
Archived in project
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.

Attempt to replace invoke_result with std equivalent
3 participants
@aurianer @pika-bot @msimberg