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

Enable returning future<R> from actions where R is not default-constructible #2850

Merged
merged 7 commits into from Sep 2, 2017

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Aug 20, 2017

This fixes #2847.

@krivenko: please verify whether this solves the issue you reported.

Note: this requires #2849 to be merged first

- this fixes #2846
- flyby: fixed a couple of warnings in tests
…uctible

- this fixes #2847
- this also removes a long-standing inconsistency where hpx::async<Action>
  was implicitly unwrapping a returned future<T>. hpx::async<Action> now
  behaves like any other hpx::async, e.g. if the action itself returns a
  future<R>, hpx::async<Action>() will return a future<future<R>>.

NOTE: this is possibly a breaking change!
@hkaiser hkaiser added this to the 1.1.0 milestone Aug 20, 2017
@hkaiser hkaiser changed the title Unify serialization of non-default-constructable types Enable returning future<R> from actions where R is not default-constructible Aug 20, 2017
@krivenko
Copy link

Hm... I'm a bit confused by this change.
Am I allowed to define my action as

non_default_ctor plain_future_non_default_ctor()
{
    return non_default_ctor(42);
}

and then apply it as

hpx::future<non_default_ctor> result = hpx::async<plain_future_non_default_ctor_action>(id);

?

This code does not compile with fixing_2847 branch.

hpx.git/hpx/util/tuple.hpp:72:24: error: use of deleted function ‘non_default_ctor::non_default_ctor()’
               : _value()
                        ^
hpx.git/tests/regressions/actions/return_future_2847.cpp:19:5: note: declared here
     non_default_ctor() = delete;
hpx.git/hpx/lcos/base_lco_with_value.hpp:85:22: error: use of deleted function ‘non_default_ctor::non_default_ctor()’
             set_value(RemoteResult());
                      ^
hpx.git/tests/regressions/actions/return_future_2847.cpp:19:5: note: declared here
     non_default_ctor() = delete;

@hkaiser
Copy link
Member Author

hkaiser commented Aug 20, 2017

@krivenko: in the test you added the action returned a future<R> where R was not default-constructible. That test passes now. I will see what I can do for this new test case, however.

@krivenko
Copy link

There is a problem in the latest commit...

hpx.git/hpx/util/tuple.hpp:72:17: error: declaration of ‘class Enable’
                 typename Enable = typename std::enable_if<
                 ^
hpx.git/hpx/util/tuple.hpp:67:46: error:  shadows template parm ‘class Enable’
         template <std::size_t I, typename T, typename Enable = void>

@hkaiser
Copy link
Member Author

hkaiser commented Aug 21, 2017

@krivenko: ok, should be good now. This new commit enables using non-default constructible types as action arguments and action return types.

@krivenko
Copy link

A minor compatibility issue still remains in transfer_base_action.hpp.
C++11 does not allow constexpr functions to have more than one statement in their bodies.

hpx.git/hpx/runtime/actions/transfer_base_action.hpp:104:5: error: body of constexpr function ‘constexpr typename hpx::util::tuple_element<I, T>::type&& hpx::util::get(hpx::actions::detail::argument_holder<Args>&&) [with long unsigned int I = 0ul; Args = hpx::util::tuple<non_default_ctor>; typename hpx::util::tuple_element<I, T>::type = non_default_ctor]’ not a return-statement
     }
     ^
hpx.git/hpx/runtime/actions/transfer_base_action.hpp:85:5: error: body of constexpr function ‘constexpr typename hpx::util::tuple_element<I, T>::type& hpx::util::get(hpx::actions::detail::argument_holder<Args>&) [with long unsigned int I = 0ul; Args = hpx::util::tuple<non_default_ctor>; typename hpx::util::tuple_element<I, T>::type = non_default_ctor]’ not a return-statement
     }

@krivenko
Copy link

Still not quite there...

hpx.git/hpx/runtime/actions/transfer_base_action.hpp:78:25: error: ‘constexpr const Args& hpx::actions::detail::argument_holder<Args>::data() const’ cannot be overloaded
             Args const& data() const
                         ^
hpx.git/hpx/runtime/actions/transfer_base_action.hpp:73:19: error: with ‘constexpr Args& hpx::actions::detail::argument_holder<Args>::data() const’
             Args& data()

@krivenko
Copy link

hpx.git/hpx/runtime/actions/transfer_base_action.hpp:94:35: error: ‘std::unique_ptr<hpx::util::tuple<non_default_ctor>, std::default_delete<hpx::util::tuple<non_default_ctor> > > hpx::actions::detail::argument_holder<hpx::util::tuple<non_default_ctor> >::data_’ is private
             std::unique_ptr<Args> data_;
                                   ^
hpx.git/hpx/runtime/actions/transfer_base_action.hpp:123:26: error: within this context
             util::get<I>(*t.data_));
                          ^

