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.

Sign up for GitHub

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

Jump to bottom

Allowing component clients as action return types #2184

Merged
merged 1 commit into from
May 29, 2016
Merged

Allowing component clients as action return types #2184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions hpx/lcos/async_continue_fwd.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

#include <hpx/config.hpp>
#include <hpx/traits/extract_action.hpp>
#include <hpx/runtime/actions/detail/remote_action_result.hpp>
#include <hpx/traits/action_remote_result.hpp>
#include <hpx/util/decay.hpp>
#include <hpx/util/result_of.hpp>
#include <hpx/lcos/future.hpp>
Expand All @@ -26,7 +26,7 @@ namespace hpx
{
template <typename Action, typename Cont>
struct result_of_async_continue
: actions::detail::remote_action_result<
: traits::action_remote_result<
typename util::result_of<typename util::decay<Cont>::type(
naming::id_type,
typename hpx::traits::extract_action<
Expand Down
4 changes: 4 additions & 0 deletions hpx/runtime/actions/action_invoke_no_more_than.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <hpx/runtime/actions/continuation.hpp>
#include <hpx/lcos/local/spinlock.hpp>
#include <hpx/lcos/local/counting_semaphore.hpp>
#include <hpx/util/assert.hpp>
#include <hpx/util/static.hpp>
#include <hpx/util/bind.hpp>
#include <hpx/traits/is_future.hpp>
Expand Down Expand Up @@ -170,8 +171,11 @@ namespace hpx { namespace actions { namespace detail
void trigger_value(remote_result_type && result)
{
if(cont_)
{
HPX_ASSERT(0 != dynamic_cast<base_type *>(cont_.get()));
static_cast<base_type *>(cont_.get())->
trigger_value(std::move(result));
}
construct_semaphore_type::get_sem().signal();
}

Expand Down
69 changes: 36 additions & 33 deletions hpx/runtime/actions/action_support.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
#include <hpx/lcos/future.hpp>
#include <hpx/runtime/parcelset_fwd.hpp>
#include <hpx/runtime/actions/continuation.hpp>
#include <hpx/runtime/actions/detail/remote_action_result.hpp>
#include <hpx/runtime/serialization/output_archive.hpp>
#include <hpx/runtime/serialization/input_archive.hpp>
#include <hpx/runtime/serialization/base_object.hpp>
#include <hpx/runtime/threads/thread_helpers.hpp>
#include <hpx/runtime/threads/thread_init_data.hpp>
#include <hpx/runtime/components/pinned_ptr.hpp>
#include <hpx/traits/action_remote_result.hpp>
#include <hpx/traits/is_bitwise_serializable.hpp>
#include <hpx/util/tuple.hpp>
#include <hpx/util/detail/count_num_args.hpp>
Expand Down Expand Up @@ -80,6 +80,41 @@ namespace hpx { namespace traits
hpx::actions::detail::action_serialization_data>
: boost::mpl::true_
{};

namespace detail
{
///////////////////////////////////////////////////////////////////////
// If an action returns a future, we need to do special things
template <>
struct action_remote_result_customization_point<void>
{
typedef util::unused_type type;
};

template <typename Result>
struct action_remote_result_customization_point<lcos::future<Result> >
{
typedef Result type;
};

template <>
struct action_remote_result_customization_point<lcos::future<void> >
{
typedef hpx::util::unused_type type;
};

template <typename Result>
struct action_remote_result_customization_point<lcos::shared_future<Result> >
{
typedef Result type;
};

template <>
struct action_remote_result_customization_point<lcos::shared_future<void> >
{
typedef hpx::util::unused_type type;
};
}
}}

/// \endcond
Expand Down Expand Up @@ -111,38 +146,6 @@ namespace hpx { namespace actions
return util::type_id<Action>::typeid_.type_id();
}
#endif

///////////////////////////////////////////////////////////////////////
// If an action returns a future, we need to do special things
template <>
struct remote_action_result<void>
{
typedef util::unused_type type;
};

template <typename Result>
struct remote_action_result<lcos::future<Result> >
{
typedef Result type;
};

template <>
struct remote_action_result<lcos::future<void> >
{
typedef hpx::util::unused_type type;
};

template <typename Result>
struct remote_action_result<lcos::shared_future<Result> >
{
typedef Result type;
};

template <>
struct remote_action_result<lcos::shared_future<void> >
{
typedef hpx::util::unused_type type;
};
}

///////////////////////////////////////////////////////////////////////////
Expand Down
13 changes: 6 additions & 7 deletions hpx/runtime/actions/basic_action.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <hpx/runtime/threads/thread_data_fwd.hpp>
#include <hpx/runtime/threads/thread_enums.hpp>
#include <hpx/traits/action_decorate_function.hpp>
#include <hpx/traits/action_remote_result.hpp>
#include <hpx/traits/is_distribution_policy.hpp>
#include <hpx/traits/is_future.hpp>
#include <hpx/util/bind.hpp>
Expand Down Expand Up @@ -131,14 +132,12 @@ namespace hpx { namespace actions
typedef Derived derived_type;

// result_type represents the type returned when invoking operator()
typedef
typename traits::promise_local_result<
R
>::type
result_type;
typedef typename traits::promise_local_result<R>::type result_type;

// The remote_result_type is the remote type for the type_continuation
typedef typename detail::remote_action_result<R>::type remote_result_type;
// The remote_result_type is the local type for the type_continuation
typedef typename traits::action_remote_result<R>::type remote_result_type;

// The local_result_type is the local type for the type_continuation
typedef
typename traits::promise_local_result<
remote_result_type
Expand Down
57 changes: 41 additions & 16 deletions hpx/runtime/actions/continuation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <hpx/runtime/actions/action_priority.hpp>
#include <hpx/runtime/actions/basic_action_fwd.hpp>
#include <hpx/runtime/actions/continuation_fwd.hpp>
#include <hpx/runtime/actions/detail/remote_action_result.hpp>
#include <hpx/runtime/agas/interface.hpp>
#include <hpx/runtime/applier/detail/apply_implementations_fwd.hpp>
#include <hpx/runtime/find_here.hpp>
Expand All @@ -27,6 +26,7 @@
#include <hpx/util/demangle_helper.hpp>
#include <hpx/util/result_of.hpp>
#include <hpx/util/unique_function.hpp>
#include <hpx/traits/action_remote_result.hpp>
#include <hpx/traits/is_action.hpp>
#include <hpx/traits/is_callable.hpp>
#include <hpx/traits/is_continuation.hpp>
Expand Down Expand Up @@ -332,8 +332,16 @@ namespace hpx { namespace actions
{
try {
HPX_ASSERT(result.is_ready());
static_cast<typed_continuation<Result, RemoteResult>*>(cont.get())->
trigger(result.get());
HPX_ASSERT((0 !=
dynamic_cast<
typed_continuation<Result, RemoteResult>*
>(cont.get())));

static_cast<
typed_continuation<Result, RemoteResult>*
>(cont.get())->trigger(
hpx::util::detail::decay_copy(result.get())
);
}
catch (...) {
// make sure hpx::exceptions are propagated back to the client
Expand All @@ -359,17 +367,14 @@ namespace hpx { namespace actions
void trigger_impl_future(boost::mpl::true_,
std::unique_ptr<continuation> cont, F&& f, Ts&&... vs)
{
typedef typename traits::future_traits<Future>::type type;
typedef
typename traits::future_traits<
Future
>::type type;
typedef
typename detail::remote_action_result<type>::type
typename traits::action_remote_result<type>::type
remote_result_type;
typedef typename std::is_void<type>::type is_void;

Future result = util::invoke(std::forward<F>(f),
std::forward<Ts>(vs)...);
std::forward<Ts>(vs)...);

if(result.is_ready())
{
Expand All @@ -396,9 +401,15 @@ namespace hpx { namespace actions
std::unique_ptr<continuation> cont, F&& f, Ts&&... vs)
{
try {
static_cast<typed_continuation<Result, RemoteResult>*>(cont.get())->
trigger(util::invoke(std::forward<F>(f),
std::forward<Ts>(vs)...));
HPX_ASSERT((0 !=
dynamic_cast<
typed_continuation<Result, RemoteResult>*
>(cont.get())));

static_cast<
typed_continuation<Result, RemoteResult>*
>(cont.get())->trigger(
util::invoke(std::forward<F>(f), std::forward<Ts>(vs)...));
}
catch (...) {
// make sure hpx::exceptions are propagated back to the client
Expand All @@ -422,12 +433,12 @@ namespace hpx { namespace actions
template <typename Result, typename F, typename ...Ts>
void trigger(std::unique_ptr<continuation> cont, F&& f, Ts&&... vs)
{
typedef typename util::result_of<F(Ts...)>::type
result_type;
typedef typename util::result_of<F(Ts...)>::type result_type;

typedef
typename std::is_same<result_type, util::unused_type>::type
is_void;

detail::trigger_impl<Result, result_type>(is_void(), std::move(cont),
std::forward<F>(f), std::forward<Ts>(vs)...);
}
Expand Down Expand Up @@ -896,6 +907,11 @@ namespace hpx { namespace actions
this->trigger();
}

virtual void trigger_value(util::unused_type const&)
{
this->trigger();
}

private:
char const* get_continuation_name() const
{
Expand Down Expand Up @@ -925,10 +941,19 @@ namespace hpx { namespace actions
template <typename Arg0>
void continuation::trigger(Arg0 && arg0)
{
// The dynamic cast decays the argument type to avoid the assert firing
// for cases when Arg0 is a const&. This does not make the code invalid
// as trigger_value (which is a virtual function) takes its argument
// by && anyways.
HPX_ASSERT(0 != dynamic_cast<
typed_continuation<typename util::decay<Arg0>::type> *
>(this));

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we decay in the static_cast as well? It's about the correct dynamic type of the continuation, not how the argument is passed to 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.

Unfortunately that will not work. If Arg0 is a const&, a static_cast to the decayed type will fail compiling as the trigger_value will expect a pure rvalue.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand the issue here completely. The dynamic cast fails if the type is not decayed, that means that the dynamic type is with the decayed type, right?
On the other hand, the compilation fails when using the static_cast with the decayed type?
This sounds like there is something very fishy going on there.

Copy link
Member Author

@hkaiser hkaiser May 27, 2016 •
edited

Choose a reason for hiding this comment

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

It's difficult to explain. Let's assume Arg0 is deduced as id_type const&. In this case, the dynamic_cast<Arg0>(this) would fail as this is not of type typed_continuation<id_type const&> but of type typed_continuation<id_type>. Hence the decayed argument.

At the same time, if we statically casted this to typed_continuation<id_type>, then its member function trigger_value effectively would have the signature trigger_value(id_type&&) causing compilation problems (remember, trigger_value is a virtual function and we have no way to use a forwarding reference here - which would solve the issue).

One solution would be to have another virtual function trigger_value(Result const&) which would be used by the compiler in this case. Adding this however causes issues whenever Arg0 represents a move-only type (we can't 'disable' the virtual function from the type for this case).

This code is fishy as it does a static_cast<typed_continuation<id_type const&>>(this) where this is actually a typed_continuation<id_type>. This however should not be a problem as for both types the effectively generated class is 100% identical in layout and member function signatures.

Please let me know if you have any other suggestion on how to resolve the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Yuck...

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't it be typed_continuation<id_type> in the first place?

That's what I said. this points to an object of that type.

Do we really need forwarding references here? Aren't rvalue references enough? set_lco_value accepts rvalue references iirc.

That does not work if the function is called with a const&, didn't I mention that?

The easiest would be if you changed the static_cast to use the decayed type yourself. That would demonstrate the problem right away.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll play with it a little bit... the sanitizer should still complain about that code.

Right, it most likely will complain. But you could tell it to ignore this instance.

Copy link
Member

Choose a reason for hiding this comment

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

If you decay copy, everything works, here is the patch that works for me:
https://gist.github.com/sithhell/7c4f73280b236662e448b571a9218030

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, thanks.

// The static_cast is safe as we know that Arg0 is the result type
// of the executed action (see apply.hpp).
static_cast<typed_continuation<Arg0> *>(this)->trigger_value(
std::forward<Arg0>(arg0));
static_cast<
typed_continuation<typename util::decay<Arg0>::type> *
>(this)->trigger_value(std::forward<Arg0>(arg0));
}
}}

Expand Down
21 changes: 0 additions & 21 deletions hpx/runtime/actions/detail/remote_action_result.hpp
View file
Open in desktop

This file was deleted.

9 changes: 9 additions & 0 deletions hpx/runtime/components/client_base.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <hpx/config.hpp>
#include <hpx/throw_exception.hpp>
#include <hpx/traits/action_remote_result.hpp>
#include <hpx/traits/is_client.hpp>
#include <hpx/traits/is_future.hpp>
#include <hpx/traits/future_access.hpp>
Expand Down Expand Up @@ -121,6 +122,14 @@ namespace hpx { namespace traits
typename std::enable_if<is_client<Derived>::value>::type>
: shared_state_ptr<typename traits::future_traits<Derived>::type>
{};

///////////////////////////////////////////////////////////////////////
template <typename Derived>
struct action_remote_result_customization_point<Derived,
typename std::enable_if<is_client<Derived>::value>::type>
{
typedef id_type type;
};
}
}}

Expand Down
5 changes: 4 additions & 1 deletion hpx/runtime/trigger_lco.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,10 @@ namespace hpx
/// send all credits in \a id along with the generated
/// message. The default value is \a true.
template <typename Result>
void set_lco_value(naming::id_type const& id, Result && t,
typename std::enable_if<
!std::is_same<typename util::decay<Result>::type, naming::address>::value
>::type
set_lco_value(naming::id_type const& id, Result && t,
naming::id_type const& cont, bool move_credits = true)
{
set_lco_value(id, naming::address(), std::forward<Result>(t), cont,
Expand Down
28 changes: 28 additions & 0 deletions hpx/traits/action_remote_result.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) 2007-2014 Hartmut Kaiser
// Copyright (c) 2011 Bryce Lelbach
// Copyright (c) 2011 Thomas Heller
//
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#ifndef HPX_TRAITS_ACTION_REMOTE_RESULT_HPP
#define HPX_TRAITS_ACTION_REMOTE_RESULT_HPP

namespace hpx { namespace traits
{
namespace detail
{
template <typename Result, typename Enable = void>
struct action_remote_result_customization_point
{
typedef Result type;
};
}

template <typename Result>
struct action_remote_result
: detail::action_remote_result_customization_point<Result>
{};
}}

#endif
1 change: 1 addition & 0 deletions tests/regressions/components/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

set(tests
moveonly_constructor_arguments_1405
returned_client_2150
)

foreach(test ${tests})
Expand Down