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(distortion_corrector_node): performance tuning #2913

Merged
merged 21 commits into from
Mar 6, 2023
Merged

perf(distortion_corrector_node): performance tuning #2913

merged 21 commits into from
Mar 6, 2023

Conversation

sykwer
Copy link
Member

@sykwer sykwer commented Feb 20, 2023 •
edited
Loading

Description

This PR makes distortion_corrector_node faster without changing the logical output (slight changes by approximate calculation of sin/cos). The processing of the distortion_corrector_node gets speeded up from about 21ms to 13ms under the following condition.

distortion_corrector_node_performance_improvement_page-0001

This PR contains two kinds of changes.

  • Avoid unnecessary instantiation of objects
  • Pre-compute sin/cos values

It should be noted that the latter change results in a slight approximation of the logical output.

Related links

Background, Implementation Detail, and Performance Analysis

Tests performed

Check if distortion_corrector_node publishes the same logical output as before (using Autoware Universe rosbag simulation).

The output is very slightly different in some parts since sin/cos is approximated by precomputation.

Notes for reviewers

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Feb 20, 2023
@sykwer sykwer changed the title perf(distortion_corrector_node): Performance tuning perf(distortion_corrector_node): performance tuning Feb 20, 2023
@sykwer sykwer self-assigned this Feb 20, 2023
@codecov
Copy link

codecov bot commented Feb 20, 2023 •
edited
Loading

Codecov Report

Patch coverage: 38.09% and project coverage change: +0.19 🎉

Comparison is base (36fcf3f) 11.98% compared to head (c545cc2) 12.17%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2913      +/-   ##
==========================================
+ Coverage   11.98%   12.17%   +0.19%     
==========================================
  Files        1315     1370      +55     
  Lines       91518   100422    +8904     
  Branches    24277    30072    +5795     
==========================================
+ Hits        10968    12230    +1262     
- Misses      69187    75623    +6436     
- Partials    11363    12569    +1206
Flag Coverage Δ *Carryforward flag
differential 14.02% <38.09%> (?)
total 11.83% <ø> (-0.16%) ⬇️ Carriedforward from 75aaee6

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

Impacted Files Coverage Δ
.../src/distortion_corrector/distortion_corrector.cpp 0.00% <0.00%> (ø)
...autoware_utils/test/src/math/test_trigonometry.cpp 75.00% <75.00%> (ø)
...mon/tier4_autoware_utils/src/math/trigonometry.cpp 100.00% <100.00%> (ø)
...vehicle_model/vehicle_model_bicycle_kinematics.cpp 62.06% <0.00%> (-29.60%) ⬇️
planning/obstacle_avoidance_planner/src/node.cpp 3.58% <0.00%> (-10.00%) ⬇️
...nner/src/vehicle_model/vehicle_model_interface.cpp 76.47% <0.00%> (-9.25%) ⬇️
...eespace_planning_algorithms/abstract_algorithm.hpp 42.85% <0.00%> (-7.15%) ⬇️
common/osqp_interface/src/osqp_interface.cpp 43.15% <0.00%> (-6.85%) ⬇️
...anning/obstacle_velocity_limiter/src/obstacles.cpp 46.87% <0.00%> (-3.13%) ⬇️
..._geometry/include/geometry/spatial_hash_config.hpp 78.87% <0.00%> (-1.62%) ⬇️
... and 123 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sykwer sykwer marked this pull request as ready for review February 20, 2023 15:11
@sykwer sykwer requested review from amc-nu, miursh, yukkysaito and a team as code owners February 20, 2023 15:11
miursh
miursh reviewed Feb 21, 2023
View reviewed changes
Copy link
Contributor

@miursh miursh left a comment

Choose a reason for hiding this comment

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

That's a great improvement. Thank you so much.
Just one question from me.

...d_preprocessor/include/pointcloud_preprocessor/distortion_corrector/distortion_corrector.hpp Outdated Show resolved Hide resolved
@miursh
Copy link
Contributor

miursh commented Feb 25, 2023

I checked the performance.
Processing time is reduced by around 60% no my desktop PC.

proc time [ms] before after
average 12.27 7.31
max 29.00 22.00
std.dev 1.63 1.20

image

@sykwer
Place sincos precompute under util
4f5f089 
Signed-off-by: Takahiro Ishikawa <[email protected]>
@github-actions github-actions bot added the component:common Common packages from the autoware-common repository. (auto-assigned) label Feb 27, 2023
@sykwer
Copy link
Member Author

sykwer commented Feb 28, 2023

Accuracy of approximated sin/cos functions are 10^(-6) ~ 10^(-5).

