Optimization of the SingleThreadedExecutor and MultiThreadedExecutor to reduce CPU overhead. #825

Activity

1. iluetkeb commented on Aug 15, 2019

   

   iluetkeb

   on Aug 15, 2019

   Last edited by iluetkeb

   Contributor

   More actions

   Work with callback groups instead of nodes

   So far I have not seen data that points to nodes vs. callback groups being a decisive factor. Have I missed something?

   Use events to notify executor of changes to system instead of constant checking with weak nodes

   Here, you mean changes such as adding/removing subscribers, right?
2. MartinCornelis2 commented on Aug 15, 2019

   

   MartinCornelis2

   on Aug 15, 2019

   ContributorAuthor

   More actions

   So far I have not seen data that points to nodes vs. callback groups being a decisive factor. Have I missed something?

   My problem with working with nodes is that a node has no meaning to the executor. The executor is concerned with callbacks and their priorities. The concept of callback groups is interesting, because they could imply a certain priority, nodes however do nothing (unless I'm missing something). This is more of a semantic issue or nuisance I'm having, so yes maybe for clarity I should have left this out.

   Here, you mean changes such as adding/removing subscribers, right?

   Yes this would prevent constant checking.
3. iluetkeb commented on Aug 15, 2019

   

   iluetkeb

   on Aug 15, 2019

   Contributor

   More actions

   My problem with working with nodes is that a node has no meaning to the executor.

   Callbacks from different nodes can safely be executed in parallel.

   I’m not aware that this is currently exploited anywhere, but just saying.

   The same holds between callback groups, so those would be sufficient as well, if we take care to map each node to a different group.
4. ivanpauno commented on Aug 16, 2019

   

   ivanpauno

   on Aug 16, 2019

   Member

   More actions

   Work with callback groups instead of nodes

   That would avoid this kind of logic:
   

   rclcpp/rclcpp/src/rclcpp/memory_strategy.cpp

   Lines 20 to 49 in 25f1969

   	rclcpp::SubscriptionBase::SharedPtr
   	MemoryStrategy::get_subscription_by_handle(
   	std::shared_ptr<const rcl_subscription_t> subscriber_handle,
   	const WeakNodeList & weak_nodes)
   	{
   	for (auto & weak_node : weak_nodes) {
   	auto node = weak_node.lock();
   	if (!node) {
   	continue;
   	}
   	for (auto & weak_group : node->get_callback_groups()) {
   	auto group = weak_group.lock();
   	if (!group) {
   	continue;
   	}
   	for (auto & weak_subscription : group->get_subscription_ptrs()) {
   	auto subscription = weak_subscription.lock();
   	if (subscription) {
   	if (subscription->get_subscription_handle() == subscriber_handle) {
   	return subscription;
   	}
   	if (subscription->get_intra_process_subscription_handle() == subscriber_handle) {
   	return subscription;
   	}
   	}
   	}
   	}
   	}
   	return nullptr;
   	}

   If all the nodes are only using the default callback group, there's not much performance benefit of working with callback groups. But I agree the change is a good idea.

   ---

   There's some duplication of data between memory_strategy and wait_set that can be affecting performance. What wait_for_work method is doing:

   • Entities of all the nodes are collected
   • All the entities are copied to a wait_set
   • It waits
   • It checks all the wait set entities and delete them from memory_strategy if needed

   If we would avoid this back and forth between entities in memory_strategy and wait_set (be just using directly the entities in wait_set) we will probable improve performance.

   ---

   Another idea. We're collecting entities from all the nodes in each iteration. If instead of that, we would collect them once and have a way of notify the executor that an entity was added (by storing a shared_ptr of the executor in the nodes for example), we could improve the performance a little bit more. We will still need to copy the entities once, because after waiting the entities that were not notified are cleared (but copying is less expensive that collecting).

   👍React with 👍1Reacted by William Woodall
5. nnmm

   added a commit that references this issue on Jul 9, 2022

   Update maintainers (ros2#825)

   ...

   0ca8c61