[JSB] skip publish on no subscription #1609
Conversation
github-actions
bot
requested review from
bmagyar,
christophfroehlich,
mhubii,
moriarty,
olivier-stasse,
progtologist and
VX792
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1609 +/- ##
==========================================
+ Coverage 85.16% 85.20% +0.03%
==========================================
Files 126 126
Lines 11915 11922 +7
Branches 997 997
==========================================
+ Hits 10148 10158 +10
+ Misses 1454 1452 -2
+ Partials 313 312 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@saikishor I don't think this will help with the other issue as there are subscribers on the joint state topic - I also explained in the other issue why I think this happens in our setup. Also a small comment from my side on this patch. I am not sure how ROS2 actually determines the amount of subscribers of a topic. I could imagine that this might be a rather expensive operation. It might be cheaper just to publish the data. |
I thought the overhead might come from the dynamic_joint_states.
I don't think it's expensive operation, I have used in RT loops and didn't find any major jitters |
I see. I can hopefully give it a try next week if our setup is free. |
| @@ -351,7 +351,9 @@ controller_interface::return_type JointStateBroadcaster::update( | |||
| state_interface.get_value()); | |||
| } | |||
|
|
|||
| if (realtime_joint_state_publisher_ && realtime_joint_state_publisher_->trylock()) | |||
| if ( | |||
| realtime_joint_state_publisher_ && joint_state_publisher_->get_subscription_count() > 0u && | |||
There was a problem hiding this comment.
looks good. Are the rmw and implementations thereof doing this implicitly already?
There was a problem hiding this comment.
meaning subscription count is polled outside the RT loop where it is definitely safe, right? To my best understanding, the publish operation just appends a buffer
There was a problem hiding this comment.
Yes! That's what I understood
|
So we wait for @firesurfer 's feedback? |
Yeah, let's wait for his feedback just in case. |
|
Hey guys. Unfortunately I wasn't able to test this patch on our setup. Given that this patch does not make things worse:
I would say feel free to merge it. |
|
If it's not broken, don't fix it. Might make sense to benchmark this before modifying such a sensitive line of code based on empirical insights |
|
wasn't the initial issue closed? |
@mhubii Will you be able to test it on your setup? |
|
essentially run a mock JSB with and without modification, spawn subscriptions + monitor CPU load and cycle time / jitter each across all available middlewares? wonder whether this could be done using something like Google benchmark on an abstract level |
|
Change of heart, I am with @mhubii on this. Instead of benchmarking though I think we should look closer at the JSB with a profiler first. The changes suggested on this PR do not fix the underlying issue, only avoid them when there aren't subscribers which will lead to a massive performance change when there is a subscriber. Let's fix the root cause instead. |
might help with #1409
@firesurfer can you check if this helps?