Allocator template for publish/subscribe pipeline #137

jacquelinekay · 2015-10-22T23:04:27Z

This pull request exposes template arguments to Publisher, Subscription, AnySubscriptionCallback, and MappedRingBuffer so that a custom allocator can be passed to these entities.

Memory leaks in classes with Allocators. Should change raw pointer to shared pointer to handle ownership.
Style cleanup to make templated names less scary-looking. (I would appreciate more feedback on this; I'm considering writing macros since I repeat similar type aliases in several files.)
Pass allocator to shared_ptrs instantiated during runtime so that their copies and control blocks use the custom allocator.
Figure out how to pass allocator to shared pointers and other stdlib structures in: CallbackGroup, Executor, IntraProcessManager (add allocator to Context?)

After this PR is merged, both the intra-process and inter-process pipelines should be set up for real-time safe allocation.

jacquelinekay · 2015-10-23T20:52:34Z

Contrary to my expectations, the branch in its current state built on the Windows Jenkins job!

http://ci.ros2.org/job/ros2_batch_ci_windows/518/

wjwwood · 2015-10-23T21:14:12Z

Well that's exciting.

jacquelinekay · 2015-10-24T00:55:33Z

I tried to implement a change to Executor where I got rid of MemoryStrategy and replaced with ExecutorRuntimeState, a templated class that keeps track of all the stdlib structures that dynamically change size during the execution path, and inherits from a pure virtual non-templated base class. I ripped out a lot of the functionality of Executor (basically anything that interfaces with the lists and vectors in Executor) and put it in ExecutorRuntimeState. But then I realized it wouldn't compile because a lot of the other entities in rclcpp declare themselves as friend of Executor, and they can't declare themselves as friend of ExecutorRuntimeState without knowing about T (I think).

Back to the drawing board... I'm going to look at how to template the IntraProcessManager next.

wjwwood · 2015-10-24T00:57:01Z

We may be able to get rid of the friend relationships. Don't be afraid to suggest changes in the relationships between objects.

jacquelinekay · 2015-10-27T22:48:55Z

I now consider this ready for review.

CI jobs:

http://ci.ros2.org/job/ros2_batch_ci_linux/488/
http://ci.ros2.org/job/ros2_batch_ci_osx/387/
http://ci.ros2.org/job/ros2_batch_ci_windows/538/

dirk-thomas · 2015-10-27T23:15:04Z

New code should pass cpplint.

Btw. the jobs are failing to compile.

dirk-thomas · 2015-10-27T23:16:33Z

rclcpp/include/rclcpp/allocator/allocator_common.hpp

+namespace allocator
+{
+
+template<typename U, typename Alloc>


What does the U stand for? If nothing specific I would recommend to use the common variable T for template typenames.

Same below.

jacquelinekay · 2015-10-28T23:54:45Z

Another set of CI jobs.

I'm going to wait on a clean set of CI before squashing, just in case:

http://ci.ros2.org/job/ros2_batch_ci_linux/511/
http://ci.ros2.org/job/ros2_batch_ci_osx/392/
http://ci.ros2.org/job/ros2_batch_ci_windows/543/

dirk-thomas · 2015-10-28T23:55:58Z

Why do you think the Linux CI job is broken?

jacquelinekay · 2015-10-28T23:56:38Z

there are two jobs pending that haven't started yet, and they've both been pending for a few minutes.

dirk-thomas · 2015-10-28T23:57:12Z

This job is currently running on the Linux slave: http://ci.ros2.org/view/packaging/job/ros2_packaging_linux/

jacquelinekay · 2015-10-29T01:11:01Z

and again:

http://ci.ros2.org/job/ros2_batch_ci_linux/513
http://ci.ros2.org/job/ros2_batch_ci_osx/393/
http://ci.ros2.org/job/ros2_batch_ci_windows/544/

jacquelinekay · 2015-10-29T15:39:33Z

There are a lot of regressions. But many of them exist in the nightly build:

http://ci.ros2.org/view/nightly/job/ros2_batch_ci_osx_nightly/94/testReport/
vs.
http://ci.ros2.org/job/ros2_batch_ci_osx/393/testReport/

On Linux, the allocator test doesn't work, probably because the backtrace trick only works when the code is compiled in Debug mode. I will change the test so that it fails if the number of allocations during execution per call to "spin" exceeds a set number, which is not as great.

I'm not sure where many of the regressions in Windows are coming from: the nightly build has 86 failures and the CI for this branch has 214 failures. I am rerunning Windows CI on default just to compare.

dirk-thomas · 2015-10-29T17:09:26Z

For some of the failing tests (nose.failure.Failure.runTest) there are PRs which are only waiting for reviews...

esteve · 2015-10-30T00:13:07Z

+1 I just ran this on Windows and the tests pass

Allocator template for publish/subscribe pipeline

* fix documentation of expand_topic_name() * use expand_topic_name() in pub/sub/client/service * update test expectations * use absolute topic names to avoid mismatch due to expansion * propagate new functions from rcutils error handling * convert given rcl allocator to a rcutils allocator

* ros2GH-118 Make rosbag2::Writer use converters - Use converters in Writer::write() when input rmw serialization format is different from desired storage serialization format - Add new field in rosbag2::StorageOptions to keep track of the rmw format given by the user to store the message in * ros2GH-118 Add --encoding option to ros2 bag record * ros2GH-118 Associate to each topic its rmw_serialization_format - Add 'serialization_format' field to TopicMetadata - Add 'serialization_forat' column in 'topics' table in sqlite storage - Remove 'storage_format' from BagMetadata and use the TopicMetadata field directly, instead - the field 'rmw_serialization_format' has been moved from rosbag2::StorageOptions to rosbag2_transport::RecordOptions, because it's a topic property rather than a storage one. - Currently all topics in a bag file must have the same serialization format - The tests have been updated accordingly * ros2GH-118 Fix tests after rebase * ros2GH-118 Fix MockMetadataIO and use it in test_writer * ros2GH-118 Fix Windows build and minor refactoring * ros2GH-118 Add test for writer to check that error is thrown if converter plugin does not exist * ros2GH-118 Add test to check that metadat_io_ writes metadata file in writer's destructor * ros2GH-118 Build Converter before opening the database in Writer::open() - This assures that if one of the converter plugins does not exist, the database is not created * ros2GH-118 Add end-to-end tests to check graceful failure if converter plugins do not exists - Both a test for record and play has been added * ros2GH-118 Rename 'encoding' CLI option to 'serialization_format' * ros2GH-127 Write serialization format in database also when it's not specified at CLI level - Tests to check that the serialization format is written in the database have also been added. * ros2GH-17 Add leak sanitizer to test - one of the main test goals can only be ssen by valgrind or sanitizers - enable leak sanitizer for gcc builds only (for now) * ros2GH-137: Fix cdr converter plugin - update pluginlib descriptions file after several renames - fix export of missing includes folder * ros2GH-137 Add integration test for cdr converter * ros2GH-137 Fix superfluous printf * ros2GH-137 It suffices to have only one converter test * ros2GH-137 Minor refactoring for better readability of test N.B. This exposes an pre-existing memory leak (not fixed here). * ros2GH-137 Fix memory leak of topic_name - topic_name member needs to be freed - provide a setter for convenience - Directly assigning a string literal in the test is not sufficient as this would be static memory that does not need to be freed. * ros2GH-17 Allow disabling the usage of sanitizers This allows manual usage of valgrind. * ros2GH-17 Fix renaming after rebase * ros2GH-17 Small cleanups (addressing review comments)

…2#137) Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

jacquelinekay added the in progress Actively being worked on (Kanban column) label Oct 22, 2015

jacquelinekay mentioned this pull request Oct 22, 2015

Allocator template ros2/system_tests#60

Merged

jacquelinekay self-assigned this Oct 22, 2015

jacquelinekay force-pushed the allocator_template branch 3 times, most recently from 25fe598 to b4c7cb1 Compare October 27, 2015 01:54

jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 27, 2015

This was referenced Oct 27, 2015

remove pool memory strategy examples ros2/examples#67

Merged

remove HeapPoolMemoryStrategy ros2/demos#33

Merged

dirk-thomas reviewed Oct 27, 2015
View reviewed changes

Jackie Kay added 10 commits October 28, 2015 18:03

publisher template on allocator

2e68bd5

templated stl structures exposed in Node

444e4fd

template subscriber, using rebind semantics

788be00

Add allocator memory strategy

0cd1360

rebind allocator in mapped_ring_buffer

ea21d92

Use allocate_shared

4138e6b

templatize IPM state

c663d89

move a lot to memory strategy

06818ee

static factory method

0e78ea0

remove pool memory strategies

fc48cf5

jacquelinekay force-pushed the allocator_template branch from 07ca744 to b6d2033 Compare October 29, 2015 01:05

wjwwood mentioned this pull request Oct 29, 2015

Make rclcpp in to a library #140

Merged

Jackie Kay and others added 2 commits October 29, 2015 17:14

Cpplint and cleanup

24fb204

Fix windows vector issue

142c9ab

jacquelinekay force-pushed the allocator_template branch from 1df0f24 to 142c9ab Compare October 30, 2015 00:14

jacquelinekay added a commit that referenced this pull request Oct 30, 2015

Merge pull request #137 from ros2/allocator_template

010fa3d

Allocator template for publish/subscribe pipeline

jacquelinekay merged commit 010fa3d into master Oct 30, 2015

jacquelinekay removed the in review Waiting for review (Kanban column) label Oct 30, 2015

jacquelinekay deleted the allocator_template branch October 30, 2015 00:15

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

disable plugins/tests which need rmw_fastrtps_cpp if unavailable (ros…

fd65963

…2#137) Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allocator template for publish/subscribe pipeline #137

Allocator template for publish/subscribe pipeline #137

jacquelinekay commented Oct 22, 2015

jacquelinekay commented Oct 23, 2015

wjwwood commented Oct 23, 2015

jacquelinekay commented Oct 24, 2015

wjwwood commented Oct 24, 2015

jacquelinekay commented Oct 27, 2015

dirk-thomas commented Oct 27, 2015

dirk-thomas Oct 27, 2015

jacquelinekay commented Oct 28, 2015

dirk-thomas commented Oct 28, 2015

jacquelinekay commented Oct 28, 2015

dirk-thomas commented Oct 28, 2015

jacquelinekay commented Oct 29, 2015

jacquelinekay commented Oct 29, 2015

dirk-thomas commented Oct 29, 2015

esteve commented Oct 30, 2015

Allocator template for publish/subscribe pipeline #137

Allocator template for publish/subscribe pipeline #137

Conversation

jacquelinekay commented Oct 22, 2015

jacquelinekay commented Oct 23, 2015

wjwwood commented Oct 23, 2015

jacquelinekay commented Oct 24, 2015

wjwwood commented Oct 24, 2015

jacquelinekay commented Oct 27, 2015

dirk-thomas commented Oct 27, 2015

dirk-thomas Oct 27, 2015

Choose a reason for hiding this comment

jacquelinekay commented Oct 28, 2015

dirk-thomas commented Oct 28, 2015

jacquelinekay commented Oct 28, 2015

dirk-thomas commented Oct 28, 2015

jacquelinekay commented Oct 29, 2015

jacquelinekay commented Oct 29, 2015

dirk-thomas commented Oct 29, 2015

esteve commented Oct 30, 2015