- edited description
Add serialization through `std::reference_wrapper`
Currently we serialize "through" references, ie: (spec 6.4-{1,2})
- A reference type
cq T&
orcq T&&
is Serializable whenT
is Serializable. Such a type is serialized by serializing the referent, and deserialization produces an object of non-reference typedeserialized_type_t<T>
. Thus,deserialized_type_t<cq T&>
anddeserialized_type_t<cq T&&>
are both the same asdeserialized_type_t<T>
.- A reference type is never TriviallySerializable.
However we have not extended this to include the std::reference_wrapper<T>
utility class that is often used to contain references. Worse still, std::reference_wrapper<T>
is usually TriviallyCopyable (required starting in C++17), which means we consider it TriviallySerializable (which it semantically is not, because the implementation contains a raw pointer). As a result, sending a std::reference_wrapper<T>
via RPC to any other process currently silently results in garbage.
Example program:
#include <upcxx/upcxx.hpp>
#include <iostream>
#include <string>
#include <functional>
int main(int argc, char *argv[]) {
upcxx::init();
std::string s = "Hello from rank ";
s += std::to_string(upcxx::rank_me());
if (!upcxx::rank_me()) std::cout << upcxx::is_trivially_serializable<decltype(s)>::value <<std::endl;
upcxx::barrier();
upcxx::rpc(0,[](std::string const &s1) {
std::cout << s1 << std::endl;
}, s).wait();
upcxx::barrier();
std::string &s_ref = s;
if (!upcxx::rank_me()) std::cout << upcxx::is_trivially_serializable<decltype(s_ref)>::value <<std::endl;
upcxx::barrier();
upcxx::rpc(0,[](std::string const &s1, std::string const &s2) {
UPCXX_ASSERT_ALWAYS(s1 == s2);
std::cout << s2 << std::endl;
}, s, s_ref).wait();
upcxx::barrier();
std::reference_wrapper<std::string> s_refwrap = s;
if (!upcxx::rank_me()) std::cout << upcxx::is_trivially_serializable<decltype(s_refwrap)>::value <<std::endl;
upcxx::barrier();
upcxx::rpc(0,[](std::string const &s1, std::string const &s2) {
UPCXX_ASSERT_ALWAYS(s1 == s2);
std::cout << s2 << std::endl;
}, s, s_refwrap).wait();
upcxx::barrier();
upcxx::barrier();
if (!upcxx::rank_me()) std::cout << "SUCCESS" <<std::endl;
upcxx::finalize();
return 0;
}
Example Output from 2022.3.0:
upcxx -network=smp -g ref-serialize.cpp && upcxx-run -n 4 a.out
0
Hello from rank 0
Hello from rank 2
Hello from rank 1
Hello from rank 3
0
Hello from rank 0
Hello from rank 3
Hello from rank 1
Hello from rank 2
1
Hello from rank 0
*** FATAL ERROR (proc 0):
//////////////////////////////////////////////////////////////////////
UPC++ assertion failure:
on process 0 (pcp-d-10)
at ref-serialize.cpp:32
in function: main(int, char**)::<lambda(const string&, const string&)>()
Failed condition: s1 == s2
To have UPC++ freeze during these errors so you can attach a debugger,
rerun the program with GASNET_FREEZE_ON_ERROR=1 in the environment.
//////////////////////////////////////////////////////////////////////
This seems like a simple oversight on our part. If we serialize through references T&
and T&&
then we should apply the same policy to std::reference_wrapper<T>
and deploy appropriate serialization logic to make that happen.
The proposed change would presumably imply asymmetric deserialization for std::reference_wrapper<T>
, in the same way that we have asymmetric deserialization now for T&
and T&&
. IOW we'd need to explain that:
deserialized_type_t<std::reference_wrapper<T>> == deserialized_type_t<T>
which (due to our normal serialization rules) has implications for the containers where this utility is frequently used - for example:
deserialized_type_t<std::vector<std::reference_wrapper<T>>> == std::vector<deserialized_type_t<T>>
ie, the asymmetry propagates outward to containers (something that might need to be explained in the guide).
Comments (5)
-
reporter -
Proposed implementation in impl PR 445.
-
reporter As mentioned in the Impl PR:
The spec for this needs to make clear that we strip const qualifiers from inside
std::reference_wrapper
(and regular references). The current spec wording for this detail is IMO a bit too subtle and should be clarified. Currently 6.4-1 and 6.4-4 could be misread as mutually contradictory. This is probably just a matter of being a bit more explicit, and possibly reordering the 6.4 paragraphs for clarity. -
reporter - marked as blocker
-
assigned issue to
Impl PR 445 is now merged.
Making this a blocker to finish documentation before release
-
reporter - changed status to resolved
Resolved in spec PR 91, merged at 547fee6
- Log in to comment