Minimize header includes by moving impl to .cpp files #331
Conversation
Rayman
force-pushed
the
refactor/minimize-header-includes
branch
from
7357420
to
2eada65
Compare
|
Hello, can I get some feedback on my PR? |
|
Hi @Rayman. To be honest, I prefer the header-based approach right now for simplicity. Do you know that this has a noticeable speed improvement? |
|
The templates from rclcpp are relatively heavy, especially Also the include With this PR our software still compiles perfectly fine, so no change needed in dependent software If you want I can show you some measurements, the results are not big but multiplied by every node in our system (100s) it becomes noticeable. |
| #include "rclcpp/rclcpp.hpp" | ||
|
|
There was a problem hiding this comment.
I see that it was probably wrong that this was missing before. But now it is technically a breaking change.
There was a problem hiding this comment.
Yes it is a breaking change. So this should not be merged into iron, but in rolling. I'll then get released for jazzy
|
I did some quick tests with a simple node that has a diagnostic_updater. The compilation time went from 5 to 2.5 seconds. I only measured this once so there could be some variability.
|
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* Minimize header includes by moving impl to .cpp files * Make sure to build a shared library (cherry picked from commit 1af8c80) # Conflicts: # diagnostic_updater/include/diagnostic_updater/diagnostic_updater.hpp
* Minimize header includes by moving impl to .cpp files * Make sure to build a shared library Signed-off-by: Christian Henkel <[email protected]>
…usage of rclcpp::ok with a non-default context (#352) (#390) * Minimize header includes by moving impl to .cpp files (#331) * Minimize header includes by moving impl to .cpp files * Make sure to build a shared library Signed-off-by: Christian Henkel <[email protected]> * Fix usage of rclcpp::ok with a non-default context (#352) * Fix usage of rclcpp::ok with a non-default context The current implementation calls `rclcpp::ok` without any arguments, which amounts to verifying that the global default context is valid. In the case where a node is added to a custom context, and the global context is not used, `rclcpp::ok` without any arguments will always return `false` since the global context has never been initialized. To fix it, pass to rclcpp the context that's available via the node's base interface. * Add a test for custom context Signed-off-by: Christian Henkel <[email protected]> --------- Signed-off-by: Christian Henkel <[email protected]> Co-authored-by: Ramon Wijnands <[email protected]> Co-authored-by: Hervé Audren <[email protected]>
I'm trying to improve compilation times. One obvious candidate was this package. I've tried to remove includes of rclcpp (which is huge) and move all implementations to a cpp file. I did not modify any code, only move it.