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

perf(occupancy_grid_map_outlier_filter): improve performance #5980

Merged
merged 10 commits into from
Mar 14, 2024

Conversation

takanotaiga
Copy link
Contributor

Signed-off-by: Taiga Takano [email protected]

Description

This PR improves performance with the following major changes:

  • Elimination of unnecessary conversions between PCL and ROS Messages.
  • Changing the function used for filtering from radiusSearch to nearestKSearch.
  • Reducing computational load by revising point cloud processing.

Measurement

With this PR, the node (measurement is the overall latency of the callback) will be approximately more than twice as fast.

PERCENTILE Before [ms] After [ms] Improvement Factor
99% 13.0 6.2 2.1 times
90% 10.5 4.4 2.4 times
50% 0.7 0.4 1.8 times

onOccupancyGridMapAndPointCloud2のcall latency

@github-actions github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Dec 27, 2023
@takanotaiga takanotaiga force-pushed the perf_occupancy branch 2 times, most recently from 1ac3db7 to 7726ae6 Compare December 27, 2023 08:57
@veqcc veqcc self-requested a review December 27, 2023 08:59
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@YoshiRi YoshiRi added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Dec 28, 2023
@veqcc
Copy link
Contributor

veqcc commented Dec 28, 2023

If possible, .size() should not be used to many times.
This is because .size() 's complexity depends on its data structure (eg. vector is O(1) but linked list is O(N)) and the internal implementation is usually hidden in libraries.

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 15.04%. Comparing base (8bdb542) to head (e0225cc).
Report is 21 commits behind head on main.

Files Patch % Lines
.../src/occupancy_grid_map_outlier_filter_nodelet.cpp 0.00% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5980      +/-   ##
==========================================
+ Coverage   14.80%   15.04%   +0.24%     
==========================================
  Files        1915     1839      -76     
  Lines      132245   126319    -5926     
  Branches    39317    37969    -1348     
==========================================
- Hits        19576    19009     -567     
+ Misses      90838    86027    -4811     
+ Partials    21831    21283     -548     
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 15.05% <ø> (+0.24%) ⬆️ Carriedforward from 3d1d84e

*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.

@YoshiRi
Copy link
Contributor

YoshiRi commented Jan 15, 2024

@veqcc Is this PR ready for merge?

takanotaiga and others added 7 commits February 18, 2024 22:16
Signed-off-by: Taiga Takano <[email protected]>
Signed-off-by: Taiga Takano <[email protected]>
Signed-off-by: Taiga Takano <[email protected]>
Signed-off-by: Taiga Takano <[email protected]>
Signed-off-by: Taiga Takano <[email protected]>
Signed-off-by: Taiga Takano <[email protected]>
@takanotaiga
Copy link
Contributor Author

@YoshiRi
The PR has been revised to address certain points that were raised, and is now in a state where it can be merged. As for the changes made to the filter part (changes to the nearestKSearch function), these have been reverted.

Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@YoshiRi YoshiRi enabled auto-merge (squash) March 11, 2024 06:19
@YoshiRi YoshiRi disabled auto-merge March 11, 2024 06:22
@YoshiRi YoshiRi self-assigned this Mar 11, 2024
@takanotaiga
Copy link
Contributor Author

I measured the latency in the subscriber function onOccupancyGridMapAndPointCloud2 and mainly improved the latency in splitPointCloudFrontBack. The changes made the other day have not improved the speed as initially anticipated.
splitPointCloudFrontBack
onOccupancyGridMapAndPointCloud2

@veqcc
Copy link
Contributor

veqcc commented Mar 11, 2024

@takanotaiga
Thank you for measuring the performance after improvements!
It looks the first figure indicates that the latency becomes worse, right?

@takanotaiga
Copy link
Contributor Author

@veqcc
When comparing with percentiles, there is a slight improvement.

PERCENTILE Before [ms] After [ms]
99% 16.9 16.4
90% 13.1 12.6
50% 9.2 8.8

@veqcc
Copy link
Contributor

veqcc commented Mar 11, 2024

@takanotaiga
You mean the onOccupancy... function slightly improved, while the splitPoint... function got worse, right?
Your improvements are implemented only in splitPoint..., so it sounds strange for me.
Could you give me more detailed explanation?

@takanotaiga
Copy link
Contributor Author

@veqcc
First, it should be noted that the splitPoint... function is called within the onOccupancy... function. And since the onOccupancy... function is the main callback for this node, it measures it.

I thought that simply displaying the improvement graph of the main callback might not clearly show the difference, so I also displayed the improved splitPoint... function to make the improvement more understandable.

@veqcc
Copy link
Contributor

veqcc commented Mar 11, 2024

@takanotaiga
Thank you! I understand the point.
My question here is "Why splitPoint... latency got larger?".

@takanotaiga
Copy link
Contributor Author

@veqcc

I'm sorry. I misunderstood the graph. Tomorrow, I will check if the numbers are incorrect. It seems that I might have confused "before" and "after."

@takanotaiga
Copy link
Contributor Author

@veqcc
As a result of measuring performance to verify once again based on the feedback, performance improvements were observed in both the splitPoint... function and the onOccupancy... function.

onOccupancyGridMapAndPointCloud2

onOccupancyGridMapAndPointCloud2

before[ms] after[ms]
99% 26.98 24.60
90% 21.29 18.98
50% 13.88 12.77

splitPointCloudFrontBack

splitPointCloudFrontBack

before[ms] after[ms]
99% 2.20 1.02
90% 1.58 0.63
50% 1.07 0.38

Copy link
Contributor

@veqcc veqcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@YoshiRi YoshiRi enabled auto-merge (squash) March 14, 2024 02:15
@YoshiRi YoshiRi merged commit 3c01dc9 into autowarefoundation:main Mar 14, 2024
24 of 25 checks passed
yhisaki pushed a commit to yhisaki/autoware.universe that referenced this pull request Mar 15, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
kaigohirao pushed a commit to kaigohirao/autoware.universe that referenced this pull request Mar 22, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
Signed-off-by: kaigohirao <[email protected]>
0x126 pushed a commit to tier4/autoware.universe that referenced this pull request Apr 3, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
YoshiRi added a commit to tier4/autoware.universe that referenced this pull request Apr 9, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
badai-nguyen pushed a commit to tier4/autoware.universe that referenced this pull request Apr 16, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
0x126 pushed a commit to tier4/autoware.universe that referenced this pull request Apr 19, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
badai-nguyen pushed a commit to tier4/autoware.universe that referenced this pull request Apr 22, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
badai-nguyen pushed a commit to tier4/autoware.universe that referenced this pull request Apr 23, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
badai-nguyen pushed a commit to tier4/autoware.universe that referenced this pull request May 9, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
…efoundation#5980)

* improve perfomance

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fixed the bug and corrected the spelling mistake.

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* fix bug

Signed-off-by: Taiga Takano <[email protected]>

* style(pre-commit): autofix

Signed-off-by: Taiga Takano <[email protected]>

* change filter

Signed-off-by: Taiga Takano <[email protected]>

---------

Signed-off-by: Taiga Takano <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Yoshi Ri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants