New inflation layer with optional OpenMP acceleration#5933
New inflation layer with optional OpenMP acceleration#5933SteveMacenski merged 31 commits intoros-navigation:mainfrom
Conversation
Signed-off-by: Tony Najjar <[email protected]>
01201af to
66f46c6
Compare
Signed-off-by: Tony Najjar <[email protected]>
|
Note: This is an AI summary from the benchmarking files Benchmark Comparison SummaryTest EnvironmentsDev Machine (ubuntu@dexory)
Robot (arri-74)
Performance Comparison (Key Benchmarks)1000×1000 Grid (1M cells, 50% occupancy, 2m inflation radius)
2000×2000 Grid (4M cells, 50% occupancy, 2m inflation radius)
3333×3333 Grid (11.1M cells, 50% occupancy, 2m inflation radius)
4000×4000 Grid (16M cells, 30% occupancy, 1m inflation radius)
Key Findings1. New Implementation Impact (OpenMP disabled)
2. OpenMP Parallelization Impact
3. Grid Size Scaling
4. Occupancy Impact (1500×1500 tests)
5. Inflation Radius Impact
Detailed Results by ParameterVarying Occupancy (1500×1500 grid, 2m inflation)
Key Observation: Old implementation degrades significantly with higher occupancy (8→75 ms on dev), while new implementation remains stable (14-16 ms without OpenMP, 4-5 ms with OpenMP). Varying Inflation Radius (1000×1000 grid, 50% occupancy)
Key Observation: Old implementation shows 36% slowdown from smallest to largest radius (11.2→17.6 ms on dev). New implementation shows minimal variation (<3% difference). Varying Cost Scale (1000×1000 grid, 50% occupancy, 2m radius)
Key Observation: Cost scale factor has negligible impact on performance across all implementations. Recommendations✅ Use new implementation with OpenMP enabled - Provides 6.5-15.3× speedup ✅ Even without OpenMP, new implementation is 1.1-3.5× faster ✅ Performance is more predictable and scales better with grid size ✅ Robot shows excellent speedup despite lower CPU frequency ✅ New implementation handles varying occupancy and inflation radii efficiently Performance Summary Chart |
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
SteveMacenski
left a comment
There was a problem hiding this comment.
Last tangible comment.
Otherwise:
- Some ~1 hour of experimentation with using Eigen operations to improve performance further would be nice
- With the naming as it stands, I don't have any other issues. However, I wonder if we should make this the
PotentialFieldInterfaceLayerand be more generic than specifically inflation. With that,getInflationRadius->getPotentialFieldMaxDistanceandgetCostScalingFactorwould need to return more generic weights or a lambda that represents the distance -> cost calculation for other methods to use.
Signed-off-by: Tony Najjar <[email protected]>
Not a bad idea but we can also do the renaming once we have other layers that don't fit the description anymore |
|
OK - check DCO and otherwise LGTM after whatever optimization playgrounding / Dexory testing is required to make you comfortable with me merging this + the docs PR |
I have auto-signoff in vscode, it just doens't work when I revert 😭
Will come back in a couple of weeks! If you want to shoutout to potential testers that also need a faster inflation layer, go ahead btw did you play around with it or the benchmarks yourself? |
|
I did not - I trust you not to make up numbers 😉 |
Signed-off-by: Tony Najjar <[email protected]>
|
LMK when you're ready for merge + link me back to your docs PR to merge at the same time :-) |
|
I tested (rather quickly) implementing some SIMD optimization with the help of Claude but it failed miserably (unreadable and not much performance improvement). If you have a certain direction in mind you can share. Otherwise, I'm ready to merge. I just fixed a merge conflict in ros-navigation/docs.nav2.org#861 cc @doisyg |
|
Codecov didn't upload, I want to see those before merging. I retriggered |
|
@tonynajjar please update one of the nav2 system test configs / launch files to use the legacy inflation layer so it continues to be exercised. This removes all testing of it so its now completely untested 🙃 |
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
|
@SteveMacenski re-added the legacy inflation tests. By the way, I also fixed a build error introduced by the rebase that was not detected by CI because the flag |
Want to add that https://github.com/ros-navigation/navigation2/blob/main/.circleci/config.yml#L218-L221 or https://github.com/ros-navigation/navigation2/blob/main/.circleci/defaults.yaml? Can we also have one of the system tests run the legacy version too? |
Ah yeah
You mean have it build with OPENMP on by default? In this case there is barely any uncovered code for OPENMP disabled so we could go for it but in general I think we should be building with all possible values of flags. |
Signed-off-by: Tony Najjar <[email protected]>
|
Now we get these warnings because ENABLE_OPENMP is set for all packages Probably cleanest to use Regarding the system test using legacy inflation layer, it works -> test 19 doesn't show the log |
|
@SteveMacenski again ready from my side |
|
@tonynajjar disable openmp then for now; its not worth the trouble. We can deal with it later :-) Then I can merge once the docs PR is un-conflicted 😉 |
Signed-off-by: Tony Najjar <[email protected]>
done
What do you mean, it's already merged: ros-navigation/docs.nav2.org#861 |
…#5933) * OpenMP inflation layer Signed-off-by: Tony Najjar <[email protected]> * restart tests Signed-off-by: Tony Najjar <[email protected]> * disable openmp Signed-off-by: Tony Najjar <[email protected]> * enable flag Signed-off-by: Tony Najjar <[email protected]> * limit thread number Signed-off-by: Tony Najjar <[email protected]> * enable openmp for testing Signed-off-by: Tony Najjar <[email protected]> * check docker cores Signed-off-by: Tony Najjar <[email protected]> * set OMP_NUM_THREADS to 4 Signed-off-by: Tony Najjar <[email protected]> * dynamic num_thread Signed-off-by: Tony Najjar <[email protected]> * remove openmp from packages.xml Signed-off-by: Tony Najjar <[email protected]> * turn on by default Signed-off-by: Tony Najjar <[email protected]> * hardcode COST_LUT_PRECISION Signed-off-by: Tony Najjar <[email protected]> * test out big test Signed-off-by: Tony Najjar <[email protected]> * add back legacy layer Signed-off-by: Tony Najjar <[email protected]> * set to 100 Signed-off-by: Tony Najjar <[email protected]> * fix legacy inflation layer Signed-off-by: Tony Najjar <[email protected]> * Revert "fix legacy inflation layer" This reverts commit 21fa149. * off by default Signed-off-by: Tony Najjar <[email protected]> * base class approach Signed-off-by: Tony Najjar <[email protected]> * simplified interface Signed-off-by: Tony Najjar <[email protected]> * add test coverage Signed-off-by: Tony Najjar <[email protected]> * remove cpp file Signed-off-by: Tony Najjar <[email protected]> * Fix broken build Signed-off-by: Tony Najjar <[email protected]> * Add tests for legacy inflation layer Signed-off-by: Tony Najjar <[email protected]> * Enable OpenMP support and add legacy inflation layer test configuration Signed-off-by: Tony Najjar <[email protected]> * lint * lint * remove -DENABLE_OPENMP=ON Signed-off-by: Tony Najjar <[email protected]> --------- Signed-off-by: Tony Najjar <[email protected]> (cherry picked from commit 46172d7)
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
ros-navigation/docs.nav2.org#861
Description of how this change was tested
For Maintainers:
backport-*.