Move semantics for distributed objects

[Amir Kamil]

Now that issue 189 is resolved, resource object types support both move construction and assignment, and they have a notion of whether an object is active or not. Distributed objects are somewhat semantically different than these resource object types:

• The “resource” is an unboxed T held directly within the distributed object, plus a collective name.
• T may or may not have relevant concepts of DefaultConstructible, MoveConstructible, MoveAssignable, etc.

• Distributed objects are destroyed implicitly rather than through an explicit destroy().

  • Unlike resource object types, destruction is not collective.

• It is currently impossible for a distributed object to reach an “inactive” state.

Despite these differences, we suspect that users do want to construct distributed objects before UPC++ is initialized, much as they might want to do with resource object types. Since such a construction cannot be collective, we need a mechanism for constructing a distributed object in some sort of inactive state, and to later make it active.

The following are some options for accomplishing this:

• Modify distributed objects to hold the T value indirectly, so that an inactive object holds a null pointer while an active holds a T on the heap (private or shared?).
• Allow distributed objects to be constructed in an “unregistered” state, where the underlying T is constructed but the collective name has not been assigned. Add APIs for registering and unregistering a distributed object.
• Allow distributed objects to be created in an “unconstructed” state. Use something like a std::optional<T> to hold the underlying value. Possibly add an API for tearing down the caller’s representative of a distributed object, which would move the object to the unconstructed state.

In the first case, distributed objects would be unconditionally DefaultConstructible, MoveConstructible, and MoveAssignable. The tradeoff is the cost of the indirection when a distributed object is dereferenced. In the other cases, they would only have the subset of these concepts that T has.

Other options?

[Dan Bonachea]

Mass roll-over of open issues to next release milestone

[Dan Bonachea]

My thoughts on this:

• Modify distributed objects to hold the T value indirectly, so that an inactive object holds a null pointer while an active holds a T on the heap (private or shared?).

I'm opposed to this approach, because it adds an unavoidable overhead to the use of dist_object. More importantly, it doesn't technically add any expressive power. In particular, users can already construct a dist_object<T> *p = nullptr; before UPC++ library init, treat a null pointer as "inactive", and "activate" it by calling p = new dist_object<T>(...). IOW if users are willing to pay the overhead of the extra level of indirection, they can already get that with current dist_object and a few additional characters. We should not force all dist_object users to pay this allocation/indirection cost (especially lacking a real gain in expressiveness).

• Allow distributed objects to be constructed in an “unregistered” state, where the underlying T is constructed but the collective name has not been assigned. Add APIs for registering and unregistering a distributed object.
• Allow distributed objects to be created in an “unconstructed” state. Use something like a std::optional<T> to hold the underlying value. Possibly add an API for tearing down the caller’s representative of a distributed object, which would move the object to the unconstructed state.

Both of these approaches seem like viable options to me, and could potentially both be provided as orthogonal boolean states of operation: where the "registered" state is a precondition for dist_object::id(), dist_object::fetch() or passing as an RPC argument, and the "constructed" state is a precondition for dist_object::operator*() and dist_object::operator->() that access the contained value. Registration needs to remain collective and we currently couple that to dist_object construction (which should remain the default for legacy reasons), but we could provide non-collective dist_object constructor(s) that defer registration to a later collective register(team = world()) call. Similarly we could provide dist_object constructor(s) that leave the value in an unconstructed state for a later dist_object<T>::emplace(args...) or dist_object<T>::operator=(T&&).

I think at runtime this boils down to adding a boolean constructed state bit in each dist_object which we use for debug-mode assertion of preconditions, and possibly even compile away in opt mode (unless we decide to provide a runtime has_value() state query). The "registration" state already comes "for free" from the existing digest and team state variables.

[Amir Kamil]

Proposed API changes:

struct inactive_t;
constexpr inactive_t inactive{};

template<typename T>
class dist_object {
public:
  // constructs an empty and inactive (unregistered) dist_object
  // permitted before upcxx init
  dist_object();

  // constructs a non-empty but inactive (unregistered) dist_object
  // permitted before upcxx init
  template<typename ...Args>
  dist_object(inactive_t, Args&& ...args);

  // equivalent to dist_object(T{})
  // resolves an ambiguity when passing an empty initializer list to the constructor
  dist_object(std::initializer_list</* unspecified */>);

  // add MoveAssignable concept (for overwriting default-constructed dist_objects)
  // requires !is_active() (and upcxx init and the master persona)
  // inactivates that, but does not change that.has_value()
  dist_object& operator=(dist_object &&that) noexcept;

  // register this dist_object over the given team
  // requires !is_active() && has_value() (and upcxx init and the master persona)
  // collective over tm
  void activate(const upcxx::team &tm);

  // whether or not this dist_object has been registered with a team
  // permitted before upcxx init
  bool is_active() const;

  // whether or not this dist_object holds a value
  // permitted before upcxx init
  bool has_value() const;

  // construct the underlying T value
  // (does NOT require this dist_object to be empty, nor active)
  // permitted before upcxx init
  template<typename ...Args>
  T& emplace(Args&&... args)};
};

(UPDATE: API modified to avoid the problem below.)

