Implement parallel::inplace_merge. #2978
Conversation
flyby: Modify documentations of some parallel algorithms.
… for that. flyby: Add tests for user_defined_type in unit test of container version of parallel::merge. flyby: Remove non-meaning thing in unit test of parallel::inplace_merge.
flyby: Fix my mistake that container isn't sorted before used in benchmark code of parallel::merge. flyby: Change order of restoring container data in benchmark code of parallel::partition.
…k of parallel::inplace_merge.
|
@taeguk Could you add the new files to the build system, please (see here: https://github.com/STEllAR-GROUP/hpx/blob/master/docs/CMakeLists.txt#L46-L196)? Also, could you reference the new algorithm for the documentation index as well (here: https://github.com/STEllAR-GROUP/hpx/blob/master/docs/hpx.idx#L165-L348)? |
hpx/parallel/algorithms/merge.hpp
Outdated
|
|
||
| template <typename ExPolicy, typename RandIter, | ||
| typename Comp, typename Proj, | ||
| HPX_CONCEPT_REQUIRES_( |
There was a problem hiding this comment.
Do we have to use concepts for this internal API? What is your rationale of using it here?
There was a problem hiding this comment.
@hkaiser I just wanted to restrict that RandIter is only random access iterator.
Orginally, inplace_merge have to support bidirectional iterator, too.
But, in now, it only supports random access iterator.
I thought that I might have to implement different helper function for the bidirectional iterator in the future.
In that context, I used concepts for this helper function.
If it is strange or misuse of concepts, I will modify this. Is this misuse of concepts?
| > algorithm_result; | ||
|
|
||
| try { | ||
| return algorithm_result::get( |
There was a problem hiding this comment.
If implementing an asynchronous version of inplace_merge is not feasible/necessary, could you at least run the algorithm on a new thread for par(task) and similar?
There was a problem hiding this comment.
I was just wondering. No need to change things.
There was a problem hiding this comment.
@hkaiser Oh.. I was very fool.... :(
I don't know why I didn't consider asynchronous execution policy..
As you say, I modify the algorithm will run on a new thread.
There are similar problems in parallel::merge and parallel::partition, too.
I will fix them in another PR soon.
There was a problem hiding this comment.
@hkaiser The problem you suggested should be fixed as I think. Why I don't need to change things?
(Maybe your mis-addressing for comment??)
There was a problem hiding this comment.
What I meant is that there is no need to remove the concept checks.
I'd very much appreciate you adding the asynchronous behavior, though.
hpx/parallel/algorithms/merge.hpp
Outdated
| template <typename ExPolicy, typename RandIter, | ||
| typename Comp, typename Proj, | ||
| HPX_CONCEPT_REQUIRES_( | ||
| hpx::traits::is_random_access_iterator<RandIter>::value)> |
…in parallel::inplace_merge.
…::merge and parallel::inplace_merge.
taeguk
force-pushed
the
tg_inplace_merge
branch
2 times, most recently
from
78ad041
to
12eb526
Compare
|
@hkaiser I referenced the new algorithm for the documentation index as you said. |
|
@taeguk do you plan do add the async implementations to this PR or would that be another one? IOW, can we go ahead with merging this PR now? |
|
@hkaiser I'm ready to be merged. I supported async with |
Check Box
Issues
-> For now, restrict the requirements of parallel::inplace_merge to only random access iterator.
Note for future