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(pointcloud_preprocessor): prevent excessive log and speed up a bit #6843

Conversation

KYabuuchi
Copy link
Contributor

@KYabuuchi KYabuuchi commented Apr 18, 2024

Description

This PR contains two changes:

  1. Avoiding calling RCLCPP_WARN_STREAM_THROTTLE for a number of points
  2. Removing unnecessary for loops and rclcpp::Time class instantiations

RCLCPP_WARN_STREAM_THROTTLE takes some processing time even if it does not actually output logs, so calling it for a number of points would take a very long time. 😭 In practice, printing once per point cloud is sufficient, so I made that change.

The second change provides a slight speedup. In my environment (with about 300,000 points), it resulted in a maximum speedup of about 10 ms.


Also, if the distortion_corrector is delayed for some reason, the following scenario occurs, which is fatal:

  1. Processing is significantly delayed for some reason.
  2. Twist and IMU are stored in the queue at the correct time, so regardless of which data is paired with the point cloud, the stamp difference is large.
  3. When the stamp difference is large, RCLCPP_WARN_STREAM_THROTTLE is called for each point cloud (~300,000 times).
  4. Even if RCLCPP_WARN_STREAM_THROTTLE does not output logs, calling it many times takes a lot of time.
  5. If step 4 takes too long, the stamp difference for matching between the vehicle speed and IMU in subsequent iterations also becomes large.
  6. Return to step 2 and repeat indefinitely.

When it actually happens, it looks like the image below. This PR resolves this issue.

Tests performed

I ran the logging_simulator on the sample data and measured the processing time. Unfortunately, there was minimal improvement in processing speed with the sample data.
The following graph shows the processing time for two runs before and after the changes. The vertical axis represents processing_time_ms. While there was little effect in the beginning, an improvement can be observed around 80-85 seconds

compare_distortion_corrector

Also, in my environment (with about 300,000 points), it resulted in a maximum speedup of about 10 ms.

Effects on system behavior

The system behavior does not change.

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.

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.

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

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>
Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>
Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>
@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Apr 18, 2024
@KYabuuchi KYabuuchi marked this pull request as ready for review April 18, 2024 07:08
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

@KYabuuchi KYabuuchi added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Apr 18, 2024
@KYabuuchi KYabuuchi enabled auto-merge (squash) April 18, 2024 07:35
@KYabuuchi KYabuuchi merged commit 3adffc3 into autowarefoundation:main Apr 18, 2024
33 of 35 checks passed
@KYabuuchi KYabuuchi deleted the fix/remove_excessive_log_in_distortion_corrector branch April 18, 2024 08:18
YoshiRi pushed a commit to tier4/autoware.universe that referenced this pull request May 13, 2024
…it (autowarefoundation#6843)

* provide only one warning

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>

* associate only one time

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>

* Revert "associate only one time"

This reverts commit 984d028.

* remove redundant for loop and rclcpp::Time instantiation

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>

---------

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
…it (autowarefoundation#6843)

* provide only one warning

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>

* associate only one time

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>

* Revert "associate only one time"

This reverts commit 984d028.

* remove redundant for loop and rclcpp::Time instantiation

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>

---------

Signed-off-by: Kento Yabuuchi <kento.yabuuchi.2@tier4.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (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.

2 participants