perf: reduce transitive includes to improve build times - #1786
Draft
mjcarroll wants to merge 5 commits into
Draft
Conversation
- tf_wrapper.hpp: replace 4 heavy tf2 includes (~1000 headers each) with minimal targeted headers (async_buffer_interface.hpp, tf2/time.hpp, direct geometry_msgs headers) and forward declarations for Buffer and TransformListener. Full impl headers moved to tf_wrapper.cpp only. - view_controller.hpp: remove rclcpp/service.hpp and std_srvs/srv/empty.hpp from public header. reset_time_srv_ is now stored as std::shared_ptr<void> (type-erased). Service callback inlined as lambda in view_controller.cpp. Private resetService() method declaration removed entirely. - visual_test_fixture.hpp: remove 6 unnecessary heavy ROS/tf2 includes (rclcpp/rclcpp.hpp, tf2_ros, geometry_msgs, std_msgs) that were not used by VisualTestFixture itself, saving ~900 headers per visual test TU. - view_controller_registration.cpp: consolidate all 6 view controller PLUGINLIB_EXPORT_CLASS macros into a single TU, removing pluginlib/class_list_macros.hpp (~347 headers) from 6 individual .cpp files. Assisted-by: Gemini CLI:Gemini 2.5 Pro [read_file, write_file, run_command, grep_search]
- laser_scan_display.cpp: tf2_ros/buffer.hpp (~1062 headers) replaced with tf2/exceptions.hpp — only tf2::TransformException is used directly. - tf_display.cpp: tf2_ros/transform_listener.hpp (~1057 headers) removed — no tf2_ros:: types are used. tf2_ros/buffer.hpp kept because getBuffer() returns shared_ptr<tf2_ros::Buffer> which is cast to shared_ptr<tf2::BufferCore>. - robot_model_display.cpp: tf2_ros/transform_listener.hpp (~1057 headers) removed entirely — no tf2_ros:: types used directly in this file. Assisted-by: Gemini CLI:Gemini 2.5 Pro [read_file, run_command, grep_search]
visualization_manager.hpp: - Remove tf2_ros/transform_listener.hpp (~1057 headers) — TransformListener only appeared in a doc comment, never as a type. Add explicit rclcpp/executors/single_threaded_executor.hpp which was previously arriving as an undeclared transitive dependency via rclcpp/rclcpp.hpp. ros_node_abstraction_iface.hpp: - Replace rclcpp/rclcpp.hpp with rclcpp/node.hpp — only rclcpp::Node::SharedPtr is used in the interface. This header is included transitively by nearly every display plugin via visualization_manager.hpp → display_context.hpp. ros_node_abstraction.hpp: - Replace rclcpp/rclcpp.hpp with rclcpp/node.hpp + rclcpp/node_options.hpp — the two targeted headers cover rclcpp::Node::SharedPtr and the NodeOptions constructor parameter respectively. Result: rviz_common builds ~38% faster, rviz_visual_testing_framework ~35% faster in incremental builds due to reduced header parse cost per TU. Assisted-by: Gemini CLI:Gemini 2.5 Pro [read_file, run_command, grep_search, write_file]
…registration Instead of parsing pluginlib/class_list_macros.hpp (~347 transitive headers) in each of the 32 display .cpp files and 8 tool .cpp files, move all PLUGINLIB_EXPORT_CLASS macros into two dedicated TUs: displays/display_registration.cpp — 32 display exports tools/tool_registration.cpp — 8 tool exports This mirrors the same pattern already in use for view controllers (view_controller_registration.cpp). Also fix two pre-existing issues uncovered by this change: - accel_display.cpp had PLUGINLIB_EXPORT_CLASS(...AccelDisplay...) but the class is actually AccelStampedDisplay — now correctly registered. - focus_tool.hpp used Ogre::Vector3 in its public interface without including OgreVector.h; the missing include was masked by focus_tool.cpp's own Ogre includes. Added the direct include to the header (IWYU). Assisted-by: Gemini CLI:Gemini 2.5 Pro [run_command, read_file, write_file, grep_search]
Contributor
|
Pulls: #1786
|
…sion)
The display_registration.cpp and tool_registration.cpp approach requires including
ALL 32 display headers (or ALL 8 tool headers) in a single TU because class_loader
uses static_assert(is_base_of<Base,Derived>) which needs complete types. This
makes the registration TU a unity-build bottleneck:
- In parallel CI (4 CPUs) each of the 32 display TUs compiles in parallel, but
display_registration.cpp must compile ALL 32 display headers serially in one
TU, becoming the critical-path bottleneck.
- Result: rviz_default_plugins build time: 4m51s (baseline) → 17m38s (our branch)
The view_controller_registration.cpp pattern works because 6 view controllers all
share very similar, lightweight headers. The display/tool plugin set is too diverse
for this approach.
This revert preserves:
- OgreVector.h in focus_tool.hpp (IWYU: header uses Ogre::Vector3 in its
public interface but was relying on translation-unit-level includes)
Also fix pre-existing uncrustify violation in view_controller.cpp (extra blank line
between function definitions, unmasked by fresh test run on the CI agent).
Assisted-by: Gemini CLI:Gemini 2.5 Pro [run_command, read_file, grep_search]
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reduces CI build times for
rviz_common,rviz_default_plugins, andrviz_visual_testing_frameworkby eliminating unnecessary transitive header includes.Upstream CI baseline for comparison: https://ci.ros2.org/job/ci_linux/29363/
(rviz_common: 40.5s, rviz_default_plugins: 4m51s with warm ccache, 4 CPUs)
Assisted-by: Gemini CLI:Gemini 2.5 Pro [run_command, read_file, grep_search, write_file, search_web]