refactor(bridge): reduce standard bridge manager's memory usage#1276
refactor(bridge): reduce standard bridge manager's memory usage#1276
Conversation
Signed-off-by: bdm-k <kokusyunn@gmail.com>
…s into DirectedBridgeRef Signed-off-by: bdm-k <kokusyunn@gmail.com>
Signed-off-by: bdm-k <kokusyunn@gmail.com>
There was a problem hiding this comment.
Pull request overview
Refactors the standard bridge manager to reduce memory usage by storing a compact per-topic entry instead of full MqMsgBridge requests for both directions, and updates the loader/factory plumbing accordingly (including a delegation corner-case guard).
Changes:
- Replace stored per-topic
BridgeInfo(twoMqMsgBridges) with compactManagedBridgeEntry+BridgeFactorySpecstored at request reception time. - Update
StandardBridgeLoaderAPI and caching to resolve factories from the compact spec and create bridges using(node, topic_name, qos)factories. - Add handling for delegation requests arriving after the owner manager has already dropped the topic from
managed_bridges_.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/agnocastlib/src/bridge/standard/agnocast_standard_bridge_manager.cpp | Decomposes incoming requests into compact entries, updates activation/delegation flow to use DirectedBridgeRef, and adds corner-case delegation handling. |
| src/agnocastlib/src/bridge/standard/agnocast_standard_bridge_loader.cpp | Refactors factory resolution to use BridgeFactorySpec (optional lib path + two offsets) and instantiates bridges from topic name + direction. |
| src/agnocastlib/include/agnocast/bridge/standard/agnocast_standard_bridge_manager.hpp | Introduces ManagedBridgeEntry/DirectedBridgeRef and switches loader storage to std::unique_ptr. |
| src/agnocastlib/include/agnocast/bridge/standard/agnocast_standard_bridge_loader.hpp | Changes public loader/factory types (BridgeFn, BridgeFactorySpec) and loader constructor/create API. |
| src/agnocastlib/include/agnocast/bridge/agnocast_bridge_node.hpp | Updates exported factory templates to match the new BridgeFn signature (topic name string instead of BridgeTargetInfo). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using BridgeFn = std::shared_ptr<BridgeBase> (*)( | ||
| rclcpp::Node::SharedPtr, const BridgeTargetInfo &, const rclcpp::QoS &); | ||
| rclcpp::Node::SharedPtr, const std::string &, const rclcpp::QoS &); | ||
|
|
||
| struct BridgeFactorySpec | ||
| { | ||
| // If set to std::nullopt, the factory functions reside in the main executable. | ||
| std::optional<std::string> shared_lib_path; | ||
| uintptr_t fn_offset_r2a; | ||
| uintptr_t fn_offset_a2r; | ||
| }; |
There was a problem hiding this comment.
Changing BridgeFn (and the corresponding factory functions) signature from (Node, BridgeTargetInfo, QoS) to (Node, std::string, QoS) is a public API/ABI break for any out-of-tree bridge factory libraries compiled against the old headers. If external factories/plugins are supported, consider providing a compatibility adapter (e.g., keep the old signature and forward to the new one) or explicitly documenting this as a breaking change and bumping the appropriate version.
Closes #1209
Description
The previous
BridgeInfostruct stored two fullMqMsgBridgeinstances, holding many redundant fields that are identical for both directions.This PR replaces
BridgeInfowith a compact structManagedBridgeEntry:BridgeFactorySpectakes advantage of the fact that thesymbol_namefield inBridgeFactoryInfois only used to identify whether the factories are in the main executable. We setshared_lib_pathtonulloptwhen the factories are in the main executable.Received
MqMsgBridgeis now decomposed intoManagedBridgeEntryat reception time inregister_request(), rather than being stored as is. The parameters of the related functions are updated accordingly.This PR also adds handling of a previously ignored corner case (indeed, this can barely happen): a request is delegated to a bridge manager that has already removed the topic from its
managed_bridges_. This is a timing issue.Related links
How was this PR tested?
bash scripts/test/e2e_test_1to1.bash(required)bash scripts/test/e2e_test_2to2.bash(required)bash scripts/test/run_requires_kernel_module_tests.bash(required)Notes for reviewers