Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[wip] Add cache for topic filter to avoid performance burden on discovery #1486

Draft
wants to merge 1 commit into
base: rolling
Choose a base branch
from
+23 −12
Draft

[wip] Add cache for topic filter to avoid performance burden on discovery #1486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rosbag2_transport/include/rosbag2_transport/topic_filter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class ROSBAG2_TRANSPORT_PUBLIC TopicFilter
bool allow_unknown_types_ = false;
std::unordered_set<std::string> already_warned_unknown_types_;
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph_;
std::unordered_map<std::string, bool> take_topics_cache_;
};
} // namespace rosbag2_transport

Expand Down
34 changes: 22 additions & 12 deletions rosbag2_transport/src/rosbag2_transport/topic_filter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,15 @@ topic_is_unpublished(
const std::string & topic_name, rclcpp::node_interfaces::NodeGraphInterface & node_graph)
{
auto publishers_info = node_graph.get_publishers_info_by_topic(topic_name);
return publishers_info.size() == 0;
return publishers_info.empty();
}

bool
is_leaf_topic(
const std::string & topic_name, rclcpp::node_interfaces::NodeGraphInterface & node_graph)
{
auto subscriptions_info = node_graph.get_subscriptions_info_by_topic(topic_name);
return subscriptions_info.size() == 0;
return subscriptions_info.empty();
}
} // namespace

Expand All @@ -94,21 +94,31 @@ TopicFilter::TopicFilter(
RecordOptions record_options,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph,
bool allow_unknown_types)
: record_options_(record_options),
: record_options_(std::move(record_options)),
allow_unknown_types_(allow_unknown_types),
node_graph_(node_graph)
{}

TopicFilter::~TopicFilter()
{}
TopicFilter::~TopicFilter() = default;

std::unordered_map<std::string, std::string> TopicFilter::filter_topics(
const std::map<std::string, std::vector<std::string>> & topic_names_and_types)
{
std::unordered_map<std::string, std::string> filtered_topics;
for (const auto & [topic_name, topic_types] : topic_names_and_types) {
if (take_topic(topic_name, topic_types)) {
filtered_topics.insert(std::make_pair(topic_name, topic_types[0]));
// Check take_topics_cache_ first to avoid performance burden when discovery thread running
auto take_topics_cache_it = take_topics_cache_.find(topic_name);
if (take_topics_cache_it != take_topics_cache_.end()) {
if (take_topics_cache_it->second) {
filtered_topics.insert(std::make_pair(topic_name, topic_types[0]));
}
} else {
if (take_topic(topic_name, topic_types)) {
filtered_topics.insert(std::make_pair(topic_name, topic_types[0]));
take_topics_cache_[topic_name] = true;
} else {
take_topics_cache_[topic_name] = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st discovery, lets say if the topic is bound with unknown type, and the register here with false. after 2nd discovery, even if the topic is rebound to the appropriate type, it cannot be taken since it does not reconsider the topic type every time. how about using already_warned_unknown_types_ to check if we already know that is unknown type or not to deny the topic instead?

}
}
}
return filtered_topics;
Expand All @@ -117,6 +127,11 @@ std::unordered_map<std::string, std::string> TopicFilter::filter_topics(
bool TopicFilter::take_topic(
const std::string & topic_name, const std::vector<std::string> & topic_types)
{
if (!has_single_type(topic_name, topic_types)) {
return false;
}
const std::string & topic_type = topic_types[0];

if (!record_options_.include_unpublished_topics && node_graph_ &&
topic_is_unpublished(topic_name, *node_graph_))
{
Expand Down Expand Up @@ -147,18 +162,13 @@ bool TopicFilter::take_topic(
return false;
}

if (!has_single_type(topic_name, topic_types)) {
return false;
}

if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) {
RCUTILS_LOG_WARN_ONCE_NAMED(
ROSBAG2_TRANSPORT_PACKAGE_NAME,
"Hidden topics are not recorded. Enable them with --include-hidden-topics");
return false;
}

const std::string & topic_type = topic_types[0];
if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) {
return false;
}
Expand Down
Loading