OMPL and Qt API update, bump cmake minimum version requirement#6136
OMPL and Qt API update, bump cmake minimum version requirement#6136mini-1235 wants to merge 19 commits into
Conversation
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Temporarily comment this out Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Pin ros2/rviz to version 2cbba6bbeb32 to avoid Qt5 and Qt6 mismatch. Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
I figured out that the EventsExecutor wasn't included in the last rolling sync, but we do have it in Lyrical, so it seems easier for me to test it in Lyrical :) This PR fixes all the compile errors but is not ready to review/merge, I am opening as draft to check the backward compatibility for Jazzy/Kilted. It also depends on ros2/message_filters#292, ros/rosdistro#51321, BehaviorTree/BehaviorTree.CPP#1139, and an upstream OMPL sync, we should probably merge this after we migrate to Ubuntu 26.04 (most fixes here are related to 26.04 using g++15) |
|
This pull request is in conflict. Could you fix it @mini-1235? |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
Executor Performance Report.docx I finally found some time to generate those flamegraphs today. To speed things up, I had AI put together a quick summary report. I did a scan through it, and for the most part, the findings look pretty reasonable to me. The report also digs into the lifecycle_manager. We definitely see some performance gains there, but if you look closely at the flamegraph, it is still eating up a massive chunk of the CPU (compared to other components like the controller_server or planner_server). Also, like we discussed on slack earlier, if you download the EventsCBGExecutor flamegraph and search for "singlethreaded", you will still see it popping up all over the place. I suspect we are going to see another massive performance once absolutely everything is switched over to the event executor, but waiting for the ros2 core repos to merge those updates first |
|
My apologies for the delay. Saying I've simply been overwhelmed would be a misuse of the word. Can you summarize for me the actions or what you think should happen from the flamegraph? I'm reading from your phrasing you think some things still need to change so I'm not clear if this is ready or not (seems like there's some places the wrong executor is being used still?). Wrt the report: I see that it reduces CPU cycles, but what's the actual % improvement in terms of stable-runtime performance? As your report mentions, CPU cycles doesn't mean the % changes and the % is important to understand.
I disagree with this, we need fast detection failure for all deployments. Rather than simply reducing the rate, I think we should fix it (which AI may well help with) in code changes or in how its run (i.e. executor, in another process, etc).
Have you shown this is actually true? I would be open to moving that to another process if that's the case. I question though if AI is properly diagnosing this though. If this solves it, that seems easy enough. |
|
No worries, I marked this as draft because of the blocking issues I mentioned in the lyrical release ticket. I am also very busy at the moment, feel free to review now or wait until everything is ready :)
I thought your preference was to use SingleThreadedExecutor in the code until the ros2 team tests and agrees to switch everything to EventsExecutor by default. I also have a branch here mini-1235@d779849. Did you change your mind? For context we discussed this on Slack:
Could you take a quick look at the lifecycle manager flamegraph/report and let me know if you are satisfied with the improvement (and the potential improvement once everything switches to EventsExecutor)? If you are not satisfied with the current results, then my proposal in #6061 is probably in the wrong direction, and we will need to think of some alternative approaches
When you ask about % improvement, do you mean total CPU usage? I have been measuring with btop before and after, and it's around 1% as I mentioned on Slack(my CPU is much faster than typical onboard computers, so I would still consider the improvement meaningful)
I would ignore the AI-generated solutions in there, I didn't have time to polish the report. I generated the data by hand and had AI summarize it just to give you a rough before vs after picture. |
|
It is not building for me... Base docker image - "osrf/ros:lyrical-desktop-full" - June 1st 2026 FROM osrf/ros@sha256:e2850b225534d1c9b9f511a1d19ce45bbd632a4711a7f74ef4c466c808fdb909 AS base Relevant dockerfile snippet WORKDIR /opt/nav2_ws Error: |
|
Check the blockers I listed earlier, you might need to build some packages from source before a lyrical sync happens |
|
OK... just needed one more line - building BT from source and it all built on the lyrical image. |
I don't think we need to wait for the ROS team to make it default, but I would want to see testing that this works reliably and has a measurable performance improvement. Once we change it though, we can/should change it everywhere else too including the BT nodes and others you've mentioned in that branch. I think its an all-at-once thing so we can have consistent behavior across the board.
Yes, this is what I mean. And OK - its just not listed here anywhere and I've forgotten the context in Slack after this long :-)
This is what I'm not understanding. There's some potential plans, ideas, and options, but I'm not seeing a recommendation or what works and the analysis to show the impact. As you pointed out in the report, just having a large reduction in CPU cycles is not eq. to having a change in resource utilization. Not to say reducing cycles is bad, but if you're saying that the lifecycle manager is taking a ridiculous amount of compute %, then we should also be measuring CPU % of changes to know if they make an impact on the actual problem. I suspect you actually did a bunch of testing off those ideas and options and have a conclusion that perhaps I'm just missing or maybe you didn't communicate it yet. I might just be tired and missing something obvious between all of these sources. I am starting tomorrow able to get back into normal hours so my thought process will be alot less clouded, but I'm feeling like I'm missing some key details to make decisions or help answer your concerns. Can you be specific and let me know what you changed, tested, and measured -- or concisely what you are trying to get from me that I'm not giving & the context for that in one post? 😆 It seems like we're talking about the new executor for use in Nav2 as well as fixes for the lifecycle manager taking too much compute for what it rightfully does. I don't see a proposal in #6061 that you're referring to 🙃 There's just a flamegraph without context or comparison, so I'm not sure what to make of it honestly in isolation, i.e.:
Did you get to testing and that made the difference to fix the lifecycle manager high compute problem with the current bond rate? |
My previous testing shows it is pretty much stable (the rviz bug I mentioned is hard to reproduce and I am not even sure it is related to the new executor). I think real robot testing would be more reliable here, and that is probably not something I can lead, do you know anyone who might be interested? And should I cherry-pick the commit here?
Yeah that is more of an idea than a proposal. My initial assumption was that "switching to EventExecutor could bring a big reduction in lifecycle manager CPU usage", but from my preliminary testing that doesn't seem to be the case. Based on the flamegraphs, there is some improvement, but it is not that obvious. Right now we are also in a mixed state where SingleThreadedExecutor and EventExecutor are both used in lifecycle manager, I suspect even if we unified everything under EventExecutor (by modifying rclcpp/bond code), it wouldn't bring much more improvement based on what the flamegraphs are showing(but worth testing). Another thing I want to note: from the flamegraph, lifecycle manager is taking roughly 3-6x more CPU than the other individual components. If the goal is to get it closer to the same level as other components, that feels like it might need more fundamental changes beyond just switching the executor So my current take on #6061: there is some benefit, but probably not enough to be obvious. I would love to hear your thoughts as well before I do more testing, like, what kind of reduction would you want to see to consider this worthwhile? Next steps on my end would be to switch bond/rclcpp to EventExecutor as well and remeasure, then see if there is room to optimize further on the hottest paths. But realistically that can only happen around August/September when I am back home |
|
Or simply put(if you agree with me), I think I was wrong about #6061, switching to EventExecutor does bring some benefit, but it doesn't solve it. The gap is still there and we probably need a better solution discussed over there. I don't think we should continue the lifecycle manager discussion in this PR |
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
|
it looks like jazzy + kilted image is not up to date: |
|
This pull request is in conflict. Could you fix it @mini-1235? |
I think we may have surprised there, but I agree I would not put the probabilities very high (but still real).
Agreed, it would be good to understand exactly why this is happening and if there's any obvious solutions to it. What kind of improvements did you see with the events executor? What's the state of this work? It seems to address a few things. Perhaps the non-executor items can be spun out and merged pretty quickly and the executor ones can be WIP/discussion continues to happen for performance improvements. I see no reason to block the vast majority of this PR and since I'm back from vacation now we can iterate quickly |
|
Responding here quickly before I go to bed 😀
I have been a bit too busy recently, and honestly I don't remember the exact numbers without looking at the graph again. I will leave that for later when I have more time to re-test and discuss properly.
The purpose of this PR is to try to migrate to 26.04, but as far as I know there are still a few blockers:
If I remember correctly, there are no other blockers after OMPL is released, but we will see once we have Lyrical CI / Rolling moved to 26.04. I think perhaps, if you have time, you can start reviewing this PR. Most of it is due to compilation errors from the platform migration / API updates. The last few commits, as I noted above, are changes that I think would be good to have before branching off. Some are related to The PR is currently not really related to For the "make everything internal use |
|
(I will resolve the conflict tomorrow / Thursday) |
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
There was a problem hiding this comment.
Mostly just a few questions but LGTM! Interestinly Jazzy continues to work but Kilted doesn't.
Kilted is EOL in 6 months, non-LTS, and is now superseded by Lyrical, so I could also be convinced to simply drop compilation compatibility with it at this point if its too much a hassle to make it work.
|
|
||
| // Check if it is not same with the current one | ||
| if (goal_.path != new_path && new_path != nav_msgs::msg::Path()) { | ||
| if (goal_.path != new_path && !new_path.poses.empty()) { |
There was a problem hiding this comment.
These are actually not the same thing since there is a possibility to have an empty, but "populated" path with no poses. Checking the constructor is to check if there was never a new path in the first place, not simply that its empty.
I know this was to fix a compilation issue, but perhaps we could just change this to:
nav_msgs::msg::Path initialized_path;
if (goal_.path != new_path && new_path != initialized_path) {
I however, don't actually feel that strongly about it. Its just a potential subtle bug that could happen in some situations I'm trying to avoid.
| # event executor + isolated + thread num = 1 | ||
| arguments=['--isolated', '--executor-type', 'events-cbg', | ||
| '--ros-args', '--log-level', log_level, '-p', 'thread_num:=1'], | ||
| # single threaded + isolated | ||
| # arguments=['--isolated', '--executor-type', 'single-threaded', | ||
| # '--ros-args', '--log-level', log_level], |
There was a problem hiding this comment.
Obviously some of this commentary would need to be removed (temporarily) until we actually do migrate to the events executor in another PR?
| void Polygon::getPolygon(std::vector<Point> & poly) const | ||
| { | ||
| poly = poly_; | ||
| poly.assign(poly_.begin(), poly_.end()); |
There was a problem hiding this comment.
Why? This seems immensely silly if we cannot copy a vector
|
This pull request is in conflict. Could you fix it @mini-1235? |
|
Otherwise if you can just fix the conflict with those other changes I can merge |
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*.