default constructor for dist_object

[Steven Hofmeyr]

Currently, dist_object has no default constructor. That makes it awkward to use in certain cases. Here's an example:

class DistrMap {
  dist_object<unordered_map<K, V> > local_map;
  DistrMap() : local_map((unordered_map<K,V>){}) {
    ....
  }
}

The only way to get the above example to work correctly (as far as I could tell) is to use a c-style cast to get the compiler to choose the correct constructor for dist_object. This solution was non-obvious to me, and getting it wrong produces lots of ugly template errors that are hard to parse. Other users will get bitten by this, for sure. If we had a default constructor for dist_object then the above would simply be:

class DistrMap {
  dist_object<unordered_map<K, V> > local_map;
  DistrMap() {
    ....
  }
}

Adding a default constructor to dist_object is trivial. Is there any reason not to have one?

[BrianS]

A default constructor can be made. Then a user can move the real one into the member object. I'll look it this one after the meeting today.

Brian

[Dan Bonachea]

dist_objects are special in that they require collective construction to function correctly, but we do not (currently) validate the constructor is actually called collectively. Titanium's type system would have labelled the dist_object class as "single" and had type machinery to ensure the collective construction property (not an option we have as a library). In the absence of such machinery we are trusting the user to correctly propagate the collective construction property from a dist_object to its containing objects and scopes. So in Steve's example, DistrMap must be constructed collectively otherwise the embedded dist_object will malfunction -- but there is no enforcement mechanism and due to type abstraction this requirement might not be clear to the client of this class.

Adding a default constructor makes it even easier to write things that are likely to malfunction, some examples:

auto a = new dist_object<int>[rank_me()];

unordered_map<int,dist_object<int>> map;
*map[rank_me()] = 5;

None of this is a fundamental argument against default construction, I'm just pointing out that dist_object is already a "pointy" feature that is easily broken by misuse, and default construction makes it even easier to misuse.

[Amir Kamil]

What Brian proposes would break when_here(), which is pretty fundamental to the abstraction of a dist_object.

However, we are missing a constructor that defaults the team when constructing the value directly. We could add the following:

template <typename T>
template<typename ...Arg>
dist_object<T>::dist_object(Arg &&...arg);

which constructs over the world team.

[BrianS]

I think we figured out that default construction would either create an invalid not-distributed object cheaply, or result in global synchronization communication on every invocation, neither of which sounds great. containers of dist_objects is going to be a problem without null construction though.

[Amir Kamil]

You can still make a container of dist_objects with an initializer list, the move version of methods such as push_back(), or a method such as emplace_back().

[Dan Bonachea]

This issue was discussed at length in the 2018-03-02 developer meeting (unfortunately after Steve left).

The decision was to leave the spec as-is (no default constructor for dist_object), because introducing one makes it significantly easier for the user to generate undefined behavior (eg via non-collectively-created containers of dist_object, or default construction of a contained T which is not default constructible).

The C++ experts also believe that there is a cleaner syntactic solution to the compile error Steve encountered (that should probably be added to this issue and possibly the PG), and that the eventual addition of the specified dist_object constructors that take a team argument will allow further simplification of resolution ambiguity.

[Amir Kamil]

Here's what I suggest:

template<typename K, typename V>
class DistrMap {
  dist_object<unordered_map<K, V> > local_map{unordered_map<K, V>{}};
  DistrMap() {
  }
};