- changed status to resolved
Completion handlers can't accept `&&` references
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&
orT&&
to ensure no copies are made.T const&
is for readonly access, andT&&
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-valueT
also permits this, but comes with the cost of the extra move before the invocation even begins which for types likedouble[1000]
is not a good thing. For types with cheap moves, the difference betweenT
andT&&
is negligible. -
And the TLDR,
T&&
is always the fastest and most permissive, script kiddies go here.
Comments (8)
-
reporter -
- changed status to open
@john bachan : You mention some spec ambiguities above. What do you think needs to be said?
Your point about statically prohibitingoperation_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 callsfuture<T>::then()
repeatedly to schedule multiple callbacks on the samefuture<T>
? In that case the ownership of theT
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
-
reporter 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 passesT&&
(caller intent=input-only), the callee can bind withT
,T const&
,T&&
but notT&
. If the caller passesT&
(caller intent = inout) then the callee can bindT
,T const&
,T&
, but notT&&
. 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 eitherT&&
orT&
, the second makes far morse sense. In the spec, I think all we need to say is that the callback will be passedT&&
.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 strikeT&& 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 afuture<T>
ought to be handled with constructs like out-of-place RMA. Also, the user could always get back future mutability withfuture<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). -
@john bachan I think we are in agreement to remove
future<T>::result_moved()
andfuture<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 asT const &
- does this mean that the common idiom ofmyfut.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 declaringT const & val
?On a closely related topic, the result of
future<T>::result()
is currently specified asT
- are you advocating changing that toT const &
? -
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 aconst
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()
andteam_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 ateam const &
-
reporter A
T const&
can be passed to an argument accepted asT
. 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 aconst&
will only be added to passed-in types which aren't already references (of any kind: &, &&) -
reporter 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 maintainT result()
and addT const& result_reference()
, where thatconst&
is only added to non-& types. -
- changed status to resolved
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.
- Log in to comment
PR 126 permits handlers accepting
T&&
and obeys the per-handler-copy semantics recommended by this issue.