Completion handlers can't accept `&&` references

[john bachan]

Users tempted to write completion handlers which accept the event value by rvalue-reference would be stopped by the compiler with obscure type errors related to std::bind. For instance:

// ok, references handler value in-place as readonly
upcxx::operation_cx::as_lpc(
  upcxx::current_persona(),
  [](some_type const &value) { ... }
);

// ok, gets a copy of handler value (hopefully a moved-out copy thus avoiding performance overheads)
upcxx::operation_cx::as_lpc(
  upcxx::current_persona(),
  [](some_type value) { ... }
);

// User wants ownership of handler value with permission to mutate it or move it out
// Should be ok, but actually DIES with compilation failure mentioning std::bind
upcxx::operation_cx::as_lpc(
  upcxx::current_persona(),
  [](some_type &&value) { ... }
);

// User wants in-out semantics. This should and does die at type check time. Though quality of
// diagnostic is currently poor (type check death goop)
upcxx::operation_cx::as_lpc(
  upcxx::current_persona(),
  [](some_type &value) { ... }
);

I believe the first three of these ought to be allowed. The spec probably also needs some work to guarantee this. Things get interesting when we consider multiple handlers attached to the same event. I think we need to say that each handler gets its own copy of the value. For instance:

operation_cx::as_lpc(persona1, [](T const &value) { ... }) |
operation_cx::as_lpc(persona2, [](T const &value) { ... });

Here a crafty Implementaiton could conceivably use one underlying object and provide its reference to each handler. But that would mean coordinating that the object only gets destructed after both handlers have returned, which could involve multiple threads, and thus hard. It would be easier on the implementation if it were allowed to make copies of the value per handler.

operation_cx::as_lpc(persona1, [](T &&value) { ... }) |
operation_cx::as_lpc(persona2, [](T &&value) { ... });

In this case permitting reference sharing seems confusing and dangerous. Multiple threads could be racing to mutate the object. I think the right semantics are to ensure each handler has an rvalue to its private copy.

So both of these cases argue for per-handler-copies. For performance, the implementation should strive to make sure exactly one of the handlers receives its copy as moved-out from the deserialized source object.

We might also consider performance advice to users on how to decide between T const &, T&&, and T:

• If the value does not need to survive beyond handler invocation, then use either T const& or T&& to ensure no copies are made. T const& is for readonly access, and T&& is in case they want to modify it in place for some reason.

• If the value does need to outlive the handler invocation, then T&& permits them to move it out into another object. By-value T also permits this, but comes with the cost of the extra move before the invocation even begins which for types like double[1000] is not a good thing. For types with cheap moves, the difference between T and T&& is negligible.

• And the TLDR, T&& is always the fastest and most permissive, script kiddies go here.

[john bachan]

PR 126 permits handlers accepting T&& and obeys the per-handler-copy semantics recommended by this issue.

[Dan Bonachea]

@john bachan : You mention some spec ambiguities above. What do you think needs to be said?
Your point about statically prohibiting operation_cx::as_lpc( persona, [](T&value) { ... } ) is not obvious to me, and definitely not currently specified as a constraint.

Also, your discussion above concentrates on operation_cx::as_lpc() callbacks, but aren't there similar semantic questions (and potential bugs) with value ownership if the application calls future<T>::then() repeatedly to schedule multiple callbacks on the same future<T>? In that case the ownership of the T embedded in the corresponding promise is semantically visible, but we should probably clarify which callback signatures are permitted, what they mean and when we are copying/moving that object to run chained callbacks..

I'm reopening this issue temporarily pending the creation of that spec issue, to ensure we don't lose this task.

CC: @Amir Kamil

[john bachan]

With regard to prohibiting handlers from accepting T& arguments, C++ has taken the stance to restrict how rvalues and lvalues can be bound arguments specifically to guard against errors of user intent. If the caller passes T&& (caller intent=input-only), the callee can bind with T,T const&,T&& but not T&. If the caller passes T& (caller intent = inout) then the callee can bind T,T const&,T&, but not T&&. We in the implementation have to chose which caller regime matches our intent, which for these callbacks is clearly passing as input-only (T&&). There is no reference type the caller can pass which will bind universally (sfinae can hack around this but I strongly advise against skirting the established idiom) so we have to choose disallowing either T&& or T&, the second makes far morse sense. In the spec, I think all we need to say is that the callback will be passed T&&.

Future callbacks are related but different. Our futures were designed to be shared (ref-counted) objects since I believed that made DAG-buillding more ergonomic. Immutability is very important in keeping the under-the-hood sharing semantically sane. We offer no way for the user to even determine whether two future's share the same underlying object, so for a user to safely mutate the value in a future they would have to prove by the future's construction that the data wasn't being aliased (they would have to know that this future was backed by its own promise). Given this, I would argue that futures pass their data as T const& to prohibit mutation. I think we should also strike T&& future<T>::result_moved() and friends from the spec. I introduced those mutable backdoors in case performance demanded them, but when looking at the design of upcxx in total, anything too expensive to be put in a future<T> ought to be handled with constructs like out-of-place RMA. Also, the user could always get back future mutability with future<std::unique_ptr<T>>. By removing all mutable access from futures, we save ourselves the burden of spec'ing when future may or may not alias (which is complicated).

[Dan Bonachea]

@john bachan I think we are in agreement to remove future<T>::result_moved() and future<T>::wait_moved(), this is now in spec PR #26.

We don't currently have spec language to clarify how T's are passed to callbacks, but that seems to clearly be called for (pun intended).

I'm worried by your proposal that futures pass their data to .then() callbacks as T const & - does this mean that the common idiom of myfut.then([](T val) { ... } would be rejected at compile time due to a lacking const? (if so, I'd see that as a major impediment) Or does it just mean such a callback will potentially impose a copy-in that the user could eliminate by declaring T const & val?

On a closely related topic, the result of future<T>::result() is currently specified as T - are you advocating changing that to T const &?

[Dan Bonachea]

I'm worried by your proposal that futures pass their data to .then() callbacks as T const &

I'm especially worried by the case of future<T&>::then([](T &gimme_my_mutable_T) { ... } ), where the user has explicitly created a future to wrap a reference to a mutable object, and we slap a const on there and break his handoff.

Lest you think that futures containing reference types is a silly case, note that dist_id<T>::when_here() and team_id::when_here() both return such objects. A user might reasonably want to do something like:

team_id my_team_id = ...;
my_team_id.when_here().then([](team &t) { 
   rpc_ff(t, 0, []() { cout << "hello team leader" << endl; } 
 })

which I believe would fail to compile if future <team &>::then() passes a team const &

[john bachan]

A T const& can be passed to an argument accepted as T. When a callee accepts such an argument by-value, they are demanding their own copy be made.

future<U&>.then() definitely requires special handling. We should take the stance that a const& will only be added to passed-in types which aren't already references (of any kind: &, &&)

[john bachan]

T const& future<T>::result() is a little scary because it could lead to dangling references. Consider:

T const &val = make_future<T>(...).result();
// uh-oh, the future was destroyed

Fortunately the user was sort of asking for it by declaring val a reference. And good news, auto val = (...).result() will infer val's type to be T (non &) regardless if result returns a &-type or not. I think the proper thing to do is maintain T result() and add T const& result_reference(), where that const& is only added to non-& types.

[Dan Bonachea]

This issue was originally opened to document a bug fixed in pull request #124

The spec issues raised in the follow-on discussion have been relocated to the spec tracker:

See Spec issue 149

See Spec issue 150

So this implementation-centric issue is now resolved.