Add loaned sample zero-copy API support

[sumanth-nirmal]

This PR adds support for the zero-copy API.

[eboasson]

I have only had a quick look for now, but there clearly is an issue if a serdata is constructed from a loan (whether or not Iceoryx), and Cyclone then needs to invoke one of the operations that accesses the serialized representation. For example, the to_ser function returns a pointer in the serialized representation, and to_sample deserializes the data.

Something like the below would do, it basically performs the serialization on-demand. It is clearly broken because: (1) there are more functions that access it, and (2) there is always the possibility of multiple threads forcing the serialization at the same time. I would address (1) by going over the code and doing the serialization wherever it is needed 🙃 and address (2) by checking whether data is set, serializing into a new buffer if not, and then atomically setting data (if not yet set) or discarding that new buffer (if another thread raced us and won).

diff --git a/rmw_cyclonedds_cpp/src/serdata.cpp b/rmw_cyclonedds_cpp/src/serdata.cpp
index 496da75a..a5c7a84e 100644
--- a/rmw_cyclonedds_cpp/src/serdata.cpp
+++ b/rmw_cyclonedds_cpp/src/serdata.cpp
@@ -181,15 +181,11 @@ static struct ddsi_serdata * serdata_rmw_from_keyhash(
   return new serdata_rmw(type, SDK_KEY);
 }
 
-static struct ddsi_serdata * serdata_rmw_from_sample(
-  const struct ddsi_sertype * typecmn,
-  enum ddsi_serdata_kind kind,
-  const void * sample)
+static void serdata_rmw_serialize_into(serdata_rmw *d, const void *sample)
 {
+  const struct sertype_rmw * type = static_cast<const struct sertype_rmw *>(d->type);
   try {
-    const struct sertype_rmw * type = static_cast<const struct sertype_rmw *>(typecmn);
-    auto d = std::make_unique<serdata_rmw>(type, kind);
-    if (kind != SDK_DATA) {
+    if (d->kind != SDK_DATA) {
       /* ROS 2 doesn't do keys, so SDK_KEY is trivial */
     } else if (!type->is_request_header) {
       size_t sz = type->cdr_writer->get_serialized_size(sample);
@@ -204,8 +200,21 @@ static struct ddsi_serdata * serdata_rmw_from_sample(
       d->resize(sz);
       type->cdr_writer->serialize(d->data(), wrap);
     }
-    return d.release();
+  } catch (std::exception & e) {
+    RMW_SET_ERROR_MSG(e.what());
+  }
+}
 
+static struct ddsi_serdata * serdata_rmw_from_sample(
+  const struct ddsi_sertype * typecmn,
+  enum ddsi_serdata_kind kind,
+  const void * sample)
+{
+  try {
+    const struct sertype_rmw * type = static_cast<const struct sertype_rmw *>(typecmn);
+    auto d = std::make_unique<serdata_rmw>(type, kind);
+    serdata_rmw_serialize_into(d.get(), sample);
+    return d.release();
   } catch (std::exception & e) {
     RMW_SET_ERROR_MSG(e.what());
     return nullptr;
@@ -214,7 +223,7 @@ static struct ddsi_serdata * serdata_rmw_from_sample(
 
 static struct ddsi_serdata * serdata_rmw_from_iox(
   const struct ddsi_sertype * typecmn,
-  enum  ddsi_serdata_kind kind, iox_sub_t * sub, void * iox_buffer)
+  enum  ddsi_serdata_kind kind, void * sub, void * iox_buffer)
 {
   static_cast<void>(sub);  // unused
   const struct sertype_rmw * type = static_cast<const struct sertype_rmw *>(typecmn);
@@ -252,6 +261,10 @@ static struct ddsi_serdata * serdata_rmw_to_untyped(const struct ddsi_serdata *
 static void serdata_rmw_to_ser(const struct ddsi_serdata * dcmn, size_t off, size_t sz, void * buf)
 {
   auto d = static_cast<const serdata_rmw *>(dcmn);
+  if (d->iox_chunk && d->data() == nullptr) {
+    // FIXME: this needs to protected so it doesn't happen multiple times
+    serdata_rmw_serialize_into(const_cast<serdata_rmw *>(d), SHIFT_PAST_ICEORYX_HEADER(d->iox_chunk));
+  }
   memcpy(buf, byte_offset(d->data(), off), sz);
 }
 
@@ -260,6 +273,10 @@ static struct ddsi_serdata * serdata_rmw_to_ser_ref(
   size_t sz, ddsrt_iovec_t * ref)
 {
   auto d = static_cast<const serdata_rmw *>(dcmn);
+  if (d->iox_chunk && d->data() == nullptr) {
+    // FIXME: this needs to protected so it doesn't happen multiple times
+    serdata_rmw_serialize_into(const_cast<serdata_rmw *>(d), SHIFT_PAST_ICEORYX_HEADER(d->iox_chunk));
+  }
   ref->iov_base = byte_offset(d->data(), off);
   ref->iov_len = (ddsrt_iov_len_t) sz;
   return ddsi_serdata_ref(d);

[clalancette]

Initial CI, just up to rmw_cyclonedds_cpp:

• Linux
• Linux-aarch64
• macOS
• Windows

(assuming this is happy, I'll do a full CI to make sure nothing downstream got broken)

[clalancette]

All right, straightforward CI is happy. Here is full one:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

I can't say I love the #ifdef jungle, but we can fix that up later.

I've got one comment inline about reducing code duplication, but that can be follow-up work.

I'm going to approve this, but the following needs to happen to merge:

1. CI needs to come back green.
2. Open an issue to fix up the follow-up items: removing the ifdef jungle, and reducing code duplication.
3. We need to figure out what the situation is with the branches on cyclonedds (that is, whether we should be building this against the master branch of cyclonedds or the iceoryx branch. If we are doing the latter, we also need an update to https://github.com/ros2/ros2/blob/master/ros2.repos to change branches).
4. I'd really prefer if @eboasson gives his approval on this.

[clalancette]

This pattern of:

ts = get_typesupport(C++);
if (ts != nullptr) {
    do_c++_thing;
} else {
  ts = get_typesupport(C);
  do_C_thing;
}

Is really common in the codebase. It would be really nice to come up with a common method for this (maybe that takes a std::function to "do-the-thing"?).

[clalancette]

@sumanth-nirmal It looks like we are getting segfaults in some tests on aarch64. Can you take a look?

[sumanth-nirmal]

@sumanth-nirmal It looks like we are getting segfaults in some tests on aarch64. Can you take a look?

@clalancette I pushed the changes, can we start a new CI pipeline?

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[eboasson]

@clalancette I am in agreement with your review notes. It is not polished yet, but it works and any shortcomings can be dealt with in the coming days. So it has my approval (pending the same items that you listed).

We need to figure out what the situation is with the branches on cyclonedds (that is, whether we should be building this against the master branch of cyclonedds or the iceoryx branch. If we are doing the latter, we also need an update to https://github.com/ros2/ros2/blob/master/ros2.repos to change branches).

It is meant to be used with cyclonedds' iceoryx branch, which is on a path to become the 0.8.0 release, and which gives the actual possibility of transparently delivering data via Iceoryx. The reason why I checked against both branches of cyclonedds is that it should work with both (it does), and that in turn means keeping track of branches and what-works-with-what is kept nice and simple.

[clalancette]

CI (just up to test_communication, rclcpp, rmw_implementation, rcl, and rcl_action):

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

All right, green CI there (the macOS failure is a known flakey test). Here's the whole thing:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

All right, all is green. Going ahead and merging this one and ros2/ros2#1126