Implement parallel::partition_copy.

[taeguk]

Check Box

• Implementation of partition_copy.
• Unit tests for partition_copy.
• Benchmark codes for partition_copy.
• Benchmark with using policy.executor() instead of hpx::launch::sync in scan_partitioner.
• Adapting iterator requirements (Adapting iterator requirements for parallel algorithms #2733).
• Adapt to Ranges TS (Adapt all parallel algorithms to Ranges TS #1668).
• Adapt to master branch.

** Issue List **

• Benchmark results are inconsistent. (They are different whenever I benchmark.) (https://gist.github.com/taeguk/e40b628fefe025aa3ffc1335bbeed4ee) => With forcing the same seed, benchmark results are repeatable.
• Parallel version is slower than sequential version. (parallel version is faster if scan_partitioner use policy.executor()).
• 'Memory Allocation for caching predicate result' vs 'Duplicated calling predicate' (https://github.com/STEllAR-GROUP/hpx/tree/9644d143cc27d4933664d920c0524cb77ce23889 vs https://github.com/STEllAR-GROUP/hpx/tree/12701f567fb1fac52487339d053f0b966a49f188)

Note

2017/6/25

I implemented partition_copy with reference to copy_if.
The behavior is good, but the benchmark results are very bad.
I must find new efficient way and re-implement partition_copy

2017/7/4

The bad performance is due to #2325.
With using policy.executor() instead of hpx::launch::sync in scan_partitioner, the performance is good. (https://gist.github.com/taeguk/6abe03f9b4cb878872d2bb634cae65b0)

[hkaiser]

@taeguk: invoking the predicate twice for each element is not allowed, see here: http://en.cppreference.com/w/cpp/algorithm/partition_copy

Complexity

Exactly distance(first, last) applications of p.

[taeguk]

@hkaiser You're right. I did not know that.
But, still it does not seem good to allocate additional memory.
However, unfortunately I have no way to resolve that issue.
It would be nice to be able to use memory space of the container pointed to by the Output Iterator, but this does not seem easy because of generalization of algorithm interface.

[hkaiser]

Yes, I agree. I think the only viable solution is to allocate that additional array of Booleans as you had it in the first place.

[Naios]

Just a small hint: member functions and templated functions are implicitily inline

[Naios]

You should perfect forward callable objects since those can be overloaded on r-value references:

struct my_callable {
  void operator()()&& {
    // ...
  }
};

[hkaiser]

@Naios: Not sure if all of our compilers support that. We should add a feature test if we want to use this. Also, if we do, this could be applied in many places.

[hkaiser]

@Naios: To clarify, I meant the operator()() &&

[hkaiser]

@Naios: also, in this context, perfect forwarding wouldn't work as pred (and proj) shouldn't be invalidated (as it might - if moved); pred will be used more than once.

[Naios]

@hkaiser I guess all required compiler versions should support it:

• GCC since 4.8 or 4.9
• Clang since 3.3 or 3.4
• MSVC since 2015 (19.0)

However, it is safe for the future to support perfect forwarding for callable objects.

// EDIT Because of the second comment: yes that's right

[hkaiser]

Also, the inspect tool is not happy with your code: https://7095-4455628-gh.circle-artifacts.com/0/tmp/circle-artifacts.KS6bV0d/hpx_inspect_report.html

[hkaiser]

I'd suggest to keep the work on the scan_partitioner separate from the partition_copy algorithm implementation.

[taeguk]

@hkaiser Sorry, this is my mistake. This is just for backup. This commit will be removed.

[hkaiser]

@taeguk: could we merge this step by step? Could we merge the current state of partition_copy?

Wrt bad performance: I'd suggest to make the change from hpx::launch::sync to policy.executor() and watch the regression tests (rostam.cct.lsu.edu).

[taeguk]

@hkaiser I had already done such a test. With policy.executor, I could get acceptable performance.
I have some commits that are not pushed. And I have to fix some tiny things like inspection errors.

I stopped the progress of this PR because of #2733.
I had a plan to add some commits for adapting this PR to #2733 after #2733 is merged.
I am waiting for it to be merged.

Do you want that I will finish this PR without adapting to #2733?

[hkaiser]

@taeguk ok, understood. I applied the first batch of name changes to #2733. I will try to apply the rest asap. In the meantime it might be helpful if you added your review to #2733 as our policy is to have at least one review before merging.

[taeguk]

@hkaiser I'm ready to be merged if you want that you will do adapting this PR to #2733.
Otherwise, if you want that I will do that, I will do more commits in here after #2733 is merged.

[taeguk]

@hkaiser Sorry, I have one thing to do for Ranges TS. I will add partition_copy to container_algorithms/partition.hpp.

I have a question. As you think, which one is better between "one PR for both an implemention of parallel algorithm and an adaption for Ranges TS" and "divide one PR into two for each"?

[taeguk]

@hkaiser Finally, I'm ready to be merged!

[hkaiser]

LGTM, thanks a lot!

[hkaiser]

Benchmark results are inconsistent. (They are different whenever I benchmark.) (https://gist.github.com/taeguk/e40b628fefe025aa3ffc1335bbeed4ee)

@taeguk: Have you tried whether you get repeatable results when forcing the same seed? The different results could be caused by a different amount of work needed to perform the partitioning.

[taeguk]

@hkaiser Oh, I got almost the same results when forcing the same seed! Great :)

[hkaiser]

@taeguk: Thanks a lot for your work on this. That's much appreciated! Please get in contact with aserio on IRC so he can send you a STE||AR-group t-shirt ;)