@sykwer sykwer marked this pull request as ready for review February 28, 2023 15:35
sykwer and others added 3 commits March 1, 2023 09:49
@sykwer
Add test for trigonometry
9dfa3c6 
Signed-off-by: Takahiro Ishikawa <[email protected]>
@pre-commit-ci
style(pre-commit): autofix
de6cb7d
@sykwer
Fix
96ffc12 
Signed-off-by: Takahiro Ishikawa <[email protected]>
@sykwer
Fix
81ec153 
Signed-off-by: Takahiro Ishikawa <[email protected]>
yukkysaito
yukkysaito approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

@sykwer sykwer enabled auto-merge (squash) March 3, 2023 08:15
@yukkysaito yukkysaito self-requested a review March 5, 2023 10:59
yukkysaito
yukkysaito approved these changes Mar 5, 2023
View reviewed changes
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

@yukkysaito
Copy link
Contributor

yukkysaito commented Mar 5, 2023 •
edited
Loading

@sykwer
Many points from clang-tiny, and I think some of them are decent points, so please correct them if you have time.

@yukkysaito yukkysaito disabled auto-merge March 6, 2023 02:59
@yukkysaito yukkysaito merged commit b72ca85 into autowarefoundation:main Mar 6, 2023
1222-takeshi pushed a commit to 1222-takeshi/autoware.universe that referenced this pull request Mar 6, 2023
@sykwer @pre-commit-ci @1222-takeshi
perf(distortion_corrector_node): performance tuning (autowarefoundati…
86de1cf 
…on#2913)

* Avoid unnecessary object instantiation

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

* Fix: Avoid unnecessary object instantiation

Signed-off-by: Takahiro Ishikawa <[email protected]>

* Minimize object instantiation

Signed-off-by: Takahiro Ishikawa <[email protected]>

* Avoid transform computation if possible

Signed-off-by: Takahiro Ishikawa <[email protected]>

* Pre-compute sin/cos

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

* Place sincos precompute under util

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

* Use hardcoded sin values for approcimation

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

* Use new approximated sin/cos functions

Signed-off-by: Takahiro Ishikawa <[email protected]>

* Improve accuracy of approximated sin/cos

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

* Add test for trigonometry

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

* Fix

Signed-off-by: Takahiro Ishikawa <[email protected]>

* Fix

Signed-off-by: Takahiro Ishikawa <[email protected]>

* Conform to clang-tidy

Signed-off-by: Takahiro Ishikawa <[email protected]>

* style(pre-commit): autofix

---------

Signed-off-by: Takahiro Ishikawa <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@tier4-autoware-public-bot tier4-autoware-public-bot bot mentioned this pull request Mar 7, 2023
Merged
tkimura4 added a commit to tier4/autoware.universe that referenced this pull request May 12, 2023
@tkimura4
Revert "perf(distortion_corrector_node): performance tuning (autoware…
8d5592d 
…foundation#2913)"

This reverts commit b72ca85.

Signed-off-by: tomoya.kimura <[email protected]>
1222-takeshi added a commit to tier4/autoware.universe that referenced this pull request May 23, 2023
@1222-takeshi
Revert "perf(distortion_corrector_node): performance tuning (autoware…
efbad4a 
…foundation#2913)"

This reverts commit b72ca85.
@1222-takeshi 1222-takeshi mentioned this pull request May 23, 2023
Merged
7 tasks
tkimura4 pushed a commit to tier4/autoware.universe that referenced this pull request May 23, 2023
@1222-takeshi
revert(distortion corrector_node): performance tuning (autowarefounda…
19a8b0a 
…tion#2913) (#512)

Revert "perf(distortion_corrector_node): performance tuning (autowarefoundation#2913)"

This reverts commit b72ca85.
@yukkysaito yukkysaito mentioned this pull request Apr 8, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wep21 wep21 wep21 left review comments

@yukkysaito yukkysaito yukkysaito approved these changes

@amc-nu amc-nu Awaiting requested review from amc-nu

@TakaHoribe TakaHoribe Awaiting requested review from TakaHoribe

@takayuki5168 takayuki5168 Awaiting requested review from takayuki5168

@purewater0901 purewater0901 Awaiting requested review from purewater0901

@miursh miursh Awaiting requested review from miursh

@kenji-miyake kenji-miyake Awaiting requested review from kenji-miyake

Assignees

@sykwer sykwer

Labels
component:common Common packages from the autoware-common repository. (auto-assigned) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sykwer @miursh @yukkysaito @kenji-miyake @wep21 @mitsudome-r