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

Segmented inclusive_scan and exclusive_scan implementation #2287

Merged
merged 31 commits into from Sep 25, 2016

Conversation

3 participants
@Sanac
Contributor

Sanac commented Aug 11, 2016

  • Added segmented inclusive_scan. (see #1338)
  • Added some simple tests
@biddisco

This comment has been minimized.

Show comment
Hide comment
@biddisco

biddisco Aug 11, 2016

Contributor

We had a long discussion about this today and my concern is that the first scan phase is done by each partition, but then the whold scan result is returned to the root partition where the merge/updte steps are performed and the final result vector is held on the root partition.

The reason for this was to do with iterators.

It seem that if the algorithm is called with a partitioned_vector as input, and a partitioned_vector as output (or in place), then those iterators are already present on the relevant partitions and the solution presented here is only useful if the output/dest iterator is actually a normal vector - and not a partitioned one.

@hkaiser - can you add a few lines to explain how to ensure that the merge/update phases will happen on the locality/partition that owns it, rather than back on the root.

Contributor

biddisco commented Aug 11, 2016

We had a long discussion about this today and my concern is that the first scan phase is done by each partition, but then the whold scan result is returned to the root partition where the merge/updte steps are performed and the final result vector is held on the root partition.

The reason for this was to do with iterators.

It seem that if the algorithm is called with a partitioned_vector as input, and a partitioned_vector as output (or in place), then those iterators are already present on the relevant partitions and the solution presented here is only useful if the output/dest iterator is actually a normal vector - and not a partitioned one.

@hkaiser - can you add a few lines to explain how to ensure that the merge/update phases will happen on the locality/partition that owns it, rather than back on the root.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 11, 2016

Member

can you add a few lines to explain how to ensure that the merge/update phases will happen on the locality/partition that owns it, rather than back on the root.

There is an example demonstrating how to work with the local parts of a partitioned vector only: https://github.com/STEllAR-GROUP/hpx/blob/master/examples/quickstart/partitioned_vector_spmd_foreach.cpp

Member

hkaiser commented Aug 11, 2016

can you add a few lines to explain how to ensure that the merge/update phases will happen on the locality/partition that owns it, rather than back on the root.

There is an example demonstrating how to work with the local parts of a partitioned vector only: https://github.com/STEllAR-GROUP/hpx/blob/master/examples/quickstart/partitioned_vector_spmd_foreach.cpp

@Sanac

This comment has been minimized.

Show comment
Hide comment
@Sanac

Sanac Aug 19, 2016

Contributor
  • Added optimization to use locality of data if available
  • In-place usage of the scan possible
  • Changed tests so that changes are tested
  • Also optimized tests so that less time is wasted doing setup and verification
Contributor

Sanac commented Aug 19, 2016

  • Added optimization to use locality of data if available
  • In-place usage of the scan possible
  • Changed tests so that changes are tested
  • Also optimized tests so that less time is wasted doing setup and verification
@Sanac

This comment has been minimized.

Show comment
Hide comment
@Sanac

Sanac Aug 22, 2016

Contributor
  • Refactoring of code in preparation for exclusive scan
  • Code can be reviewed
Contributor

Sanac commented Aug 22, 2016

  • Refactoring of code in preparation for exclusive scan
  • Code can be reviewed

@Sanac Sanac closed this Aug 22, 2016

@Sanac Sanac reopened this Aug 22, 2016

@Sanac Sanac changed the title from Segmented inclusive_scan implementation to Segmented inclusive_scan and exclusive implementation Aug 23, 2016

@Sanac Sanac changed the title from Segmented inclusive_scan and exclusive implementation to Segmented inclusive_scan and exclusive_scan implementation Aug 23, 2016

@Sanac

This comment has been minimized.

Show comment
Hide comment
@Sanac

Sanac Aug 23, 2016

Contributor
  • Added some comments in code
  • Added exclusive scan with tests to PR
Contributor

Sanac commented Aug 23, 2016

  • Added some comments in code
  • Added exclusive scan with tests to PR
Show outdated Hide outdated hpx/parallel/segmented_algorithms/detail/scan.hpp
if(beg_in != end_in) {
std::uint64_t in_id = traits_in::get_id(sit_in).get_msb();
std::uint64_t out_id = traits_in::get_id(sit_in).get_msb();

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Shouldn't this compare the whole id and not only the msb?

@hkaiser

hkaiser Sep 3, 2016

Member

Shouldn't this compare the whole id and not only the msb?

Show outdated Hide outdated hpx/parallel/segmented_algorithms/detail/scan.hpp
std::size_t in_dist = std::distance(beg_in, end_in);
std::size_t out_dist = std::distance(beg_out, end_out);
if (in_id != out_id || in_dist != out_dist) {

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Minor nitpick: there is no need to invoke std::distance if the ids are different.

@hkaiser

hkaiser Sep 3, 2016

Member

Minor nitpick: there is no need to invoke std::distance if the ids are different.

Show outdated Hide outdated hpx/parallel/segmented_algorithms/detail/scan.hpp
typename T, typename Op>
static typename util::detail::algorithm_result<ExPolicy, OutIter>::type
segmented_scan_par(ExPolicy const& policy, SegIter first,
SegIter last, OutIter dest, T init, Op && op, std::true_type)

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Once init has been copied at the uppermost API level it could be passed down by const&.

@hkaiser

hkaiser Sep 3, 2016

Member

Once init has been copied at the uppermost API level it could be passed down by const&.

Show outdated Hide outdated hpx/parallel/segmented_algorithms/exclusive_scan.hpp
// use first element to save last T of scan
return result::get(
dataflow([=, &op](vector_type r) {

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Is capturing op by reference safe in this context?

@hkaiser

hkaiser Sep 3, 2016

Member

Is capturing op by reference safe in this context?

This comment has been minimized.

@Sanac

Sanac Sep 5, 2016

Contributor

Lambdas are still new to me. Thanks for your comment.

@Sanac

Sanac Sep 5, 2016

Contributor

Lambdas are still new to me. Thanks for your comment.

Show outdated Hide outdated hpx/parallel/segmented_algorithms/inclusive_scan.hpp
}
return result::get(
dataflow([=, &op](vector_type r) {

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Same here, is capturing op by reference safe?

@hkaiser

hkaiser Sep 3, 2016

Member

Same here, is capturing op by reference safe?

Show outdated Hide outdated tests/unit/component/partitioned_vector_exclusive_scan.cpp
int main()
{
std::vector<hpx::id_type> localities = hpx::find_remote_localities();

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Is this test supposed to run in distributed?

@hkaiser

hkaiser Sep 3, 2016

Member

Is this test supposed to run in distributed?

This comment has been minimized.

@Sanac

Sanac Sep 5, 2016

Contributor

Yes those should be distributed to test the version of the algorithm that use the locality of the data. (i.e. scan is performed on partition that owns the data instead of the root partition)

@Sanac

Sanac Sep 5, 2016

Contributor

Yes those should be distributed to test the version of the algorithm that use the locality of the data. (i.e. scan is performed on partition that owns the data instead of the root partition)

Show outdated Hide outdated tests/unit/component/partitioned_vector_inclusive_scan.cpp
int main()
{
std::vector<hpx::id_type> localities = hpx::find_remote_localities();

This comment has been minimized.

@hkaiser

hkaiser Sep 3, 2016

Member

Same here: should this run in distributed, i.e. should it use the locality ids?

@hkaiser

hkaiser Sep 3, 2016

Member

Same here: should this run in distributed, i.e. should it use the locality ids?

This comment has been minimized.

@Sanac

Sanac Sep 5, 2016

Contributor

Same as the other test.

@Sanac

Sanac Sep 5, 2016

Contributor

Same as the other test.

This comment has been minimized.

@hkaiser

hkaiser Sep 5, 2016

Member

I now understand, the find_remote_localities() call is not necessary here as it is done again in the called functions. Alternatively you may pass localities to the actual tests.

@hkaiser

hkaiser Sep 5, 2016

Member

I now understand, the find_remote_localities() call is not necessary here as it is done again in the called functions. Alternatively you may pass localities to the actual tests.

@Sanac

This comment has been minimized.

Show comment
Hide comment
@Sanac

Sanac Sep 5, 2016

Contributor
  • Fixed comparison of msb to locality id
  • Invocation of std::distance only if ids are the same
  • Passed down init by const&
  • Capturing of op done by value
Contributor

Sanac commented Sep 5, 2016

  • Fixed comparison of msb to locality id
  • Invocation of std::distance only if ids are the same
  • Passed down init by const&
  • Capturing of op done by value
@Sanac

This comment has been minimized.

Show comment
Hide comment
@Sanac

Sanac Sep 12, 2016

Contributor
  • Pass localities via parameter in the tests
Contributor

Sanac commented Sep 12, 2016

  • Pass localities via parameter in the tests
@hkaiser

LGTM, thanks a lot!

@hkaiser hkaiser merged commit aef13f1 into STEllAR-GROUP:master Sep 25, 2016

1 check passed

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