Clarify publish behavior for shared_ptr and unique_ptr #344
Conversation
| | unique_ptr\<MsgT\> @1 | unique_ptr\<MsgT\> <br> unique_ptr\<MsgT\> | @1 <br> @2 | | ||
| | unique_ptr\<MsgT\> @1 | shared_ptr\<MsgT\> | @1 | | ||
| | unique_ptr\<MsgT\> @1 | shared_ptr\<MsgT\> <br> shared_ptr\<MsgT\> | @1 <br> @1 | | ||
| | unique_ptr\<MsgT\> @1 | shared_ptr\<MsgT\> <br> shared_ptr\<MsgT\> | @1 <br> @2 | |
There was a problem hiding this comment.
your change here is wrong
if there is 1 publisher with unique ptr and two subscribers with shared pointers, both receive the same publisher pointer (@1 in this case)
There was a problem hiding this comment.
Thanks for your suggestions. I just took a deeper look into the code, and I noticed that only shared_ptr<const MsgT> would share the stored object pointer and avoid extra copy. Every sharedptr containing non-const msg would require intra process manager do another copy. The changes I made is based on my experiments on shared_ptr<MsgT>, which is non-const.
I think the current implementation makes sense, as non-const sharedptr could be changed and an extra copy is necessary for safety reasons. But the table doesn't show the constness of MsgT. It also seems weird that the first part of design documentation specifies const MsgT, but at some point this gets lost. I will update the document to emphasize the constness of shared_ptr.
| | unique_ptr\<MsgT\> @1 | shared_ptr\<MsgT\> <br> shared_ptr\<MsgT\> | @1 <br> @1 | | ||
| | unique_ptr\<MsgT\> @1 | shared_ptr\<MsgT\> <br> shared_ptr\<MsgT\> | @1 <br> @2 | | ||
| | unique_ptr\<MsgT\> @1 | unique_ptr\<MsgT\> <br> shared_ptr\<MsgT\> | @1 <br> @2 | | ||
| | unique_ptr\<MsgT\> @1 | unique_ptr\<MsgT\> <br> shared_ptr\<MsgT\> <br> shared_ptr\<MsgT\> | @1 <br> @2 <br> @2| |
There was a problem hiding this comment.
i don't think these should be removed
There was a problem hiding this comment.
shared_ptr messages: Cannot be published (removed incorrect zero-copy claims)
Shared pointers CAN be published, although they don't achieve zero copy.
This is exactly what's indicated in the #### Publishing SharedPtr section that you removed.
My bad, i forgot that publishing shared_ptr has been deprecated long ago.
However, your PR is also making incorrect changes.
I think we should just remove the section about Publishing shared_ptr.
| The following tables show a recap of when the proposed implementation has to create a new copy of a message. | ||
| The notation `@` indicates a memory address where the message is stored, different memory addresses correspond to different copies of the message. | ||
|
|
||
| **Note on current behavior**: The actual implementation behavior differs from the proposed implementation as described in [rclcpp issue #2872](https://github.com/ros2/rclcpp/issues/2872): |
There was a problem hiding this comment.
I would remove this section.
The fact that you can't publish shared ptr is not necessarily related to this design.
Moreover, i don't really see many references to publishing shared ptrs in this page.
- https://github.com/ros2/design/blob/gh-pages/articles/intraprocess_communication.md?plain=1#L231
- https://github.com/ros2/design/blob/gh-pages/articles/intraprocess_communication.md?plain=1#L360-L370
these should be removed
2 participants
Description
Updates the intra-process communication design document to reflect the actual current behavior with respect to function publish():
shared_ptr messages: Cannot be published (removed incorrect zero-copy claims)
unique_ptr messages: Zero-copy transfer achieved
raw messages: Both msg and std::move(msg) result in copying
Key changes:
Fixes # (issue)
Is this user-facing behavior change?
No
Did you use Generative AI?
No
Additional Information