feat(autoware_pointcloud_preprocessor): reuse collectors to reduce creation of collector and timer #10074

vividf · 2025-02-05T12:23:18Z

Description

#8300 introduces a collector and initializes a timer in each collector with a 100 ms interval, which leads to infinite object creation and deletion.

This PR reuses collectors that are no longer in use (i.e., those that have completed concatenation) by resetting their timers, effectively converting them into new collectors. This approach helps limit unnecessary collector creation and timer initialization.

How was this PR tested?

Test with unit test.
Test with rosbag that provided in feat(autoware_pointcloud_preprocessor): redesign concatenate and time sync node #8300
Test with CARET and proved that it works better than before TIER4_INTERNAL_LINK

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Signed-off-by: vividf <[email protected]>

github-actions · 2025-02-05T12:23:36Z

Thank you for contributing to the Autoware project!

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

Please ensure:

You've checked our contribution guidelines.
Your PR follows our pull request guidelines.
All required CI checks pass before marking the PR ready for review.

codecov · 2025-02-05T12:41:14Z

Codecov Report

Attention: Patch coverage is 56.25000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 28.32%. Comparing base (bd84834) to head (a8444cb).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
...oncatenate_data/concatenate_and_time_sync_node.cpp	53.33%	11 Missing and 3 partials ⚠️
...processor/src/concatenate_data/cloud_collector.cpp	61.11%	2 Missing and 5 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10074   +/-   ##
=======================================
  Coverage   28.32%   28.32%           
=======================================
  Files        1485     1486    +1     
  Lines      111090   111153   +63     
  Branches    43150    43176   +26     
=======================================
+ Hits        31466    31487   +21     
- Misses      76603    76643   +40     
- Partials     3021     3023    +2

Flag	Coverage Δ		*Carryforward flag
differential	`22.88% <56.25%> (?)`
differential-cuda	`20.64% <56.25%> (?)`
total	`28.31% <ø> (-0.01%)`	⬇️	Carriedforward from 81ec941

*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

LGTM in codespace, but I did not check with any rosbags.

mojomex

Good stuff!
I've left a few comments in places I think could be simplified or where I'd like the more safety guarantees.

mojomex · 2025-02-07T05:41:33Z

...ing/autoware_pointcloud_preprocessor/src/concatenate_data/concatenate_and_time_sync_node.cpp

+    for (auto & collector : cloud_collectors_) {
+      if (collector->get_status() == CollectorStatus::Idle) {
+        selected_collector = collector;
+        break;
+      }
+    }


This can be expressed as a std::find_if instead

Thanks! fixed in c99f1fe

sensing/autoware_pointcloud_preprocessor/src/concatenate_data/cloud_collector.cpp

mojomex · 2025-02-07T05:52:45Z

sensing/autoware_pointcloud_preprocessor/src/concatenate_data/cloud_collector.cpp

+    const auto period_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
+      std::chrono::duration<double>(timeout_sec_));
+    try {
+      set_period(period_ns.count());
+    } catch (rclcpp::exceptions::RCLError & ex) {
+      RCLCPP_WARN_THROTTLE(
+        ros2_parent_node_->get_logger(), *ros2_parent_node_->get_clock(), 5000, "%s", ex.what());
+    }
+    timer_->reset();


Is this needed? It seems to me reading the RCL source code that the period is unaffected by cancel() and so just calling reset() without re-setting the period should be fine, right?

True, thanks for pointing out!
cbb94ec

...include/autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.hpp

mojomex · 2025-02-07T06:00:59Z

...ing/autoware_pointcloud_preprocessor/src/concatenate_data/concatenate_and_time_sync_node.cpp

+  if (cloud_collectors_.size() > collectors_threshold) {
+    RCLCPP_WARN_STREAM_THROTTLE(
+      get_logger(), *get_clock(), 1000,
+      "The number of cloud collectors (" << cloud_collectors_.size() << ") exceeds the threshold ("
+                                         << collectors_threshold
+                                         << "), be careful if it keeps increasing.");
+  }
 }


I'd like this to be a hard limit. In production, nobody will look at those logs, so the memory footprint could increase forever.

Thanks!
Fixed with resetting collector that has the oldest timestamp:
81ec941 and 3375c43

fix logging: a8444cb

Signed-off-by: vividf <[email protected]>

mojomex

Probably hard-coding number of collectors to 2 or 3 would be okay. I don't see a use case with more collectors at the moment.
Also would allow us to allocate all of them once on startup instead of dynamically.

mojomex · 2025-02-07T08:03:53Z

