Conversation
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
clalancette
left a comment
There was a problem hiding this comment.
Overall, this is a good find. It's a little unfortunate for performance that we are causing copies of the DDSTopic to be made for every caller that comes in here, but I think this is simpler than doing a reference count or anything like that.
I've got one big question and a few nits inline, otherwise looking good.
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Based on rti documentation, it does seem like they are doing reference counting internally and that the copy should be cheap. |
clalancette
left a comment
There was a problem hiding this comment.
Minor nit, but otherwise looks good to me with green CI. I'll suggest running test_communication with this change in place just to get a little broader test coverage.
|
Follow up of #442.
Based on find_topic documentation, it returns a object that can be independently destroyed and not a reference to the one found.
This PR modifies publisher/subscription creation and destruction, so that the
DDS::Topicobjects are destroyed as soon as possible (i.e. when destroying a publisher/subscription).For example, an application that creates and destroy a publisher on the same topic repeatedly was increasingly allocating more resources. This PR avoids that.
I've also simplified topic creation code, as using both
find_topicand lookup_topic_descritpion is redundant.This passes
rcltests correctly locally, but it deserves full CI before being merged.Publisher and Subscriptions are still leaking the "registered type", but that's much more complex to fix because we're using undocumented rti functions 😕.