Re-enable benchmark #1245
Re-enable benchmark #1245
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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. |
Copilot uses AI. Check for mistakes.
| }, | ||
| }, | ||
| // 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; | |
| })(), |
Copilot uses AI. Check for mistakes.
| @@ -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.
Copilot uses AI. Check for mistakes.
| 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.
Copilot uses AI. Check for mistakes.
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