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.

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

move vectors instantiated in wait_for_work to MemoryStrategy #116

Merged
merged 1 commit into from
Oct 7, 2015

Conversation

jacquelinekay
Copy link
Contributor

This was done in my attempt to reduce pagefaults in Executor::spin. Several vectors that were being instantiated on the stack in Executor::spin have been moved into the MemoryStrategy, so that they can be preallocated (with vector::reserve) on construction.

@jacquelinekay jacquelinekay added the in progress Actively being worked on (Kanban column) label Sep 30, 2015
@wjwwood
Copy link
Member

wjwwood commented Sep 30, 2015

Seems reasonable to me, but I don't have time to try it right now. I'd recommend CI before merging.

@dirk-thomas
Copy link
Member

Is this necessary for the demo at ROSCon? If not I would suggest postponing it until Monday.

@jacquelinekay jacquelinekay self-assigned this Sep 30, 2015
@jacquelinekay jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 6, 2015
@jacquelinekay
Copy link
Contributor Author

CI:
http://ci.ros2.org/job/ros2_batch_ci_linux/403/
http://ci.ros2.org/job/ros2_batch_ci_osx/300/
http://ci.ros2.org/job/ros2_batch_ci_windows/448/

Windows is already done and looks good besides the connext_dynamic tests, so that's a good sign.

@esteve
Copy link
Member

esteve commented Oct 6, 2015

The test_requester_replier_cpp__empty__rmw_connext_dynamic_cpp.test_requester_replier test fails, but I don't think it's caused by this. That test seems flaky and fails with other PRs randomly.

@@ -361,14 +361,14 @@ class Executor
for (auto & weak_subscription : group->subscription_ptrs_) {
auto subscription = weak_subscription.lock();
if (subscription) {
subs.push_back(subscription);
memory_strategy_->subs.push_back(subscription);
Copy link
Member

Choose a reason for hiding this comment

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

I think the MemoryStrategy API would look clear if, instead of exposing the std::vector directly, provide add_subscription, add_service, etc. and hide the underlying storage.

jacquelinekay added a commit that referenced this pull request Oct 7, 2015
move vectors instantiated in wait_for_work to MemoryStrategy
@jacquelinekay jacquelinekay merged commit b84caa8 into master Oct 7, 2015
@jacquelinekay jacquelinekay removed the in review Waiting for review (Kanban column) label Oct 7, 2015
@jacquelinekay jacquelinekay deleted the fix_uninitialized_vectors branch October 7, 2015 21:38
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
* fix cmake for test - remove unused params

* set env variable for rmw_implementation
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
…ackage (ros2#48)

* ros2GH-111 Add package for converter plugins

* ros2GH-111 Add CDR converter plugin

* ros2GH-111 Add test for more primitives types

* ros2GH-116 Fix cdr converter after rebase on new converters interface

* ros2GH-116 Use rmw_serialize/rmw_deserialize directly in converter and link against rmw_fastrtps_cpp

* Fix converter package.xml

* Fix clang warnings

* ros2GH-30 Change interface to the same convention as rmw_(de)serialize

* comply to new rcutils error handling API

* use poco to load fastrtps

* Update rosbag2_converter_default_plugins/src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp

Co-Authored-By: Karsten1987 <[email protected]>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
* ignore cast function type warning

Signed-off-by: Karsten Knese <[email protected]>

* use pragma to ignore warning on specific lines of code

Signed-off-by: Karsten Knese <[email protected]>

* correctly branch the pragma

Signed-off-by: Karsten Knese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants