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 channel LCO #2265

Merged
merged 13 commits into from Aug 10, 2016
Merged

Adding channel LCO #2265

merged 13 commits into from Aug 10, 2016

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Jul 25, 2016

This PR adds two new LCOs: local::channel and channel (one for local only operation, the other is a component). Both expose a similar interface which is aligned with the channel object known from the Go language.

This is meant to be a basis for discussions aiming at finding the best possible API for such a facility, so please comment.

- the API is modelled after the channel type of the Go language
- add more diagnostics
- add tests
- flyby: fix queue component
- flyby: fixing issue in HPX causing actions returning future<void> swallowing exceptions
- fixed sequencing issue during channel::close

{
hpx::lcos::local::channel<int> c;
hpx::lcos::local::channel<> done;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do the types not match in the function signature and here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They could match. receive_channel and send_channel are constructable from channel, but they limit its use to one end of the communication. Thus this would work as well:

void do_something(  
    hpx::lcos::local::channel<int> c,  
    hpx::lcos::local::channel<> done)  
{ ... }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I wanted to say is that this information (implicit conversion and purpose) is missing from the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs are currently sketchy at best anyways ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so why not make them good from the start ;)?

{
return std::make_pair(f.get(), true);
}
return std::make_pair(T(), false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is swalloing exceptions, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is used to flag the end of a range, so it ignores the exceptions. This is yet another argument in favor of making the iterators lazy.

@hkaiser
Copy link
Member Author

hkaiser commented Jul 28, 2016

@sithhell How should we proceed here? Is this ok to be merged?

@sithhell
Copy link
Member

@hkaiser I am not sure, how should we resolve the remaining open points?

@hkaiser
Copy link
Member Author

hkaiser commented Jul 29, 2016

@sithhell The only thing I can try to work on is to make the iterators lazy. Everything else requires decisions beyond the scope of this PR.

@hkaiser
Copy link
Member Author

hkaiser commented Aug 1, 2016

Some more investigation has shown that implementing asyncronous iterators requires building a completely new infrastructure which is way beyond the scope of this PR. Thus I'd like to go ahead with this as is now. If the current iterator design is too controversial I'd be happy to remove it alltogether for now.

@hkaiser
Copy link
Member Author

hkaiser commented Aug 2, 2016

[07:05] heller: regarding channel ... should we just ditch the iterator? do you have a specific use case?
[07:05] heller: if you want to keep it ... let's keep it synchronous
[07:06] hkaiser: heller: I want to work on a async iterator framework, but that requires more time
[07:06] heller: nod
[07:06] hkaiser: I have no problems ditching the iterator for now
[07:07] heller: I am in favor for having the async iterator framework first
[07:07] heller: nevertheless, I have no problem keeping it synchronous for now
[07:07] hkaiser: ok
[07:07] hkaiser: I'll leave it in

@hkaiser
Copy link
Member Author

hkaiser commented Aug 2, 2016

Once #2276 has been merged we can change the interfaces for the set/set_sync/set_apply functions, etc.

@hkaiser
Copy link
Member Author

hkaiser commented Aug 8, 2016

This PR is ready to be merged now. The channel API has been adapted to the new launch policies.

hpx::apply(&sum, std::vector<int>(s.begin() + s.size()/2, s.end()), c);

int x = c.get_sync(); // receive from c
int y = c.get_sync();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines above don't look right. Why don't you use launch policies there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot to change the example.

@hkaiser hkaiser merged commit 41485e7 into master Aug 10, 2016
@hkaiser hkaiser deleted the adding_channel branch August 10, 2016 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants