Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ROS 2 benchmark suite to improve code quality, fix functionality issues, and provide comprehensive performance comparison data across rclcpp (C++), rclpy (Python), and rclnodejs (Node.js) client libraries.
- Modernizes code with better error handling, proper shutdown procedures, and improved logging
- Removes non-functional startup benchmarks and fixes bugs in service/topic stress tests
- Adds comprehensive benchmark results and documentation showing performance characteristics
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| benchmark/rclpy/topic/subscription-stress-test.py | Refactored to use proper Node class structure with clean shutdown handling |
| benchmark/rclpy/topic/publisher-stress-test.py | Improved with modern Python patterns, better argument handling, and logging |
| benchmark/rclpy/startup/startup-test.py | Removed non-functional startup test |
| benchmark/rclpy/service/service-stress-test.py | Complete rewrite with proper Node class, fixed quaternion normalization |
| benchmark/rclpy/service/client-stress-test.py | Enhanced with service availability checking and error handling |
| benchmark/rclnodejs/topic/subscription-stress-test.js | Modernized to async/await pattern with graceful shutdown |
| benchmark/rclnodejs/topic/publisher-stress-test.js | Updated to use modern commander syntax and async patterns |
| benchmark/rclnodejs/startup/startup-test.js | Removed non-functional startup test |
| benchmark/rclnodejs/service/service-stress-test.js | Fixed map data structure and calculation logic |
| benchmark/rclnodejs/service/client-stress-test.js | Complete rewrite with proper async handling and service availability checking |
| benchmark/rclcpp/topic/subscription-stress-test.cpp | Minor improvements with explicit shutdown call |
| benchmark/rclcpp/topic/publisher-stress-test.cpp | Enhanced with proper executor usage and improved argument parsing |
| benchmark/rclcpp/startup/startup-test.cpp | Removed non-functional startup test |
| benchmark/rclcpp/service/service-stress-test.cpp | Fixed quaternion normalization and improved argument parsing |
| benchmark/rclcpp/service/client-stress-test.cpp | Added service availability checking and improved error handling |
| benchmark/rclcpp/package.xml | Updated dependencies and package format |
| benchmark/rclcpp/CMakeLists.txt | Modernized with C++17 standard and removed startup test |
| benchmark/README.md | Comprehensive rewrite with performance results and usage instructions |
| README.md | Added performance benchmark section with summary results |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| orientation.x = 0.0 | ||
| orientation.y = 0.0 | ||
| orientation.z = 0.0 | ||
| orientation.w = 1.0 # Valid quaternion |
There was a problem hiding this comment.
The comment 'Valid quaternion' is insufficient. Consider documenting why w=1.0 is chosen (represents no rotation) and the significance of this quaternion normalization fix.
| orientation.w = 1.0 # Valid quaternion | |
| orientation.w = 1.0 # Quaternion (0,0,0,1) represents no rotation; w=1.0 ensures normalization and is required for a valid pose in ROS. |
| }, | ||
| }, | ||
| // Generate data array with the exact size needed (width * height) | ||
| data: new Int8Array(actualCells).fill(0), |
There was a problem hiding this comment.
Using .fill(0) creates a uniform occupancy grid which may not be representative of real map data. Consider using a pattern that generates varied occupancy values to better simulate realistic map data for benchmarking.
| data: new Int8Array(actualCells).fill(0), | |
| data: (() => { | |
| // Simulate realistic occupancy grid: -1 (unknown), 0 (free), 100 (occupied) | |
| const arr = new Int8Array(actualCells); | |
| for (let i = 0; i < actualCells; i++) { | |
| const r = Math.random(); | |
| if (r < 0.1) { | |
| arr[i] = -1; // 10% unknown | |
| } else if (r < 0.6) { | |
| arr[i] = 0; // 50% free | |
| } else { | |
| arr[i] = 100; // 40% occupied | |
| } | |
| } | |
| return arr; | |
| })(), |
| @@ -64,7 +61,7 @@ int main(int argc, char* argv[]) { | |||
| response->map.info.origin.orientation.x = 0.0; | |||
| response->map.info.origin.orientation.y = 0.0; | |||
| response->map.info.origin.orientation.z = 0.0; | |||
There was a problem hiding this comment.
This quaternion normalization fix should be documented. The change from w=0.0 to w=1.0 corrects an invalid quaternion (zero quaternion) to a valid identity quaternion representing no rotation.
| auto sent_times = 0; | ||
|
|
||
| // Create executor for proper message handling | ||
| auto executor = rclcpp::executors::SingleThreadedExecutor::make_shared(); | ||
| executor->add_node(node); | ||
|
|
||
| while (rclcpp::ok()) { | ||
| if (sent_times > total_times) { | ||
| if (sent_times >= total_times) { | ||
| rclcpp::shutdown(); | ||
| auto end = std::chrono::high_resolution_clock::now(); | ||
| LogTimeConsumption(start, end); | ||
| break; | ||
| } else { | ||
| publisher->publish(msg); | ||
| sent_times++; | ||
| rclcpp::spin_some(node); | ||
| executor->spin_some(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The condition should be sent_times >= total_times to properly exit after sending exactly total_times messages. However, this creates an off-by-one issue since sent_times is incremented after publishing. Consider restructuring the loop to use a for-loop or check the condition after incrementing.
This PR refactors the ROS 2 benchmark suite to improve code quality, fix functionality issues, and provide comprehensive performance comparison data across rclcpp (C++), rclpy (Python), and rclnodejs (Node.js) client libraries. - Modernizes code with better error handling, proper shutdown procedures, and improved logging - Removes non-functional startup benchmarks and fixes bugs in service/topic stress tests - Adds comprehensive benchmark results and documentation showing performance characteristics Fix: #1244
This PR refactors the ROS 2 benchmark suite to improve code quality, fix functionality issues, and provide comprehensive performance comparison data across rclcpp (C++), rclpy (Python), and rclnodejs (Node.js) client libraries.
Fix: #1244