-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New intra-process communication design #239
Conversation
Please format with one sentence per line and no line breaks within a sentence so that changes made in response to comments are easier to track. |
The choice of the buffer data-type is controlled through an additional field in the `SubscriptionOptions`. The default value for this option is denominated `CallbackDefault`, which corresponds to selecting the type between `shared_ptr<constMessageT>` | ||
and `unique_ptr<MessageT>` that better fits with its callback type. This is deduced looking at the output of `AnySubscriptionCallback::use_take_shared_method()`. | ||
|
||
If the history QoS is set to `keep all`, the buffers are dynamically allocated. On the other hand, if the history QoS is set to `keep last`, the buffers have a size equal to the depth of the history and they act as ring buffers (overwriting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different from how these QoS settings work in DDS. Not having the same semantics for QoS settings between inter-process and intra-process would make it hard to reuse a node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We meant to say "buffers are dynamically adjusted in size up to some max limit specified as part of KEEP_ALL QoS". Then the semantics are the same, right?
|
||
### Publishing only intra-process | ||
|
||
#### Publishing unique_ptr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes there's something to be said for using UML diagrams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I don't get what do you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alsora I am guessing the comment is recommending UML based diagrams to describe visually some of the sections of this PR. For. Eg, Creating a publisher, Creating a subscription, Publishing only intra-process etc can have sequence diagrams in addition to the description.
This is a tangential comment, but I wonder if we could achieve the same zero-copies-when-same-process result by reducing the number of copies requires for going into and out of the |
The current prototype implementation and the design focus on |
I am not sure about the status of While I agree about maintaining feature parity between the 2 supported clients, this design proposal is an improvement to the existing intra-process support, which currently is supported only in If we continue to have the intra process manager live in |
How about moving the Support for zero copies is an important objective, but its not the only one. It has been observed that creating DDS participants is pretty resource heavy in terms of net memory required (atleast for FastRTPS & OpenSplice) and the discovery process is CPU intensive (due to multicast). I don't want to derail this PR as this is totally tangential topic and needs more thought and planning. Should we move this discussion to a discourse post ? |
The aim of this PR is to improve the current intra-process communication, that is only supported in
As @raghaprasad says, I think that moving the intra-process manager to a different place from However, I think that once it has been finalized and implemented there shouldn't be any major blockers avoiding to move it to a COMMON While working to this PR I also explored the idea of implementing the intra-process manager logic at the RMW layer (I have a very dummy proof of concept for RMW DPS). |
But it is possible to do the I feel that having inter-process only available in |
ROS2 architecture [1] depicts very explicitly that intra-process communication (along with type masquerading optimizations) belongs in the language-specific client library. Also, intra-process communication is merely an optimization, and not a "feature" per say. All the features like services, parameters, etc. are rightly implemented in [1] http://docs.ros2.org/dashing/developer_overview.html#internal-ros-interfaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposal looks quite good to me.
I left some minimal comments. I will do a second read before jumping to the PR.
By setting the buffer type to `shared_ptr`, no copies are needed when the `Publisher` pushes messages into the buffers. | ||
Eventually, the `Subscription`s will copy the data only when they are ready to process it. | ||
|
||
On the other hand, if the published data are very small, it can be advantageous to do not use C++ smart pointers, but to directly store the data into the buffers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this improve performance? I don't see much benefit of this vs using an unique ptr. If memory allocation has to be avoided, an allocator can be passed.
I think it's fine to having this option too, but I'm just curious if there is another reason for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this only makes sense if you also avoid the necessity that all intra-process starts as a unique_ptr
. Once you've created or were given a unique pointer, you can either just deliver that (in the case of a single owning subscription) and/or make a single copy into a shared pointer until you're ready to deliver each (in the case of more subscriptions) and only when they're taken make another copy.
Some thoughts about moving intraprocess communication to rmw:
That's mostly "historical". I think that if we have a good rationale for implementing it at rmw level, we should.
I think something like that could be possible, but quite complicated. I would like to see something mimicking connext Zero Copy Transfer Over Shared Memory semantics (by default connext use shared memory, but it doesn't use zero copy transfer, which have an specific semantics). Basically, instead of creating a unique pointer and then publishing it: auto msg = std::make_unique<MSG_TYPE>();
/* Fill the message here */
publisher->publish(std::move(msg)) You ask to the publisher a piece of memory, fill it, and then publish: auto msg = publisher->new_message();
/* Fill the message here */
publisher->publish(std::move(msg)); // I'm using move semantics because the message will be undefined after calling publish. But how we wrap the msg for this is an implementation detail. For dds vendors that have implemented zero copy transport, this could just wrap it. I also think this idea will look idiomatic in other languages (for example, in python), and performance should be quite similar.
I agree, I think this implementation will improve performance compared with the current intraprocess implementation. We could later decide if moving intraprocess comm to the |
@ivanpauno I agree with the above discussion that we should evaluate this PR for what it is (an improvement to the existing scenario of Intra-Process existing only in rclcpp) - but I'm also very interested in making this feature more globally accessible. I was sold that one of ROS2's goals was to stop reimplementing features for each language, and to make language clients as thin as possible, therefore functionality like this should live in My question is, where can we start and maintain a focused discussion for this topic today, rather than just saying "we can decide later"? That way we can take this discussion off of this PR, but still know that we have actually begun to focus on the bigger-picture architectural discussion somewhere. Perhaps a new |
I fully agree about making the feature globally available. Actually, it's currently available in rclcpp only for publish/subscribe communication, but for example, not for services.
Maybe a PR/issue here, and post the link in discourse too. As the topic is complex, if we want to move ahead with it fast, I think that it would be good to organize a meeting for discussing the topic. I would be interested in participate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just finished going over this again. Some of my comments maybe don't apply after reading further in the document, but I tried to remove those that I could.
I'm going through the implementation pr simultaneously and will continue that in the next few days.
|
||
- If the history is set to `keep_last`, then the depth of the history corresponds to the size of the ring buffer. | ||
On the other hand, if the history is set to `keep_all`, the buffer becomes a standard FIFO queue with an unbounded size. | ||
- The reliability is only checked by the `IntraProcessManager` in order to understand if a `Publisher` and a `Subscription` are compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this above, but this may also affect how the behavior of how a publisher should block when subscriptions are full.
By setting the buffer type to `shared_ptr`, no copies are needed when the `Publisher` pushes messages into the buffers. | ||
Eventually, the `Subscription`s will copy the data only when they are ready to process it. | ||
|
||
On the other hand, if the published data are very small, it can be advantageous to do not use C++ smart pointers, but to directly store the data into the buffers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this only makes sense if you also avoid the necessity that all intra-process starts as a unique_ptr
. Once you've created or were given a unique pointer, you can either just deliver that (in the case of a single owning subscription) and/or make a single copy into a shared pointer until you're ready to deliver each (in the case of more subscriptions) and only when they're taken make another copy.
Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com>
Github does not allow me to reply to the last comment #239 (comment) ... |
@alsora Now that ros2/rclcpp#778 is merged, is there anything we need to update here? |
No I think this is fine. |
All right, thanks. @wjwwood @ivanpauno You two had the earlier comments and still some unresolved conversations. Are you satisfied with this article now, or do we still need to resolve the open conversations? |
Should we merge this? |
Merged. If there are any further comments, they can be addressed in a follow-up. |
Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com>
Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com>
This PR adds a description of the current intra-process communication mechanism, together with a design proposal for improving its performances and supporting more QoS.
The new design is supported by experimental evaluations.
An initial discussion about this design can be found here.
The implementation of this design can be found here.
@dgoel @raghaprasad