And a similar error in hpx.git/hpx/runtime/actions/transfer_base_action.hpp:106.

@hkaiser
Copy link
Member Author

hkaiser commented Aug 21, 2017

@krivenko: darn - not my day today...

@krivenko
Copy link

@hkaiser I actually feel sorry for giving you a hard time...

Anyway, all relevant tests successfully compile and run now.
Thank you for implementing this new feature! I very much appreciate your efforts.

void set_event_nonvirt(std::false_type)
{
// this shouldn't ever be called
HPX_ASSERT(false);
Copy link
Member

Choose a reason for hiding this comment

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

I think an exception would be more appropriate here, I guess. The ideal solution would be to disable set_event for non default constructible types alltogether, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll change it to an exception. I was debating with myself about this but then decided to go with an assert as if this fires it wouldn't be a user error but a problem in our code.

Copy link
Member

Choose a reason for hiding this comment

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

I think it might just as well be a user error. base_lco_with_value can be used in user code as well, so it might be good to have good diagnostics 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.

That's done now.

///////////////////////////////////////////////////////////////////////////
// If one or more arguments of the action are non-default-constructible,
// the transfer_action does not store the argument tuple directly but a
// unique_ptr to the tuple instead.
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't a better solution be to enable the non-default construction mechanism for tuple instead of adding this extra level of indirection?

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 current code relies on default-constructing the transfer_action (and transfer_continuation_action) and I was not able to find a way around that (see https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/runtime/actions/detail/action_factory.hpp#L84-L91). The indirection was added to allow to keep those as is, changing the tuple wouldn't help.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, tuple now can be serialized even if not default-constructible, see: https://github.com/STEllAR-GROUP/hpx/blob/fixing_2847/hpx/util/tuple.hpp#L1103-L1121

Copy link
Member

Choose a reason for hiding this comment

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

So why do we need that extra wrapper then?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said - the transfer_actions have to be default-constructed... Feel free to find a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't look into it. I'm a bit concerned that this is adding an allocation in a critical path (sometimes) penalizing the whole thing if one isn't default constructible

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 decided to risk adding one allocation for the case when the type is not default-constructible. This shouldn't happen more than in 1% of all use cases, thus the effect wouldn't be measurable. Let's go ahead with what's proposed and fix it if we come across a real show-stopper.

// default fall-backs for serializing non-default-constructible types
template <typename Archive, typename T>
void save_construct_data(Archive&, T const*, unsigned)
{
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 why this is needed. During saving, we don't need to construct anything, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Symmetry and compatibility with Boost serialization. The rationale is that that the serialize function should be used to serialize the internal data of a class and the *_construct_data are used to serialize any external data that might be used for constructing an object of that class, so that containers would be able to deserialize them.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, my point is that during save, you should not construct anything, so this symmetry looks odd, since it's a noop anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only a noop if there is no 'external' data needed to construct the object during load time.

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 name is a bit misleading, I agree - but that's how Boost named the functions...

Copy link
Member

Choose a reason for hiding this comment

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

I have no objection against the load_construct function. This is called during save time though. I don't think we need to keep compatibility with boost here.

Copy link
Member Author

@hkaiser hkaiser Aug 22, 2017

Choose a reason for hiding this comment

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

You lost me, sorry. What's your problem? The symmetry (needed if load_construct_data is to extract things from the archive)? The name (save_construct_data)? Or the Boost compatibility?

template <typename Archive, typename T>
void load_construct_data(Archive& ar, T* t, unsigned)
{
::new (t) T;
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 this also load *t from the archive?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above. Even if load_construct_data is used to instantiate the object, it will still use serialize for loading the data.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I would assume, that you load all ctor arguments in that function that are needed to construct the object. After that, the object should be complete and should not require any additional serialization. I was under the impression that load_construct_data completely replaces the regular serialize function.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not replace the 'normal' serialize functions. This might be needed especially in cases where vital data is not publicly accessible.

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that an appropriate ctor call should be enough for same types. But sure, there's no reason why one should arbitralily limit it here.

>::type
call(output_archive& ar, T const& val)
{
save_construct_data(ar, &val, 0);
Copy link
Member

Choose a reason for hiding this comment

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

What's the rationale to have this additional call?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above. The reasons are mostly symmetry (if load_construct_data expects to read stuff from the archive this should be written in save_construct_data) and compatibility with Boost serialization where this technique is used for serializing containers and pointers of/to non-default-constructible types.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, if you look at it that way, it makes some sense.

Copy link
Member

@sithhell sithhell left a comment

Choose a reason for hiding this comment

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

Let's go ahead with this. This also supersedes #2849.

@hkaiser hkaiser deleted the fixing_2847 branch September 2, 2017 18:16
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.

Returning a future<R> where R is not default-constructable broken
3 participants