Specify types and qualifiers permitted/expected on callback arguments

Issue #149 resolved
Dan Bonachea created an issue

Overview

Discussion below is copied from implementation issue #268

Currently the spec is underspecified with respect to const/reference qualifiers on arguments to user-supplied callbacks that are invoked by a value-producing completion. 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).

Variants of this problem affect at least:

  • future<T>::then()
  • operation_cx::as_lpc()
  • rpc and rpc_ff

future<T>::then()

Currently the spec says almost nothing formal about how the arguments are passed to func in future <T...>::then(Func func). The prose description provides lots of examples of using .then(), but doesn't explicitly provide all the preconditions and effects in a formal way. For example, it fails to even specify that func must be callable with the types in T... (which is clearly the intent, and should be made explicit in the API reference).

John advocates that futures should pass their data to .then() callbacks as T const &. This basically means .then is providing each value - which is stored in the (potentially implicit) promise - as a read-only, input-only reference argument to the callback. Callback arguments can still be declared as T, but that implies a copy is made at the call site to provide that mutable argument value (where the copy could have some noticeable cost for large objects).

I think I'm in agreement, with the caveat the reference types need special handling:

Dan said:

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 said:

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: &, &&)

Side note: the current precondition to .then():

The call func() must not throw an exception.

is ill-formed for any non-empty future (ie probably won't even type check). This should be corrected to indicate the precondition applies to the call with the actual arguments.

operation_cx::as_lpc()

The situation is slightly different for operation_cx::as_lpc(), because the value is provided by operation completion and thus always resides in anonymous runtime-owned memory, and the only access to that completion's value is the callback argument itself (ignoring for the moment that duplicate completions may provide a copy of the value in a promise/future, where the value can be retrieved by query calls).

Examples:

// 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
// Was broken in 2019.3.2 and fixed in PR126
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) { ... }
);

John believes the first three should be specified as permitted, and the fourth should be prohibited (where the completion value type some_type is not already a reference). This might be as simple as specifying that the completion value of type some_type is passed as a some_type && (unless it is already a reference type).

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.

The spec might also need to say something about when multiple completions are registered to receive the same completion value, the runtime is permitted to copy the value once per handler callback (or more likely, once per completion entry).

RPC

Similar issues also affect arguments to RPC callbacks invoked by rpc and rpc_ff.

Here we say a bit more, as the 2019.9.0 spec says:

After their receipt on the target, the data are deserialized and func(args...) is enlisted for execution during user-level progress of the master persona

but is this literally the call the runtime is invoking? In particular, the args... referred to above are NOT the literal arguments the caller provided in invoking the declared signature, but versions of those arguments whose values have been serialized, transmitted across the network and deserialized (and in the case of team and dist_object had other special-case transformations described in Sec 6.4). Are we actually saying the runtime will invoke the callback using the deserialized values passed through the same declared types of those arguments in ...Args? Or do we mean the reference-ified pack used to declare the formal arguments to the call Args &&...args? These details are especially important when a client-provided argument type includes a reference, because serialization means the reference passed to the callback is NOT a reference to the same object referenced by the initiator, but rather a reference to a runtime-owned copy created during deserialization.

The text currently in serialization pull request #20 adds some detail, but is still vague wrt reference types:

the result of serializing and deserializing the function object (a value of type \code{deserialized_type_t<Func>}) must be invocable on the values that result from serializing and deserializing the arguments (values that are of the types in \code{deserialized_type_t<Args>...}), and the invocation must not throw an exception.
[...]
After their receipt on the target, the data are deserialized and the invocation of the deserialized function object on the deserialized arguments is enlisted for execution during user-level progress of the master persona

This vagueness was necessarily added to support asymmetric deserialization, since the deserialized type of each argument used to invoke the callback may be completely different from the corresponding input argument to the rpc call. Unfortunately, this has gotten us even farther from a clear specification of the types used to invoke the RPC callback.

Comments (5)

  1. Dan Bonachea reporter

    I was just stung by this problem while writing a simple exercise, and (even though I was already aware of this spec ambiguity) spent an hour tracking it down to essentially this code:

    int x = 42;
    future<> f = upcxx::rpc(0, [](int &val) { }, x);
    

    which currently generates an ocean of inscrutable template argument deduction/substitution failures in the rpc headers, all caused by the reference qualifier on the RPC argument int &val. (As @john bachan said, "quality of diagnostic is currently poor (type check death goop)")

    In addition to closing the spec ambiguities, is it possible to improve the quality of diagnostic for this case we consider problematic? I don't find it realistic for any user to figure out from the current messages that what he's missing to make this work is a const qualifier...

  2. Max Grossman

    Hit this source of confusion/uncertainty while developing serialization examples.

    I would argue that the introduction of serialization increases the priority of this issue, as we’re enabling users to write more code that passes a wider variety of objects to rpc(). This makes things more confusing, and makes users think they’re just using serialization incorrectly.

  3. Log in to comment