However, the addition of a default constructor breaks existing code. Specifically, the following in example/prog-guide/dmap.hpp:

  DistrMap() : local_map({}) {}

Here, local_map has type dist_object<std::unordered_map<std::string, std::string>>. The initialization is ambiguous:

• It can be a call to dist_object(T value), with the initializer list {} used to default-construct the underlying unordered_map.
• It can be a call to dist_object(dist_object&&), with the initializer list used to default-construct a dist_object.

Slapping an explicit on the default ctor fixes the problem with Clang 14, but not with GCC 12.

Here’s a simple, pure-C++ program to replicate the problem:

#include <unordered_map>

template<typename T>
struct dist_object {
  dist_object() {}
  dist_object(T value) {}
  dist_object(dist_object&&) {}
};

int main() {
  dist_object<std::unordered_map<int, int>> d1 =
    dist_object<std::unordered_map<int, int>>({});
}

The fix in dmap.hpp would be to do local_map(world()) instead of local_map({}).

Thoughts on this issue, or on the proposed API above?

[Dan Bonachea]

the addition of a default constructor breaks existing code

Thank you for making this discovery - I'm very concerned about this subtle breakage to existing code.

It would be ideal if adding a default constructor for dist_object could be made to work smoothly, but you've shown that unfortunately it doesn't, at least with your straightforward approach.

However IMO the primary motivation for this enhancement is to allow construction of inactive and possibly empty dist_objects (e.g. as global variables) that would be constructed before upcxx::init and to allow move assignment onto them. For that specific but important use case it's not essential to use a zero-argument constructor. So for example instead of spelling that constructor dist_object() we could instead spell it dist_object(empty_inactive_t) and still satisfy this use case (although that spelling will likely make it harder for novice users to find). The lack of a default constructor means we cannot use dist_object's as the value type for a std::unordered_map, but that doesn't work today either and seems like an odd use case anyhow.

All that being said... I might have a workaround that addresses the breakage of legacy code described in @Amir Kamil's comment above. Specifically, I think we can add a dist_object(std::initializer_list) constructor designed to disambiguate that particular case of empty braces and forward to our chosen T-default constructor.

Here is a demonstration code, an expanded version of Amir's reproducer where I've tried to proxy all the interesting cases:

#include <unordered_map>
#include <iostream>
#include <vector>

struct inactive_t{};
constexpr inactive_t inactive{};

struct team_t{};

#define R(x) std::cout << x << std::endl

template<typename T>
struct dist_object {
private:
  struct helper{};
public:
  // proxies for existing members in 2022.9.0:

  dist_object(T value) { R("T by-value constructor"); }

  template<typename ...Args>
  dist_object(team_t, Args&& ...args) { R("team T emplace constructor"); }

  dist_object(dist_object&&) { R("move constructor"); }

  // new members:

  dist_object() { R("default (empty, inactive) constructor");}

  template<typename ...Args>
  dist_object(inactive_t, Args&& ...args) { R("inactive T emplace constructor"); }

  dist_object(std::initializer_list<helper> args)
     : dist_object(T{}) { R("empty braces -> T default construct");}

  dist_object& operator=(dist_object&&) { R("move assign"); return *this; }

};
// static lifetime construction:
dist_object<int> d1;
dist_object<int> d2(inactive,47);

int main() {
  using map_t = std::unordered_map<int, int>;
  R("\nDIST_OBJECT OF MAP {}:");
  dist_object<map_t> dm1 =
      dist_object<map_t>({});

  R("\nDIST_OBJECT OF MAP MOVE-CONS:");
  dist_object<map_t> dm2 = std::move(dm1);

  R("\nDIST_OBJECT OF MAP MOVE-ASSIGN:");
  dm1 = std::move(dm2);

  R("\nDIST_OBJECT OF MAP INACTIVE:");
  dist_object<map_t> dm3 =
      dist_object<map_t>(inactive);

  using vec_t = std::vector<int>;
  R("\nVECTOR 1:");
  dist_object<vec_t> dv1 =
      dist_object<vec_t>({  });

  R("\nVECTOR 2:");
  dist_object<vec_t> dv2 =
      dist_object<vec_t>({ 4 });

  R("\nVECTOR OF DIST_OBJECT:");
  std::vector<dist_object<int>> vd(4);
  vd[1] = {4};
  vd[2] = {};

  R("\nMAP OF DIST_OBJECT:");
  std::unordered_map<int, dist_object<int>> md;
  md[0] = {};
  md.emplace( 1, dist_object<int>{});
  md.emplace( 2, dist_object<int>{inactive, 7});
  md.emplace( std::piecewise_construct,
              std::forward_as_tuple(3), std::forward_as_tuple());

  return 0;
}

This code compiles without complaint on a handful of compilers I surveyed, and produces the expected output. It's entirely possible I missed exercising some other important case that appears in legacy code, but this approach at least seems worth pursuing?

[Amir Kamil]

Thanks for investigating @Dan Bonachea . I think your workaround should fix the issue. I’ll update the API proposal above to include it.

[Dan Bonachea]

The changes described here are now implemented in Impl PR 475, merged to develop @ 7850487.

Finishing the specification update to match these changes is now a release blocker.

[Dan Bonachea]

Resolved in pull request #97, merged at 8b6cd38 ·