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

Adapt min_element, max_element and minmax_element to C++20 #5241

Merged
merged 3 commits into from Nov 15, 2021

Conversation

Jedi18
Copy link
Contributor

@Jedi18 Jedi18 commented Mar 13, 2021

Adapts min_element, max_element and minmax_element to C++20.
Adds min_max_result result type
Add segmented algorithm tests for min_element, max_element and minmax_element

Any background context you want to provide?

#4822
#5156

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

@msimberg
Copy link
Contributor

retest

1 similar comment
@hkaiser
Copy link
Member

hkaiser commented Mar 22, 2021

retest

hkaiser
hkaiser previously approved these changes Apr 3, 2021
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to look at this! This looks good to me and can go in. Thanks a lot, good work!

@hkaiser
Copy link
Member

hkaiser commented Apr 3, 2021

@Jedi18 could you please rebase this onto master for a last time, we'll merge it afterwards.

@Jedi18
Copy link
Contributor Author

Jedi18 commented Apr 4, 2021

Thanks! Ok done, rebased onto master

@hkaiser
Copy link
Member

hkaiser commented Apr 4, 2021

retest

@hkaiser
Copy link
Member

hkaiser commented Apr 5, 2021

We now see more errors popping up (e.g. here: https://cdash.cscs.ch/viewBuildError.php?buildid=155130) I think this might be caused by those configurations testing things with HPX_NETWORKONG=OFF and HPX_WITHDISTRIBUTED_RUNTIME=OFF.

@msimberg
Copy link
Contributor

msimberg commented Apr 7, 2021

retest

@msimberg
Copy link
Contributor

msimberg commented Apr 7, 2021

@msimberg msimberg added the split: local PR targets local functionality label Nov 3, 2021
@hkaiser
Copy link
Member

hkaiser commented Nov 9, 2021

@Jedi18 FWIW, I have squashed all commits and rebased the branch onto master. This should simplify staying up to date in the future.

@hkaiser hkaiser force-pushed the adapt_min_max_minmax branch 3 times, most recently from a022b05 to 664c43d Compare November 10, 2021 17:05
@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

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

Info

PropertyBeforeAfter
HPX Datetime2021-08-21T11:38:52+00:002021-11-10T17:05:03+00:00
HPX Commit254db57c353e13
Datetime2021-08-23T15:21:22.855927+02:002021-11-10T18:15:01.637628+01:00
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
Envfile
Clusternamedaintdaint
Hostnamenid00446nid01381

Comparison

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

Info

PropertyBeforeAfter
HPX Datetime2021-10-22T15:54:13+00:002021-11-10T17:05:03+00:00
HPX Commit916f4d8c353e13
Datetime2021-10-25T11:52:24.316784+02:002021-11-10T18:15:16.968917+01:00
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
Envfile
Clusternamedaintdaint
Hostnamenid01196nid01381

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 (≤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…

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

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

Info

PropertyBeforeAfter
HPX Commit254db57c353e13
HPX Datetime2021-08-21T11:38:52+00:002021-11-10T17:05:03+00:00
Datetime2021-08-23T15:21:22.855927+02:002021-11-10T18:27:41.053243+01:00
Clusternamedaintdaint
Envfile
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
Hostnamenid00446nid01318

Comparison

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

Info

PropertyBeforeAfter
HPX Commit916f4d8c353e13
HPX Datetime2021-10-22T15:54:13+00:002021-11-10T17:05:03+00:00
Datetime2021-10-25T11:52:24.316784+02:002021-11-10T18:27:56.308797+01:00
Clusternamedaintdaint
Envfile
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
Hostnamenid01196nid01318

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 (≤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…

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

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

Info

PropertyBeforeAfter
HPX Datetime2021-08-21T11:38:52+00:002021-11-10T19:57:12+00:00
HPX Commit254db578c9badb
Envfile
Clusternamedaintdaint
Datetime2021-08-23T15:21:22.855927+02:002021-11-10T21:13:22.348157+01:00
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
Hostnamenid00446nid00120

Comparison

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

Info

PropertyBeforeAfter
HPX Datetime2021-10-22T15:54:13+00:002021-11-10T19:57:12+00:00
HPX Commit916f4d88c9badb
Envfile
Clusternamedaintdaint
Datetime2021-10-25T11:52:24.316784+02:002021-11-10T21:13:37.697222+01:00
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
Hostnamenid01196nid00120

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 (≤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 hkaiser force-pushed the adapt_min_max_minmax branch 3 times, most recently from 7e00755 to c5c9b7b Compare November 11, 2021 14:57
@hkaiser hkaiser force-pushed the adapt_min_max_minmax branch 2 times, most recently from 1168f20 to 70d9d50 Compare November 12, 2021 14:54
@hkaiser
Copy link
Member

hkaiser commented Nov 12, 2021

I think this is fine now, the problems have been resolved. Let's wait for the last round of tests to come back and merge if all is green.

@Jedi18
Copy link
Contributor Author

Jedi18 commented Nov 12, 2021

Thanks a lot! I guess the LSU tests aren't running because rostam is down?

@msimberg
Copy link
Contributor

This just needs a clang-format round... @Jedi18 or @hkaiser could you apply that?

Incidentally, @hkaiser, I see changes to the channel communicator example. I assume that's otherwise unrelated to the changes here(...), but related to the timeouts that happen every now and then on that example?

- flyby: more modernization of the min_element, max_element, and
  minmax_element algorithms
@hkaiser
Copy link
Member

hkaiser commented Nov 12, 2021

This just needs a clang-format round... @Jedi18 or @hkaiser could you apply that?

Done.

Incidentally, @hkaiser, I see changes to the channel communicator example. I assume that's otherwise unrelated to the changes here(...), but related to the timeouts that happen every now and then on that example?

Yes.

@msimberg
Copy link
Contributor

Incidentally, @hkaiser, I see changes to the channel communicator example. I assume that's otherwise unrelated to the changes here(...), but related to the timeouts that happen every now and then on that example?

Yes.

All right, thanks for looking into that!

bors merge

@bors
Copy link

bors bot commented Nov 15, 2021

👎 Rejected by code reviews

@msimberg
Copy link
Contributor

bors merge

@bors
Copy link

bors bot commented Nov 15, 2021

Build succeeded:

@bors bors bot merged commit 559b04e into STEllAR-GROUP:master Nov 15, 2021
@hkaiser hkaiser added this to the 1.8.0 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: algorithms split: distributed PR targets distributed functionality split: docs PR targets documentation split: local PR targets local functionality type: compatibility issue type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants