Generic client #1641

hkaiser · 2015-07-01T16:37:34Z

Cleaning up client object implementation …

adding generic components::client<> facility
move extract_action to traits
extract launch_policy into separate header
adding overloads for async and async_cb taking clients
cleaning up headers included by async and async_cb
added traits::is_valid_action
client_base is now a class semantically equivalent to future, it directly holds a shared state

hkaiser · 2015-07-01T16:39:07Z

This PR depends on merging #1640 first.

sithhell · 2015-07-20T07:22:36Z

Could you please rebase this PR against current master? It is very hard to review it with the changes of #1640 in between.

sithhell · 2015-07-21T10:19:43Z

hpx/runtime/components/client.hpp

+        {}
+        explicit client(naming::id_type && gid)
+          : base_type(lcos::make_ready_future(std::move(gid)))
+        {}


would it make sense here to disable the id_type constructors taking the component id and instead create a forwarding constructor, which takes the locality id and the constructor arguments?

sithhell · 2015-07-21T10:23:38Z

Looks good to me. In addition, this goes into the direction of what I proposed here:
http://thread.gmane.org/gmane.comp.lib.hpx.devel/286

We might want to continue the discussion about future directions after this has been merged.

hkaiser · 2015-07-21T16:01:37Z

I'd prefer continuing the discussion before merging. Let's do it right in the first place.

- renamed all functions `id_type get_gid()` to `get_id` - applied backwards compatibility flag `HPX_HAVE_COMPONENT_GET_GID_COMPATIBILITY` to temporarily still support `get_gid`

- adding generic components::client<> facility - move extract_action to traits - extract launch_policy into separate header - adding overloads for async and async_cb taking clients - cleaning up headers included by async and async_cb - added traits::is_valid_action

- fixing rebase conflicts - flyby make_client --> make_clients for version which returns more than one

- removing obsolete prototype

sithhell · 2016-04-03T14:09:41Z

For example, client is now the only future like type that makes async and friends behave like dataflow, that is, the action is only invoked when the client is ready.

Why do you think so? As far as I can see, clients behave exactly like future<>. This: async(id, f, ...) 'waits' for the future to become ready in order to invoke the action as well (however I think this is an implementation detail and not part of the semantics, it happens for remote invocations only anyways).

Fair point. The difference to regular futures is that we need the client to
be ready.

I agree that we're missing one particular piece of functionality, namely allowing to use future<id_type> as the target of an async(). But even that wouldn't introduce any consistencies if it was added.

Depends on how we define it. What we do is implicit unwrapping of the
future<id_type>, so we need special error handling here.

In the long run I would like to get rid of using id_types in the API altogether, they are not needed for the daily work. They are essentially void* and we should treat them as such.

I absolutely agree, that's the reason why I think it should expose the
semantics of id_type...

I think clients should expose the API of id_type and only deal internally with potential non ready states, if that makes sense.

Hmmm, I beg to disagree here. clients are initialized from the return value of new_<>, which gives us a future<id_type>. Reducing clients to id_types would force a barrier onto any use of a client. It also overly complicates code using them - as we have seen in the past (it leads to code which constantly explicitly packs/unpacks futures into clients and v.v.). Making them behave like futures (which they essentially are) greatly simplifies this.

... once id_type is removed this problem goes away automatically. We only
have to return client from new_ which does the unpacking etc by
itself. There shouldn't be a need for plain future<id_type> in the public
API anymore.

hkaiser · 2016-04-03T14:27:30Z

Fair point. The difference to regular futures is that we need the client to be ready.

