-
Notifications
You must be signed in to change notification settings - Fork 255
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
Performance improvements in Rosbag2 recorder discovery #1825
Performance improvements in Rosbag2 recorder discovery #1825
Conversation
d141e90
to
80a9816
Compare
@MichaelOrlov @Rayman so this discovery improvement provided originally #1466, kinda rolled back because of #1480, right? just checking if i understand correctly. CC: @Barry-Xu-2018 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that checking recording options parameter and return immediately before time expensive API call for NodeGraph
would be better for user experience.
Don't warn for unknown types if topics are not selected
Move checks for single type and hidden topics to the bottom
Move checks for single type and hidden topics to the bottom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, with the corrected PR title and description, it looks good to me.
Pulls: #1825 |
https://github.com/Mergifyio backport jazzy |
✅ Backports have been created
|
This PR improves the performance of the Rosbag2 recorder discovery by moving graph checks such as
topic_is_unpublished(topic_name, *node_graph_))
andis_leaf_topic(topic_name, *node_graph_))
, which are expensive to the end of the checklist.This PR is similar to the fix from 51a83f4 which was discussed in CPU overhead when discovery is used #1485. This gives a massive CPU improvement.
In the screenshot below you can see the performance difference: