-
Notifications
You must be signed in to change notification settings - Fork 2
Add EventsQueue class with 3 push strategies #39
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
Add EventsQueue class with 3 push strategies #39
Conversation
|
Some early comments:
|
| : memory_strategy(rclcpp::memory_strategies::create_default_strategy()), | ||
| context(rclcpp::contexts::get_global_default_context()), | ||
| max_conditions(0) | ||
| max_conditions(0), queue_options(QueueOptions()) |
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.
The ExecutorOption structure should only contain stuff that is used by all executors
| { | ||
| QueueOptions() | ||
| : queue_strategy(QueueStrategy::CPU_PERFORMANCE), | ||
| max_events(1000) |
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.
Following my comments above, max_events should be an internal detail of a bounded queue class
| RCLCPP_DISABLE_COPY(EventsExecutor) | ||
|
|
||
| using EventQueue = std::queue<rmw_listener_event_t>; | ||
| using EventQueue = std::deque<rmw_listener_event_t>; |
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 looks not needed anymore now (and also you can remove the queue/deque include)
| // Notify that the event queue has some events in it. | ||
| this_executor->event_queue_cv_.notify_one(); | ||
| // Push event and notify the queue it has some events | ||
| this_executor->events_queue_->push_and_notify(event); |
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 should be just a simple push, then the notify should still be done here
| */ | ||
| RCLCPP_PUBLIC | ||
| rmw_qos_profile_t | ||
| get_actual_qos() const |
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.
I'm not sure I understand what this is.
Why a waitable has a QOS?
| waitable_events_in_queue_map_.clear(); | ||
| } | ||
|
|
||
| void EventsQueue::swap(EventQueue & event_queue) |
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.
We don't need to expose that we are doing a swap. That's just an optimization.
This API should have a signature like:
std::queue<rmw_listener_event_t> EventsQueue::get_all_events()
| clear_stats_implem_(); | ||
| } | ||
|
|
||
| void EventsQueue::wait_for_event(std::chrono::nanoseconds timeout) |
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.
wait_for_event and wait_for_event_and_swap seem not needed to me.
The executor should manage the wait, then call swap (or its renamed version as I suggest above).
It's also ok to have this class without any mutex and let the executor manage that
| clear_stats_implem_(); | ||
| } | ||
|
|
||
| bool EventsQueue::wait_and_get_first_event( |
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.
Same as above.
If the executor manages the wait and the mutex, the queue only need to expose empty() and front() APIs
|
Created a new PR based on this comments: #40 |
Usage example:
I ended up creating 3 policies:
CPU_PERFORMANCEthe default policy, it should peform as good as without this PR (to confirm soon)LIMITED_EVENTS_WITH_TIME_ORDERINGchecks before pushing is there are more events than QoS->depth and reordersBOUNDEDthe queue is hard bounded to a limit. In case of more events, a prune of the queue happens based on QoS.TODO:
clients,servicesandevents