For what? There are no semantic differences between future<id_type> and any client (at least that's the idea). At some point I was even considering to derive client_base<> from future<id_type>. I decided against it and replicated the future-API instead.

hkaiser · 2016-04-03T14:28:42Z

Depends on how we define it. What we do is implicit unwrapping of the future<id_type>, so we need special error handling here.

That error handling wouldn't be different from the error handling proposed by this PR: let the user deal with any exceptions.

hkaiser · 2016-04-03T14:35:08Z

I absolutely agree, that's the reason why I think it should expose the semantics of id_type...

... once id_type is removed this problem goes away automatically. We only have to return client from new_ which does the unpacking etc by itself. There shouldn't be a need for plain future<id_type> in the public API anymore.

I'm not sure we should let new_<Component> return a client directly. This would 'not look right':

client<Component> c = new_<Component>(id, ...);

(even if this currently would compile and work properly). I'd rather have it explicit:

client<Component> c = new_<client<Component> >(id, ...);

which already compiles and works as well.

In the end it's a matter of documentation and converting all code to this scheme. In plain C++ you can always write:

void* p = new Foo;

so why shouldn't we allow for

future<id_type> f = new_<Component>(id, ...);

?

sithhell · 2016-04-03T14:38:36Z

Fair point. The difference to regular futures is that we need the client to be ready.

For what? There are no semantic differences between future and any client (at least that's the idea). At some point I was even considering to derive client_base<> from future<id_type>. I decided against it and replicated the future-API instead.

We need the future to be ready to know the target where to invoke the
action on. The semantic difference to plain future comes from the
special handling in async and friends.

sithhell · 2016-04-03T14:43:56Z

I absolutely agree, that's the reason why I think it should expose the semantics of id_type...

... once id_type is removed this problem goes away automatically. We only have to return client from new_ which does the unpacking etc by itself. There shouldn't be a need for plain future in the public API anymore.

I'm not sure we should let new_ return a client directly. This would 'not look right':

client c = new_(id, ...);

(even if this currently would compile and work properly). I'd rather have it explicit:

This looks correct to me though. You are allocating a component and get a
client back.

client c = new_<client >(id, ...);

which already compiles and works as well.

This IMHO is confusing. You are not creating the client but the component.
Similar to shared_ptr, you are allocating a T and not the shared_ptr.

In the end it's a matter of documentation and converting all code to this scheme. In plain C++ you can always write:

void* p = new Foo;

so why shouldn't we allow for

future<id_type> f = new_(id, ...);

?

To enforce strong typing. If you really want that though I'd rather suggest
to have a client here instead of future<id_type>.

sithhell · 2016-04-03T14:46:09Z

Depends on how we define it. What we do is implicit unwrapping of the future, so we need special error handling here.

That error handling wouldn't be different from the error handling proposed by this PR: let the user deal with any exceptions.

Guess that's just a documentation issue in any case... That is the user
needs to know that the client might be erroneous and how to query the
possible exception.

hkaiser · 2016-04-03T14:47:47Z

We need the future to be ready to know the target where to invoke the action on. The semantic difference to plain future comes from the special handling in async and friends.

Thomas, I think we're talking about different things here. We have two different use cases:

(1) Use a future<id_type> or a client<Component> as the target of async<Action>()/dataflow<Action>()
This PR introduces the functionality allowing to use a client as the target. As said above, we could add the ability to use future<id_type> as the target as well (and I wouldn't object to that). There wouldn't be any semantic differences between the two cases if we introduced it, though. In both cases the target would have to be ready in order for the action to be invoked (naturally).

(2) Use any future<T> or any client<Component> as an argument to any async() or dataflow()
This case is already handled in the same way for both. If those are used with async, the user has to make sure the arguments are ready in order to be usable. If those are used with dataflow, they are guaranteed to be ready at the point when the function is invoked.

Could you explain again where you see inconsistencies?

hkaiser · 2016-04-03T14:55:06Z

client c = new_(id, ...);

(even if this currently would compile and work properly). I'd rather have it explicit:

This looks correct to me though. You are allocating a component and get a client back.

Ok. I'm almost impartial to this.
I guess we could make the conversion future<id_type> --> client<T> explicit.

client c = new_<client >(id, ...);

which already compiles and works as well.

This IMHO is confusing. You are not creating the client but the component.
Similar to shared_ptr, you are allocating a T and not the shared_ptr.

That's what we have right now:

 template <typename Component>
 future<id_type> new_(...);

and

template <typename Client>
Client new_(...);

I'm not opposed to changing this in any way we can reach consensus on, but I believe this is outside the scope of this PR.

hkaiser · 2016-04-03T14:56:28Z

In the end it's a matter of documentation and converting all code to this scheme. In plain C++ you can always write:

void* p = new Foo;

so why shouldn't we allow for

future<id_type> f = new_(id, ...);

?

To enforce strong typing. If you really want that though I'd rather suggest to have a client here instead of future<id_type>.

I like this idea.

sithhell · 2016-04-03T15:02:11Z

We need the future to be ready to know the target where to invoke the action on. The semantic difference to plain future comes from the special handling in async and friends.

Thomas, I think we're talking about different things here. We have two different use cases:

(1) Use a future<id_type> or a client as the target of async()/dataflow()
This PR introduces the functionality allowing to use a client as the target. As said above, we could add the ability to use future<id_type> as the target as well (and I wouldn't object to that). There wouldn't be any semantic differences between the two cases if we introduced it, though. In both cases the target would have to be ready in order for the action to be invoked (naturally).

(2) Use any future or any client as an argument to any async() or dataflow()
This case is already handled in the same way for both. If those are used with async, the user has to make sure the arguments are ready in order to be usable. If those are used with dataflow, they are guaranteed to be ready at the point when the function is invoked.

Could you explain again where you see inconsistencies?

Inconsistency might have been too strong. My point is that this adds
special semantics to future<id_type>/client by essentially
adding special support for implicit unwrapping to use those as the target
for an action. I can see that it has certain benifits. What I'm not sure
is if it's a good idea to have that, as it's an exception to a otherwise
generic rule. Furthermore, I think that clients are implemented using
essentially future<id_type> is an implementation detail. That's why I
proposed to have them expose id_type semantics.

sithhell · 2016-04-03T15:04:27Z

client c = new_(id, ...);

(even if this currently would compile and work properly). I'd rather have it explicit:

This looks correct to me though. You are allocating a component and get a client back.

Ok. I'm almost impartial to this.
I guess we could make the conversion future<id_type> --> client explicit.

client c = new_ >(id, ...);

which already compiles and works as well.

This IMHO is confusing. You are not creating the client but the component.
Similar to shared_ptr, you are allocating a T and not the shared_ptr.

That's what we have right now:

template
future<id_type> new_(...);

and

template
Client new_(...);

I'm not opposed to changing this in any way we can reach consensus on, but I believe this is outside the scope of this PR.

Well, we derailed a bit, but answering those questions might affect further
design decisions, IMHO.

hkaiser · 2016-04-03T15:09:48Z

Inconsistency might have been too strong. My point is that this adds special semantics to future<id_type>/client by essentially adding special support for implicit unwrapping to use those as the target for an action. I can see that it has certain benifits. What I'm not sure is if it's a good idea to have that, as it's an exception to a otherwise generic rule.

Hmmm, do I remember correctly that it was you proposing to implement direct support in async for using clients as targets (including static_asserts ensuring type safety)? What do you suggest now? To remove this again?

hkaiser · 2016-04-03T15:13:43Z

Furthermore, I think that clients are implemented using essentially future<id_type> is an implementation detail.

No! This is not an implementation detail. This is part of the API and their semantics.

That's why I proposed to have them expose id_type semantics.

I'm still not convinced this to be a good idea. As said above, this would make code using them overly complex and would enforce an (implicit) barrier onto any use of clients.

sithhell · 2016-04-03T15:28:02Z

Inconsistency might have been too strong. My point is that this adds special semantics to future/client by essentially adding special support for implicit unwrapping to use those as the target for an action. I can see that it has certain benifits. What I'm not sure is if it's a good idea to have that, as it's an exception to a otherwise generic rule.

Hmmm, do I remember correctly that it was you proposing to implement direct support in async for using clients as targets (including static_asserts ensuring type safety)? What do you suggest now? To remove this again?

You remember correctly, I'm still in favor for that. We absolutely agree, I
guess, that id_type should vanish from any user facing API. Where we
disagree currently is whether this should happen through future<id_type>
like semantics or through id_type like semantics. I can see both having
benefits...

What would a user expect?

sithhell · 2016-04-03T15:34:22Z

Furthermore, I think that clients are implemented using essentially future is an implementation detail.

No! This is not an implementation detail. This is part of the API and their semantics.

That's why I proposed to have them expose id_type semantics.

I'm still not convinced this to be a good idea. As said above, this would make code using them overly complex and would enforce an (implicit) barrier onto any use of clients.

Not if the futurization would happen behind the scene... I understand
though that it might make sense to just expose the future behavior there.
But I think that there is one exception, expressing a locality GID through
a future is probably unnecessary (I might be wrong there). We could also
implement some optimizations for purely local components that don't need
asynchronous remote operations.

hkaiser · 2016-04-03T20:28:36Z

@sithhell I added implicit futurization for using client_base with async. Is that what you had in mind?

sithhell · 2016-04-04T11:13:42Z

hpx/lcos/async.hpp

-            return hpx::detail::async_impl<Action>(launch_policy, c.get_id(),
-                std::forward<Ts>(ts)...);
+            // invoke directly if client is ready
+            if (c.is_ready())


Can't this be part of the client_base::then implementation?

It's an optimization which avoids creating a future<future<>> for the sole purpose of unwrapping it right away. So yes, we could simply call .then() unconditionally if we didn't care about this overhead.

What I meant is to have this optimization directly in client_base. I was under the impression we had the same we have for future<T>::then ... the only optimization we in that regard is here: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/lcos/detail/future_data.hpp#L565

I missed the part that we need to create a new future inside ::then ... sorry for the noise.

sithhell · 2016-04-04T11:29:37Z

@hkaiser yes, this is what I had in mind. Specifically one could hide it behind a trait that looks something like this:

    template <typename Gid, typename Enable>
    struct gid_traits
    {
        static_assert(is_gid<Gid>::value, "Not a Gid type");

        static gid_type get_gid(Gid const & gid);

        static id_type get_id(Gid const & gid);

        template <typename Future>
        static void keep_alive(Gid const & gid, Future & f);

        template <typename F>
        static auto then(Gid & g, F && f);
    };
}}

Note, please see the names gid_traits and is_gid as a placeholder...
The basic idea is that the traits could be either specialized (to do the right thing(tm)) or we could automatically determine what to do based on some SFINAE expression similar to executor_traits. This is completely orthogonal to this PR though, just an idea for further direction and development. This trait can also be used to support hpx::id_type, hpx::gid_type, hpx::future<id_type> and hpx::future<gid_type> in a unified way. See an example implementation (without .then support) here: https://gist.github.com/sithhell/4e28a1678cd7328b5f89ae7eb82e0022.

That being said, I think we mostly agree on all parts and can move on here.

hkaiser · 2016-04-04T11:43:13Z

@sithhell I tried that. Such a trait wouldn't work for distribution policies (for instance colocated(id)).

sithhell · 2016-04-04T11:51:41Z

@sithhell I tried that. Such a trait wouldn't work for distribution policies (for instance colocated(id)).

Why not? What problem did you run into?

hkaiser · 2016-04-04T11:56:31Z

The colocating_policy forwards the async to the target locality through an action-continuation via AGAS. There is no (efficient) way of implementing get_id() (or similar).

sithhell · 2016-04-04T12:00:28Z

The |colocating_policy| forwards the async to the target locality
through an action-continuation via AGAS. There is no (efficient) way of
implementing |get_id()| (or similar).

Ok, why do we want to include the distribution policies into this class?
I would leave the special handling for distribution policies in place
and only consider client like types for this trait.

hkaiser · 2016-04-04T12:04:27Z

The |colocating_policy| forwards the async to the target locality through an action-continuation. There is no (efficient) way of implementing |get_id()| (or similar).

Ok, why do we want to include the distribution policies into this class? I would leave the special handling for distribution policies in place and only consider client like types for this trait.

Well, conceptually those belong into the overload set of such a trait as well. Otherwise you'd have to still dispatch to the trait for 'good' types and have a separate code path for distribution policies.

hkaiser · 2016-04-04T12:24:08Z

[07:22] hkaiser: heller: what about if we go ahead with the PR as is and look at future refactorings separately?
[07:23] heller: yes, that is what I suggested in the end of the post with the PR ;)

hkaiser added type: enhancement category: LCOs category: components labels Jul 1, 2015

hkaiser added this to the 0.9.11 milestone Jul 1, 2015

sithhell reviewed Jul 21, 2015
View reviewed changes

hkaiser modified the milestones: 0.9.11, 0.9.12 Nov 12, 2015

hkaiser force-pushed the generic_client branch 3 times, most recently from b906aa5 to 7e2888b Compare February 24, 2016 02:09

hkaiser added 4 commits March 25, 2016 13:23

Adding generic client

5580267

This is a first step towards fixing 688

30cbced

- renamed all functions `id_type get_gid()` to `get_id` - applied backwards compatibility flag `HPX_HAVE_COMPONENT_GET_GID_COMPATIBILITY` to temporarily still support `get_gid`

Add client support to apply and apply_p

552830c

hkaiser force-pushed the generic_client branch 4 times, most recently from dc36649 to 0f46e9b Compare March 26, 2016 03:37

Finishing implementation of new client_base

6ef4dc2

- fixing rebase conflicts - flyby make_client --> make_clients for version which returns more than one

hkaiser force-pushed the generic_client branch from 0f46e9b to 6ef4dc2 Compare March 26, 2016 13:53

hkaiser added 2 commits March 26, 2016 10:11

Merge branch 'master' into generic_client

8bb7b97

Fixing #includes/merge conflicts

a604a61

hkaiser force-pushed the generic_client branch from 52f49a0 to 8a50cc9 Compare March 27, 2016 00:15

Fixing left-over member access to client_base

1952d3a

- removing obsolete prototype

Removing unneeded boost::mpl::has_xxx in client_base

98899c7

Added implicit futurization to async(client_base(), ...)

37a1349

hkaiser force-pushed the generic_client branch from 72cf2f8 to 37a1349 Compare April 3, 2016 23:43

sithhell reviewed Apr 4, 2016
View reviewed changes

hkaiser merged commit bbcc734 into master Apr 4, 2016

hkaiser deleted the generic_client branch April 4, 2016 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic client #1641

Generic client #1641

hkaiser commented Jul 1, 2015

hkaiser commented Jul 1, 2015

sithhell commented Jul 20, 2015

sithhell Jul 21, 2015

sithhell commented Jul 21, 2015

hkaiser commented Jul 21, 2015

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell Apr 4, 2016

hkaiser Apr 4, 2016

sithhell Apr 4, 2016

sithhell commented Apr 4, 2016

hkaiser commented Apr 4, 2016

sithhell commented Apr 4, 2016

hkaiser commented Apr 4, 2016

sithhell commented Apr 4, 2016

hkaiser commented Apr 4, 2016

hkaiser commented Apr 4, 2016

Generic client #1641

Generic client #1641

Conversation

hkaiser commented Jul 1, 2015

hkaiser commented Jul 1, 2015

sithhell commented Jul 20, 2015

sithhell Jul 21, 2015

Choose a reason for hiding this comment

sithhell commented Jul 21, 2015

hkaiser commented Jul 21, 2015

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell commented Apr 3, 2016

sithhell commented Apr 3, 2016

hkaiser commented Apr 3, 2016

sithhell Apr 4, 2016

Choose a reason for hiding this comment

hkaiser Apr 4, 2016

Choose a reason for hiding this comment

sithhell Apr 4, 2016

Choose a reason for hiding this comment

sithhell commented Apr 4, 2016

hkaiser commented Apr 4, 2016

sithhell commented Apr 4, 2016

hkaiser commented Apr 4, 2016

sithhell commented Apr 4, 2016

hkaiser commented Apr 4, 2016

hkaiser commented Apr 4, 2016