search

ros2 / rmw_zenoh

RMW for ROS 2 using Zenoh as the middleware
Apache License 2.0
184 stars 34 forks source link

Improve performance for waits on subs, clients, services, and guards. #207

Closed clalancette closed 3 months ago

clalancette commented 3 months ago

Basically by combining the check/attach and the detach/check, we can go from locking 4 times per entity to locking twice per entity, which is a large savings here.

clalancette commented 3 months ago

Personally I prefer to name these queue_is_empty..() everywhere as opposed to some being queu_has_data but let me know what you think!

I called it queue_has_data for two reasons:

  1. To make the conditionals inside of rmw_wait use the true path, instead of having to negate them. I find that a bit easier to read.
  2. To better distinguish the "check-and-attach" API calls from the "detach-and-check" API calls. Just looking at rmw_subscription_data_t as an example, we currently have queue_has_data_and_attach_condition_if_not, along with the counterpart detach_condition_and_queue_is_empty. If we were to switch everything to "queue is empty", then we would have queue_is_empty_and_attach_condition_if_so along with detach_condition_and_queue_is_empty. They are just a bit too similar for my taste.

Those are just the reasons I chose what I did, but I'm not tied to that. If you feel that queue_is_empty_and_attach_condition_if_so is clearer, I can go ahead and change it. Thoughts?

clalancette commented 3 months ago

Thanks for explaining the rationale. It makes more sense to me now!

Thanks! I'm going to go ahead and merge this one in, but in the future if we want to change it, it should be easy enough. On to the next rmw_wait fix :).