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

Implement parallel::inplace_merge. #2978

Merged
merged 11 commits into from Nov 8, 2017

Conversation

Projects
None yet
2 participants
@taeguk
Copy link
Member

commented Oct 28, 2017

Check Box

  • Implementation of parallel::inplace_merge only for random access iterator.
  • Unit tests.
  • Benchmark codes.
  • Adapt to Ranges TS.

Issues

  1. How to support bidirectional iterator. (#2826)
    -> For now, restrict the requirements of parallel::inplace_merge to only random access iterator.

Note for future

  1. parallel::inplace_merge is about 2 times or more faster than sequential thing.
  2. Utilizing 2 cores or 4 cores is the limitation of the algorithm. More cores can't improve performance. I assume performance bottleneck is due to memory bandwidth.

taeguk added some commits Oct 22, 2017

Implement parallel::inplace_merge.
flyby: Modify documentations of some parallel algorithms.
Add container version of parallel::inplace_unique. And add unit tests…
… 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.
Add benchmark codes for 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.
@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 29, 2017

@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)?


template <typename ExPolicy, typename RandIter,
typename Comp, typename Proj,
HPX_CONCEPT_REQUIRES_(

This comment has been minimized.

Copy link
@hkaiser

hkaiser Oct 29, 2017

Member

Do we have to use concepts for this internal API? What is your rationale of using it here?

This comment has been minimized.

Copy link
@taeguk

taeguk Nov 5, 2017

Author Member

@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(

This comment has been minimized.

Copy link
@hkaiser

hkaiser Oct 29, 2017

Member

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?

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 5, 2017

Member

I was just wondering. No need to change things.

This comment has been minimized.

Copy link
@taeguk

taeguk Nov 5, 2017

Author Member

@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.

This comment has been minimized.

Copy link
@taeguk

taeguk Nov 5, 2017

Author Member

@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??)

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 5, 2017

Member

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.

template <typename ExPolicy, typename RandIter,
typename Comp, typename Proj,
HPX_CONCEPT_REQUIRES_(
hpx::traits::is_random_access_iterator<RandIter>::value)>

This comment has been minimized.

Copy link
@taeguk

taeguk Nov 5, 2017

Author Member

@hkaiser Is this use of concepts strange, too?

This comment has been minimized.

Copy link
@hkaiser

hkaiser Nov 5, 2017

Member

No need to change things.

This comment has been minimized.

Copy link
@taeguk

taeguk Nov 5, 2017

Author Member

@hkaiser But, I removed that concept for consistency of codes.

taeguk added some commits Nov 5, 2017

@taeguk taeguk force-pushed the taeguk:tg_inplace_merge branch 2 times, most recently from 78ad041 to 12eb526 Nov 5, 2017

@taeguk

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2017

@hkaiser I referenced the new algorithm for the documentation index as you said.
And I also resolved merge conflicts.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

@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?

@taeguk

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2017

@hkaiser I'm ready to be merged. I supported async with execution::async_execute(..).
Do you want different async implementation for inplace_merge?

@hkaiser hkaiser merged commit 2013104 into STEllAR-GROUP:master Nov 8, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.