-
Notifications
You must be signed in to change notification settings - Fork 507
improve lookup time for matches_any_publishers() #3084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rolling
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -386,6 +386,39 @@ class IntraProcessManager | |
| std::vector<uint64_t> take_ownership_subscriptions; | ||
| }; | ||
|
|
||
| /// Hash function for rmw_gid_t to enable use in unordered_map | ||
| struct rmw_gid_hash | ||
| { | ||
| std::size_t operator()(const rmw_gid_t & gid) const noexcept | ||
| { | ||
| // Using the FNV-1a hash algorithm on the gid data | ||
| constexpr std::size_t FNV_prime = 1099511628211u; | ||
| std::size_t result = 14695981039346656037u; | ||
|
|
||
| for (std::size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) { | ||
| result ^= gid.data[i]; | ||
| result *= FNV_prime; | ||
| } | ||
| return result; | ||
| } | ||
| }; | ||
|
|
||
| /// Equality comparison for rmw_gid_t to enable use in unordered_map | ||
| struct rmw_gid_equal | ||
| { | ||
| bool operator()(const rmw_gid_t & lhs, const rmw_gid_t & rhs) const noexcept | ||
| { | ||
| // Compare the data bytes only. | ||
| // implementation_identifier pointer comparison is not used here because | ||
| // intra-process communication is always within the same process and RMW, | ||
| // and pointer comparison is fragile across dynamically loaded components. | ||
| return std::equal( | ||
| std::begin(lhs.data), | ||
| std::end(lhs.data), | ||
| std::begin(rhs.data)); | ||
| } | ||
| }; | ||
|
Comment on lines
+406
to
+420
|
||
|
|
||
| using SubscriptionMap = | ||
| std::unordered_map<uint64_t, rclcpp::experimental::SubscriptionIntraProcessBase::WeakPtr>; | ||
|
|
||
|
|
@@ -398,6 +431,16 @@ class IntraProcessManager | |
| using PublisherToSubscriptionIdsMap = | ||
| std::unordered_map<uint64_t, SplittedSubscriptions>; | ||
|
|
||
| /// Structure to store publisher information in GID lookup map | ||
| struct PublisherInfo | ||
| { | ||
| uint64_t pub_id; | ||
| rclcpp::PublisherBase::WeakPtr publisher; | ||
| }; | ||
|
|
||
| using GidToPublisherInfoMap = | ||
| std::unordered_map<rmw_gid_t, PublisherInfo, rmw_gid_hash, rmw_gid_equal>; | ||
|
|
||
| RCLCPP_PUBLIC | ||
| static | ||
| uint64_t | ||
|
|
@@ -642,6 +685,8 @@ class IntraProcessManager | |
| PublisherBufferMap publisher_buffers_; | ||
|
|
||
| mutable std::shared_timed_mutex mutex_; | ||
|
|
||
| GidToPublisherInfoMap gid_to_publisher_info_; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the critical ABI fix. The original placement shifted mutex_'s offset within the class, causing template methods (inlined in downstream packages like Nav2) to access the wrong memory address for mutex_. The corrupted mutex writes overwrote the hashmap's internal state, producing the SIGSEGV in _M_find_before_node. |
||
| }; | ||
|
|
||
| } // namespace experimental | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -51,6 +51,9 @@ IntraProcessManager::add_publisher( | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Add GID to publisher info mapping for fast lookups (stores both ID and weak_ptr) | ||||||||||||||||||||||||
| gid_to_publisher_info_[publisher->get_gid()] = {pub_id, publisher}; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Initialize the subscriptions storage for this publisher. | ||||||||||||||||||||||||
| pub_to_subs_[pub_id] = SplittedSubscriptions(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -98,6 +101,24 @@ IntraProcessManager::remove_publisher(uint64_t intra_process_publisher_id) | |||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| std::unique_lock<std::shared_timed_mutex> lock(mutex_); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Remove GID to publisher info mapping. | ||||||||||||||||||||||||
| // First try via the publisher's own GID (fast path). | ||||||||||||||||||||||||
| auto pub_it = publishers_.find(intra_process_publisher_id); | ||||||||||||||||||||||||
| if (pub_it != publishers_.end()) { | ||||||||||||||||||||||||
| auto publisher = pub_it->second.lock(); | ||||||||||||||||||||||||
| if (publisher) { | ||||||||||||||||||||||||
| gid_to_publisher_info_.erase(publisher->get_gid()); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| // Publisher weak_ptr already expired, fall back to linear scan by pub_id. | ||||||||||||||||||||||||
| for (auto git = gid_to_publisher_info_.begin(); git != gid_to_publisher_info_.end(); ++git) { | ||||||||||||||||||||||||
| if (git->second.pub_id == intra_process_publisher_id) { | ||||||||||||||||||||||||
| gid_to_publisher_info_.erase(git); | ||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||
|
Comment on lines
+104
to
+116
|
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+112
to
+118
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added fall back clean-up process just in case. |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| } | |
| } | |
| } else { | |
| // Publisher id not found in publishers_, but there might still be a stale | |
| // entry in gid_to_publisher_info_. Fall back to linear scan by pub_id. | |
| for (auto git = gid_to_publisher_info_.begin(); git != gid_to_publisher_info_.end(); ++git) { | |
| if (git->second.pub_id == intra_process_publisher_id) { | |
| gid_to_publisher_info_.erase(git); | |
| break; | |
| } | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -162,7 +162,14 @@ class PublisherBase | |||||||||||||||||||||||||||
| explicit PublisherBase(const std::string & topic, const rclcpp::QoS & qos) | ||||||||||||||||||||||||||||
| : topic_name(topic), | ||||||||||||||||||||||||||||
| qos_profile(qos) | ||||||||||||||||||||||||||||
| {} | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| // Initialize a mock GID with unique data based on this pointer | ||||||||||||||||||||||||||||
| gid_.implementation_identifier = "mock_rmw"; | ||||||||||||||||||||||||||||
| auto ptr_value = reinterpret_cast<std::uintptr_t>(this); | ||||||||||||||||||||||||||||
| for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) { | ||||||||||||||||||||||||||||
|
Comment on lines
+168
to
+169
|
||||||||||||||||||||||||||||
| auto ptr_value = reinterpret_cast<std::uintptr_t>(this); | |
| for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) { | |
| auto ptr_value = reinterpret_cast<std::uintptr_t>(this); | |
| // Zero-fill the GID data to ensure deterministic contents | |
| for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) { | |
| gid_.data[i] = 0u; | |
| } | |
| // Copy at most sizeof(std::uintptr_t) bytes from the pointer value to avoid | |
| // shifting by an amount greater than or equal to the bit width of uintptr_t. | |
| const size_t ptr_num_bytes = sizeof(std::uintptr_t); | |
| const size_t bytes_to_copy = | |
| (RMW_GID_STORAGE_SIZE < ptr_num_bytes) ? RMW_GID_STORAGE_SIZE : ptr_num_bytes; | |
| for (size_t i = 0; i < bytes_to_copy; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is new compared to #3068. i think we do not need to do the comparison against RMW identifiers.