...ing/autoware_pointcloud_preprocessor/src/concatenate_data/concatenate_and_time_sync_node.cpp

As discussed with @drwnz just now, it's probably a goof idea to allocate (on startup) collectors_threshold collectors and then not add/remove any during operation.
This makes it easier to reason about the program state.
(would also get rid of L251-L260)

Thanks!!!
Fixed in 85f75e6

mojomex · 2025-02-07T08:04:09Z

...ing/autoware_pointcloud_preprocessor/src/concatenate_data/concatenate_and_time_sync_node.cpp

+
+    // Reset the collector with the oldest timestamp if found
+    if (min_it != cloud_collectors_.end()) {
+      RCLCPP_WARN_STREAM(


Probably warn_throttle would be good here as this can run n_lidars x frequency times per second in the worst case.

Thanks, fixed in a8b512f

Signed-off-by: vividf <[email protected]>

drwnz · 2025-02-07T07:21:21Z

...ing/autoware_pointcloud_preprocessor/src/concatenate_data/concatenate_and_time_sync_node.cpp

+    if (!selected_collector) {
+      // If no idle collector exists, create a new collector
+      selected_collector = std::make_shared<CloudCollector>(
+        std::dynamic_pointer_cast<PointCloudConcatenateDataSynchronizerComponent>(


Dynamic creation of a new collector may not be ideal. How about we have a fixed number of collectors that are initialized, and then use the first idle one - if none are idle, kick the oldest one and reuse.

Thanks!!!
Fixed in 85f75e6

Signed-off-by: vividf <[email protected]>

feat: reuse collectors

2f0ff33

Signed-off-by: vividf <[email protected]>

vividf added 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) labels Feb 5, 2025

vividf self-assigned this Feb 5, 2025

vividf requested review from amc-nu, badai-nguyen, drwnz, knzo25, kyoichi-sugahara, meliketanrikulu, miursh, YoshiRi and yukkysaito as code owners February 5, 2025 12:23

vividf requested a review from mojomex February 5, 2025 12:23

YoshiRi reviewed Feb 7, 2025

View reviewed changes

vividf mentioned this pull request Feb 7, 2025

fix(autoware_pointcloud_preprocessor): fix potential double unlock in concatenate node #10082

Open

vividf requested a review from YoshiRi February 7, 2025 04:36

mojomex requested changes Feb 7, 2025

View reviewed changes

vividf added 4 commits February 7, 2025 15:10

chore: remove for-loop to find_if

c99f1fe

Signed-off-by: vividf <[email protected]>

chore: remove set period

cbb94ec

Signed-off-by: vividf <[email protected]>

chore: remove oldest timestamp

81ec941

Signed-off-by: vividf <[email protected]>

chore: fix managing collector list logic

3375c43

Signed-off-by: vividf <[email protected]>

vividf requested a review from mojomex February 7, 2025 06:59

chore: fix logging

a8444cb

Signed-off-by: vividf <[email protected]>

mojomex requested changes Feb 7, 2025

View reviewed changes

feat: change to THROTTLE

a8b512f

Signed-off-by: vividf <[email protected]>

drwnz reviewed Feb 7, 2025

View reviewed changes

feat: initialize required number of collectors when the node start

85f75e6

Signed-off-by: vividf <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(autoware_pointcloud_preprocessor): reuse collectors to reduce creation of collector and timer #10074

feat(autoware_pointcloud_preprocessor): reuse collectors to reduce creation of collector and timer #10074

vividf commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

YoshiRi left a comment

mojomex left a comment

mojomex Feb 7, 2025

vividf Feb 7, 2025

mojomex Feb 7, 2025

vividf Feb 7, 2025

mojomex Feb 7, 2025

vividf Feb 7, 2025 •

edited

Loading

vividf Feb 7, 2025

mojomex left a comment

mojomex Feb 7, 2025

vividf Feb 7, 2025

mojomex Feb 7, 2025

vividf Feb 7, 2025

drwnz Feb 7, 2025

vividf Feb 7, 2025

feat(autoware_pointcloud_preprocessor): reuse collectors to reduce creation of collector and timer #10074

Are you sure you want to change the base?

feat(autoware_pointcloud_preprocessor): reuse collectors to reduce creation of collector and timer #10074

Conversation

vividf commented Feb 5, 2025 • edited Loading

Description

Related links

How was this PR tested?

Notes for reviewers

Interface changes

Effects on system behavior

github-actions bot commented Feb 5, 2025 • edited Loading

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

YoshiRi left a comment

Choose a reason for hiding this comment

mojomex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vividf Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojomex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vividf commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

vividf Feb 7, 2025 •

edited

Loading