- changed milestone to 2023.3.0 release
Move semantics for distributed objects
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 aT
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?
Comments (7)
-
-
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 adist_object<T> *p = nullptr;
before UPC++ library init, treat a null pointer as "inactive", and "activate" it by callingp = 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 currentdist_object
and a few additional characters. We should not force alldist_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 fordist_object::operator*()
anddist_object::operator->()
that access the contained value. Registration needs to remain collective and we currently couple that todist_object
construction (which should remain the default for legacy reasons), but we could provide non-collectivedist_object
constructor(s) that defer registration to a later collectiveregister(team = world())
call. Similarly we could providedist_object
constructor(s) that leave the value in an unconstructed state for a laterdist_object<T>::emplace(args...)
ordist_object<T>::operator=(T&&)
.I think at runtime this boils down to adding a boolean
constructed
state bit in eachdist_object
which we use for debug-mode assertion of preconditions, and possibly even compile away in opt mode (unless we decide to provide a runtimehas_value()
state query). The "registration" state already comes "for free" from the existingdigest
andteam
state variables. -
reporter 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 typedist_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 underlyingunordered_map
. - It can be a call to
dist_object(dist_object&&)
, with the initializer list used to default-construct adist_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 dolocal_map(world())
instead oflocal_map({})
.Thoughts on this issue, or on the proposed API above?
- It can be a call to
-
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 constructordist_object()
we could instead spell itdist_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 usedist_object
's as the value type for astd::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?
-
reporter Thanks for investigating @Dan Bonachea . I think your workaround should fix the issue. I’ll update the API proposal above to include it.
-
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.
-
- changed status to resolved
Resolved in pull request #97, merged at 8b6cd38 ·
- Log in to comment
Mass roll-over of open issues to next release milestone