-
Notifications
You must be signed in to change notification settings - Fork 522
Avoid redundant allocation in wait_for_work #129
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
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 |
|---|---|---|
|
|
@@ -46,15 +46,39 @@ class MemoryStrategy | |
|
|
||
| /// Borrow memory for storing data for subscriptions, services, clients, or guard conditions. | ||
| /** | ||
| * The default implementation ignores the handle type and dynamically allocates the memory. | ||
| * The default implementation stores std::vectors for each handle type and resizes the vectors | ||
| * as necessary based on the requested number of handles. | ||
| * \param[in] The type of entity that this function is requesting for. | ||
| * \param[in] The number of handles to borrow. | ||
| * \return Pointer to the allocated handles. | ||
| */ | ||
| virtual void ** borrow_handles(HandleType type, size_t number_of_handles) | ||
| { | ||
| (void)type; | ||
| return static_cast<void **>(alloc(sizeof(void *) * number_of_handles)); | ||
| switch (type) { | ||
| case HandleType::subscription_handle: | ||
| if (subscription_handles.size() < number_of_handles) { | ||
| subscription_handles.resize(number_of_handles, 0); | ||
| } | ||
| return static_cast<void **>(subscription_handles.data()); | ||
| case HandleType::service_handle: | ||
| if (service_handles.size() < number_of_handles) { | ||
| service_handles.resize(number_of_handles, 0); | ||
| } | ||
| return static_cast<void **>(service_handles.data()); | ||
| case HandleType::client_handle: | ||
| if (client_handles.size() < number_of_handles) { | ||
| client_handles.resize(number_of_handles, 0); | ||
| } | ||
| return static_cast<void **>(client_handles.data()); | ||
| case HandleType::guard_condition_handle: | ||
| if (number_of_handles > 2) { | ||
| throw std::runtime_error("Too many guard condition handles requested!"); | ||
| } | ||
| return guard_cond_handles.data(); | ||
| default: | ||
| throw std::runtime_error("Unknown HandleType " + std::to_string(static_cast<int>(type)) + | ||
| ", could not borrow handle memory."); | ||
| } | ||
| } | ||
|
|
||
| /// Return the memory borrowed in borrow_handles. | ||
|
|
@@ -65,8 +89,39 @@ class MemoryStrategy | |
| */ | ||
| virtual void return_handles(HandleType type, void ** handles) | ||
| { | ||
| (void)type; | ||
| this->free(handles); | ||
| switch (type) { | ||
| case HandleType::subscription_handle: | ||
| if (handles != subscription_handles.data()) { | ||
| throw std::runtime_error( | ||
| "tried to return memory that isn't handled by this MemoryStrategy"); | ||
| } | ||
| memset(handles, 0, subscription_handles.size()); | ||
| break; | ||
| case HandleType::service_handle: | ||
| if (handles != service_handles.data()) { | ||
| throw std::runtime_error( | ||
| "tried to return memory that isn't handled by this MemoryStrategy"); | ||
| } | ||
| memset(handles, 0, service_handles.size()); | ||
| break; | ||
| case HandleType::client_handle: | ||
| if (handles != client_handles.data()) { | ||
| throw std::runtime_error( | ||
| "tried to return memory that isn't handled by this MemoryStrategy"); | ||
| } | ||
| memset(handles, 0, client_handles.size()); | ||
| break; | ||
| case HandleType::guard_condition_handle: | ||
| if (handles != guard_cond_handles.data()) { | ||
| throw std::runtime_error( | ||
| "tried to return memory that isn't handled by this MemoryStrategy"); | ||
| } | ||
| guard_cond_handles.fill(0); | ||
| break; | ||
| default: | ||
| throw std::runtime_error("Unknown HandleType " + std::to_string(static_cast<int>(type)) + | ||
| ", could not borrow handle memory."); | ||
| } | ||
|
Member
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. Is it worth checking that the value in handles matches the address of the pointer returned by |
||
| } | ||
|
|
||
| /// Provide a newly initialized AnyExecutable object. | ||
|
|
@@ -101,6 +156,11 @@ class MemoryStrategy | |
| std::vector<rclcpp::subscription::SubscriptionBase::SharedPtr> subs; | ||
| std::vector<rclcpp::service::ServiceBase::SharedPtr> services; | ||
| std::vector<rclcpp::client::ClientBase::SharedPtr> clients; | ||
|
|
||
| std::vector<void *> subscription_handles; | ||
|
Contributor
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. Is there a max number of handles supported which could be reserved in the constructor? Using resize may or may not allocate unless it's been previously allocated.
Member
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. For this memory strategy it doesn't matter if it allocates on
Contributor
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. I think it's fine for the vectors to be initialized to empty. Specifying a number of handles reserve beforehand seems unnecessarily complicated for the default case. The alternative MemoryStrategy implementations take max numbers of handles as constructor arguments, however. When http://en.cppreference.com/w/cpp/container/vector/resize My interpretation is that calling resize for a size greater than the vector's current size will insert elements into the vector to make it grow to the required size.
Contributor
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. Yes resize is safe it will allocate if not enough is reserved. The title was about reducing allocations I didn't know if we wanted to do the preallocation. If this is just the default it will work fine.
Contributor
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. @dirk-thomas clarified that if the vector's capacity is smaller than the requested size, But for our purposes this is fine since these vectors will usually store small numbers (<16). |
||
| std::vector<void *> service_handles; | ||
| std::vector<void *> client_handles; | ||
| std::array<void *, 2> guard_cond_handles; | ||
| }; | ||
|
|
||
|
|
||
|
|
||
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 doc block above looks out of date with this change.
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've updated the doc block now.