Avoid extra copy for serialized SHM chunks#551
Avoid extra copy for serialized SHM chunks#551StanByriukov02 wants to merge 1 commit intoros2:rollingfrom
Conversation
|
This is too tricky for me to review, and unfortunately I don't think @eboasson has much time to spend on this. One thing is that we probably don't want to add a new environment variable. |
| void clear_external_view() | ||
| { | ||
| m_external_data = nullptr; | ||
| m_external_size = 0; | ||
| } |
There was a problem hiding this comment.
clear_external_view() doesn't reset m_size? but set_external_view, m_size is set to size?
it looks like a subtle inconsistency that could bite in future refactors.
| auto d = static_cast<const serdata_rmw *>(dcmn); | ||
| serialize_into_serdata_rmw_on_demand(const_cast<serdata_rmw *>(d)); | ||
| ref->iov_base = byte_offset(d->data(), off); | ||
| ref->iov_base = byte_offset(const_cast<void *>(d->data_ro()), off); |
There was a problem hiding this comment.
(not blocking PR) It would be safer to either make byte_offset const-correct or add a const_byte_offset helper.
| d->resize(iox_header->data_size); | ||
| memcpy(d->data(), d->iox_chunk, iox_header->data_size); | ||
| // Fail-closed: only take a zero-copy view when we don't need the extra | ||
| // padding that `resize()` adds for DDSI/CDR alignment. | ||
| if ((iox_header->data_size % 4) == 0) { | ||
| d->set_external_view(d->iox_chunk, iox_header->data_size); | ||
| if (receipts_enabled) { | ||
| shm_serialized_view_count.fetch_add(1, std::memory_order_relaxed); | ||
| shm_serialized_view_bytes.fetch_add( | ||
| static_cast<uint64_t>(iox_header->data_size), std::memory_order_relaxed); | ||
| } | ||
| } else { | ||
| d->resize(iox_header->data_size); | ||
| memcpy(d->data(), d->iox_chunk, iox_header->data_size); | ||
| if (receipts_enabled) { | ||
| shm_serialized_copy_count.fetch_add(1, std::memory_order_relaxed); | ||
| shm_serialized_copy_bytes.fetch_add( | ||
| static_cast<uint64_t>(iox_header->data_size), std::memory_order_relaxed); | ||
| } | ||
| } |
There was a problem hiding this comment.
my biggest concern is that, set_external_view stores a raw pointer into d->iox_chunk, what guarantees that the SHM chunk remains valid for the entire lifetime of the serdata_rmw object? the original code copied the bytes precisely to decouple the serdata lifetime from the SHM chunk lifetime. if the iceoryx chunk is returned to the pool (e.g., after the subscriber loan is released) while the serdata still holds a pointer into it, you get a use-after-free?
5 participants
When running with iceoryx shared memory enabled, we can receive a chunk that already contains serialized bytes, but we still copy that chunk into a heap buffer before deserializing/forwarding it.
This change avoids that extra copy by keeping a read-only view into the received SHM chunk (only when size is 4-byte aligned, otherwise it falls back to the existing padded-copy path). The rest of the serdata ops are switched to read from a generic read-only pointer so both owned buffers and external views work.
If you want a quick sanity check without profiling tools, set RMW_CYCLONEDDS_SHM_RECEIPTS=1 and the process will print one line at exit showing how many serialized SHM bytes were handled via view vs copied.