Skip to content
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

Construction of transform listener #187

Open
domire8 opened this issue Jan 24, 2025 · 4 comments · May be fixed by #189
Open

Construction of transform listener #187

domire8 opened this issue Jan 24, 2025 · 4 comments · May be fixed by #189
Assignees

Comments

@domire8
Copy link
Member

domire8 commented Jan 24, 2025

Since I was looking a bit closer into tf2_ros I saw that there are actually several ways to construct the TransformListener. Note that we are currently doing that in modulo:

this->tf_listener_ = std::make_shared<tf2_ros::TransformListener>(*this->tf_buffer_);

However, this constructor creates yet another node under the hood, one that we can see as transform_listener_impl_XXX. I don't know why I never questioned this but there is this constructor that takes the node as argument. I believe we should at least give the transform listener access to the node to avoid an additional one being created.

Additionally, there is also the spin_thread argument that defaults to true, see what it does here. It does create another executor under the hood but here I'm not sure if that is a good thing or not.

What do you think @eeberhard @bpapaspyros ?

@domire8 domire8 self-assigned this Jan 24, 2025
@eeberhard
Copy link
Member

Ah, very interesting, good catch. If every component with a transform listener is creating a new node and new executor, that adds a bit of complexity to the ros graph.

Using the parent node certainly makes a lot of sense.

Regarding the spin_thread arg, I see it serving two purposes:

  • it creates an independent callback group, which prevents high-frequency tf messages from blocking other callbacks in the same group like services and so on (priority inversion)
  • it creates a new thread for an executor, which means the independent callback group for tf messages can be executed in parallel even if the node is otherwise part of a single-threaded executor

In principle, it might be nice to use the existing executor for tf listeners - it means we don't rely on the kernel to resolve multithreaded scheduling but instead place the duty on the Executor (which, with the EventsExecutor, is very capable of handling such tasks and would give more control or access for debugging / profiling if needed). But, based on the implementation of the TF listener class, we aren't able to split the logic; we either use the parent executor and have the tf listeners in the default callback group (not ideal, because then they won't be executed in parallel even though they could be), or we have them in a new callback group but also executed by a new thread.

Also, the Python executor is still pretty awful so adding tf callbacks even in a separate callback group could bog it down regardless.

What we can take away from this is that the transform listener class is threadsafe in any case, so apart from the scheduling logic, spinning it in the background threaed is functionally equivalent to spinning it in parallel in a separate callback group of the main thread.

TL;DR, I would pass the parent node interface to the constructor and keep spin_thread true

@domire8
Copy link
Member Author

domire8 commented Jan 31, 2025

That sounds good!

@domire8
Copy link
Member Author

domire8 commented Jan 31, 2025

Interestingly, in Python the spin_thread is false by default

Image

@domire8 domire8 linked a pull request Jan 31, 2025 that will close this issue
1 task
@domire8 domire8 linked a pull request Jan 31, 2025 that will close this issue
1 task
@eeberhard
Copy link
Member

Interestingly, in Python the spin_thread is false by default

I think we should consider benchmarking how this behaves as True, since we noticed executor performance issues with Python components that were specifically noticeable with TF (manifesting as jittery frames which could be caused by blocked execution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants