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.

Sign up for GitHub

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

Jump to bottom

perf(autoware_ndt_scan_matcher): replacement of pcl::KdTreeFLANN with nanoflann #10013

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

perf(autoware_ndt_scan_matcher): replacement of pcl::KdTreeFLANN with nanoflann #10013

wants to merge 16 commits into from
+2,620 −8

Conversation

taisa1
Copy link
Contributor

@taisa1 taisa1 commented Jan 23, 2025 •
edited
Loading

Description

This PR speeds up callback_sensor_points_main() on autoware_ndt_scan_matcher without changing the logical output.

The tail latency gets about x1.65 faster with this PR merged.
Screenshot from 2024-12-26 17-57-09

In this PR, I propose to replace pcl::KdTreeFLANN with nanoflann in this node.
nanoflann is a header-only kdtree library, a fork of FLANN used in PCL.

It is provided under the BSD license. kdtree_nanoflann.hpp contains code from nanoflann, so the license is displayed.

nanoflann.hpp is a copy from nanoflann v1.6.3, and is currently placed in include directory of this node, but will need to be moved if it is used on other nodes in the future.

Measurement Condition

  • Ubuntu22.04 + ROS2 Humble + Autoware Universe rosbag simulation
  • not core isolated / not core flequency fixed (so multi-threaded)

Related links

Private Links:

How was this PR tested?

  • I set up an environment that fixes the input to the node, and confirmed that changing to nanoflann results in an increase in speed without changing the output (details in TIER IV internal).
  • I confirmed that it can be run normally with sample rosbag simulation.

Notes for reviewers

In this PR, the library is temporarily placed under the include/ of autoware_ndt_scan_matcher, but

  • nanoflann.hpp is a bit too large (80+ kB) to place as source code
  • There are other nodes that use kdtree

So it might be better to install it in a location like /usr/include/ instead of placing it as source code.

Do you have any comments about it?

Interface changes

None.

Effects on system behavior

None.

@github-actions github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Jan 23, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 23, 2025 •
edited
Loading

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@taisa1 taisa1 marked this pull request as ready for review January 28, 2025 06:34
@SakodaShintaro
Copy link
Contributor

Thank you for the pull request!

Since this pull request has a big impact, we are running tests on all the evaluation data.

TIER IV internal links

Once these are complete, we will check for any errors and compare the processing times with the previous ones.

@SakodaShintaro SakodaShintaro added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jan 29, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 29, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.50000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 29.86%. Comparing base (461dbc1) to head (9203f63).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...de/autoware/ndt_scan_matcher/ndt_omp/nanoflann.hpp 82.79% 14 Missing and 23 partials ⚠️
...ware/ndt_scan_matcher/ndt_omp/kdtree_nanoflann.hpp 77.27% 2 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #10013      +/-   ##
==========================================
+ Coverage   29.21%   29.86%   +0.64%     
==========================================
  Files        1439     1434       -5     
  Lines      108115   108149      +34     
  Branches    42638    42892     +254     
==========================================
+ Hits        31588    32297     +709     
+ Misses      73485    72714     -771     
- Partials     3042     3138      +96
Flag Coverage Δ *Carryforward flag
differential 28.51% <82.50%> (?)
total 29.80% <ø> (+0.59%) ⬆️ Carriedforward from 8fcf88b

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SakodaShintaro
Copy link
Contributor

@taisa1
spell-check-differential detected many errors.
https://github.com/autowarefoundation/autoware.universe/actions/runs/13024889595/job/36332234200?pr=10013

Could you fix them?

ref) https://github.com/streetsidesoftware/cspell/tree/main/packages/cspell#in-document-settings

@taisa1
Copy link
Contributor Author

taisa1 commented Jan 29, 2025

Fixed spell check error.
By the way, it seems that there are some cases in which the evaluation test fails, so maybe I should reconsider the validity?

@SakodaShintaro
Copy link
Contributor

Thank you for the correction.

By the way, it seems that there are some cases in which the evaluation test fails, so maybe I should reconsider the validity?

It's probably fine because some of the data is abnormal data that is expected to fail. I'll check the results tomorrow.

@YamatoAndo
Copy link
Contributor

@taisa1 Thank you for the pull request!

So it might be better to install it in a location like /usr/include/ instead of placing it as source code.
Do you have any comments about it?

Is it not possible to install nanoflann via apt?
If you have made code modifications to nanoflann, I think it would be better to place it in include/autoware/ndt_scan_matcher rather than /usr/include/.

Alternatively, you could create a separate repository for nanoflann and include it in autoware.repos.

@YamatoAndo
Copy link
Contributor

In any case, there is no problem with placing it in include/autoware/ndt_scan_matcher.

@SakodaShintaro
Copy link
Contributor

The results are good. 👍

  • Mean error [m] with respect to the reference trajectory

compare_result_rms_anom_data

Almost no degradation

  • Mean execution time [msec]

compare_result_exe_time_anom_data

Faster than before

I will check the code later.

@taisa1
Copy link
Contributor Author

taisa1 commented Jan 31, 2025

Is it not possible to install nanoflann via apt?

nanoflann v1.4.2 can be installed via apt, but it's older than latest v1.6.3 and there are some fixes, API changes and optimizations between them.

I haven't modified code of nanoflann, but if there is no problem it might be better to place the latest nanoflann in autoware_ndt_scan_matcher (as this PR is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anhnv3991 anhnv3991 Awaiting requested review from anhnv3991 anhnv3991 is a code owner

@KYabuuchi KYabuuchi Awaiting requested review from KYabuuchi KYabuuchi is a code owner

@Motsu-san Motsu-san Awaiting requested review from Motsu-san Motsu-san is a code owner

@RyuYamamoto RyuYamamoto Awaiting requested review from RyuYamamoto RyuYamamoto is a code owner

@SakodaShintaro SakodaShintaro Awaiting requested review from SakodaShintaro SakodaShintaro is a code owner

@TaikiYamada4 TaikiYamada4 Awaiting requested review from TaikiYamada4 TaikiYamada4 is a code owner

@YamatoAndo YamatoAndo Awaiting requested review from YamatoAndo YamatoAndo is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
component:localization Vehicle's position determination in its environment. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
Software Working Group
Status: To Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@taisa1 @SakodaShintaro @YamatoAndo