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

[jazzy] Performance improvements in Rosbag2 recorder discovery (backport #1825) #1827

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Oct 3, 2024

  • This PR improves the performance of the Rosbag2 recorder discovery by moving graph checks such as topic_is_unpublished(topic_name, *node_graph_)) and is_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:

Screenshot from 2024-09-27 15-22-25


This is an automatic backport of pull request #1825 done by [Mergify](https://mergify.com).

…#1825)

Reintroduce the fix from 51a83f4 which
was discussed in #1485. This gives
a massive CPU improvement.

Signed-off-by: Ramon Wijnands <ramon.wijnands@nobleo.nl>
(cherry picked from commit e75d6d6)
@mergify mergify bot requested a review from a team as a code owner October 3, 2024 17:42
@mergify mergify bot requested review from emersonknapp and jhdcs and removed request for a team October 3, 2024 17:42
@MichaelOrlov MichaelOrlov changed the title Performance improvements in Rosbag2 recorder discovery (backport #1825) [jazzy] Performance improvements in Rosbag2 recorder discovery (backport #1825) Oct 3, 2024
@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

Pulls: #1827
Gist: https://gist.githubusercontent.com/MichaelOrlov/cd6a7268495cb147c22764baafb2f945/raw/c8bc5d6d497fa2820bb6ffde82cd3ee849c5b68e/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport
TEST args: --packages-above rosbag2_transport
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14673

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit 78f0e80 into jazzy Oct 7, 2024
12 checks passed
@Timple
Copy link

Timple commented Oct 8, 2024

This is great that this is backported! Thank you!

What is kind of the release rithm of rosbag2 jazzy? Would be good to know (kind of) when this would hit the binaries.

@MichaelOrlov MichaelOrlov deleted the mergify/bp/jazzy/pr-1825 branch October 8, 2024 18:50
@MichaelOrlov
Copy link
Contributor

@marcoag The last synch for Jazzy was Sep 6. When we would expect the next synch?

@clalancette
Copy link
Contributor

@marcoag The last synch for Jazzy was Sep 6. When we would expect the next synch?

There was a Jazzy patch release and sync on September 23: https://discourse.ros.org/t/new-packages-and-patch-release-for-jazzy-jalisco-2024/39727

@MichaelOrlov
Copy link
Contributor

Yeah, but the latest tag 0.26.5 for Jazzy in the Rosbag2 repo was created Sep.6

@marcoag
Copy link
Contributor

marcoag commented Oct 9, 2024

I was hoping to do one as soon as we get a fast-rtps and/or cyclone release, let's see how it goes in the next few weeks...

@Timple
Copy link

Timple commented Oct 9, 2024

But this rosbag2 package needs a release first right? Otherwise the sync doesn't pick it up.

@marcoag
Copy link
Contributor

marcoag commented Oct 9, 2024

@Timple yes, we usually do that on the week of the sync so we can catch the latest updates right before the sync happens.

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 this pull request may close these issues.

5 participants