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

Adding split_future #2246

Merged
merged 7 commits into from Jul 22, 2016

Conversation

Projects
None yet
4 participants
@hkaiser
Copy link
Member

commented Jul 10, 2016

This implements a first version of the split_future facility.

Still missing:

  • support for shared_future
  • MSVC12 fails compiling this (I gave up trying to make it work for this compiler, instead I disabled the feature alltogether)

This fixes #2239

hkaiser added some commits Jul 8, 2016

Adding split_all
- this allows to convert a future storing a tuple of values into a tuple of futures, each storing one of the elements of the original tuple
More work on split_future
- renamed to split_future
- added support for array<T, N>
- added special cases for future<tuple<>> and array<T, 0>
- added some documentation
- added a missing keyword 'template'

@hkaiser hkaiser added this to the 0.9.99 milestone Jul 10, 2016

@hkaiser hkaiser referenced this pull request Jul 10, 2016

Closed

Create a new facility lcos::split_all #2239

3 of 3 tasks complete
@biddisco

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2016

I have had a good look through this and it seems to be exactly what we need. I have not been able to test it yet as the existing code needs some redesign to integrate split_future in.

On thing that troubles me is that the code makes use of future_data, acquire_future, acquire_shared_state, future_access, future_traits and other internals that are undocumented. Reading through this code I had to go into all the other headers and 'discover' what the other functions were doing and verify them mentally by working through the code. If each of these had a short comment section at the start of the header explaining what the purpose of each of the classes/structs/traits were for, then it would be much easier to learn from examples like split_future and move towards implementing other features without assistance.

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2016

On thing that troubles me is that the code makes use of future_data, acquire_future, acquire_shared_state, future_access, future_traits and other internals that are undocumented. Reading through this code I had to go into all the other headers and 'discover' what the other functions were doing and verify them mentally by working through the code. If each of these had a short comment section at the start of the header explaining what the purpose of each of the classes/structs/traits were for, then it would be much easier to learn from examples like split_future and move towards implementing other features without assistance.

Fair point. We have not formally documented our customization points (yet), but we should do so. I think this could look very much like the documentation of the customization points for Boost Spirit.

I will try to add comments over time, at least. However, if you have learnt something specific which was not obvious right away, feel free to add that information as a comment to any of those facilities as well.

@dcbdan

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

The following code snippet printed the wrong output for me. This was printed

Printing map: 
Printing set: 0 1 10 100 101 2000 

not

Printing map: (6, 43556) (123, 321) (999, 999) 
Printing set: 0 1 10 100 101 2000 
#include <hpx/hpx_init.hpp>
#include <hpx/hpx.hpp>

#include <iostream>
#include <utility>
#include <vector>

///////////////////////////////////////////////////////////////////////////////
int hpx_main(int argc, char* argv[])
{
    using pair_type     = std::pair<std::map<int, int>, std::set<int> >;
    using pair_fut_type = std::pair<hpx::future<std::map<int, int> >,
                              hpx::future<std::set<int> > >;

    std::map<int, int> mm;
    // fill mm with arbitrary values
    mm[123] = 321;
    mm[999] = 999;
    mm[6]   = 43556;

    // fill ss awith arbitrary values
    std::set<int> ss{0, 1, 10, 100, 101, 2000};

    pair_type p = std::make_pair(mm, ss);
    hpx::future<pair_type> f_pair = hpx::make_ready_future(p);

    // given a future of a pair, get a pair of futures

    // Doesn't work, needs to receive r value
    // pair_fut_type pair_f = hpx::split_future(f_pair);

    // Fine, take your r value
    pair_fut_type pair_f = hpx::split_future(std::move(f_pair));

    // see if the values of mm2 and ss2 are the same
    std::map<int, int> mm2 = pair_f.first.get();
    std::cout << "Printing map: ";
    for(auto val: mm2)
        std::cout << "(" << val.first << ", " << val.second << ") ";
    std::cout << std::endl;

    std::set<int> ss2 = pair_f.second.get();
    std::cout << "Printing set: ";
    for(auto val: ss2)
        std::cout << val << " ";
    std::cout << std::endl;

    return hpx::finalize();
}

int main(int argc, char* argv[])
{
    return hpx::init(argc, argv);
}
Fix incorrect data movement
- added test
@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2016

@dcbdan Thanks for providing the test revealing a data move-problem. This has been fixed now.

@sithhell sithhell modified the milestones: 0.9.99, 1.0.0 Jul 15, 2016

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

This PR is ready to be merged now

@sithhell

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

LGTM!

@hkaiser hkaiser merged commit 11cee98 into master Jul 22, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@hkaiser hkaiser deleted the fixing_2239 branch Jul 22, 2016

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.