Add "MemoryStrategy" to Executor with dep. injection #56

jacquelinekay · 2015-07-13T21:58:19Z

This pull request introduces the MemoryStrategy class to delegate memory management on the execution path of Executor.

By default, MemoryStrategy is stateless and handles all delegated functions with dynamic memory management (malloc, free, new), but all functions are overrideable. An example subclass of MemoryStrategy, StaticMemoryStrategy, shows how these functions can be overridden and state can be added to implement fully static memory management.

The following functions are delegated to MemoryStrategy:

borrow_handles: returns a pointer to memory reserved for subscription, service, client or guard condition handles.
return_handles: cleans up memory instantiated in borrow_handles, by deleting it or resetting to NULL.
instantiate_next_executable: instantiates the next AnyExecutable object and passes it as a shared pointer.

In order to manage circular dependencies, I moved the AnyExecutable struct from a protected member of Executor to its own header file.

An example of how the memory strategy can be specified dynamically is in the examples/memory_dependency_injection:
https://github.com/ros2/examples/blob/memory_dependency_injection/rclcpp_examples/src/topics/listener_exec.cpp

…emove dynamic_memory_strategy

wjwwood · 2015-07-13T22:01:56Z

rclcpp/include/rclcpp/any_executable.hpp

+{
+namespace executor
+{
+struct AnyExecutable


You could use the RCLCPP_MAKE_SHARED_DEFINITIONS macro here to add the static function make_shared to the AnyExecutable class.

wjwwood · 2015-07-13T22:34:11Z

+1

jacquelinekay · 2015-07-13T22:43:25Z

Updates based on William's comments in 541385a:

use RCLCPP_MAKE_SHARED for shared pointer typedefs for new classes
namespace/struct newline placement
change map to unordered_map and reserve memory in constructor of StaticMemoryAllocator

dirk-thomas · 2015-07-13T22:48:45Z

rclcpp/include/rclcpp/executor.hpp

@@ -91,7 +98,7 @@ class Executor
  {
    this->add_node(node);
    // non-blocking = true
-    std::shared_ptr<AnyExecutable> any_exec = get_next_executable(nonblocking);
+    AnyExecutable::SharedPtr any_exec = get_next_executable(nonblocking);


This could use auto.

wjwwood · 2015-07-13T22:53:53Z

Thanks @jacquelinekay, 541385a lgtm.

dirk-thomas · 2015-07-13T22:54:52Z

rclcpp/include/rclcpp/strategies/static_memory_strategy.hpp

+  size_t _pool_seq;
+  size_t _exec_seq;
+
+  std::unordered_map<void *, size_t> _memory_map;


All those member variables should have a trailing underscore and no leading underscore.

dirk-thomas · 2015-07-13T22:56:48Z

rclcpp/include/rclcpp/strategies/static_memory_strategy.hpp

+
+  void ** borrow_handles(HandleType type, size_t number_of_handles)
+  {
+    switch (type) {


Should the code check that number_of_handles is within the preallocated limits?

yes, definitely. Done in e751530.

dirk-thomas · 2015-07-13T23:19:17Z

LGTM.

jacquelinekay · 2015-07-14T20:32:49Z

@tfoote Let me know if you're planning to take a look at this PR, otherwise I will merge it soon (next few days).

tfoote · 2015-07-15T06:43:07Z

+1

esteve · 2015-07-15T15:30:29Z

rclcpp/include/rclcpp/memory_strategy.hpp

+    return std::malloc(size);
+  }
+
+  virtual void free(void * ptr)


I'd suggest to name this dealloc, just to mirror alloc.

esteve · 2015-07-15T15:38:03Z

+1

Add "MemoryStrategy" to Executor with dep. injection

…events Remove use_previous_event

use CTest BUILD_TESTING

* ros2GH-112 Open storage for reading handing in rmw_identifier * ros2GH-113 Cleanup: better naming * ros2GH-113 Introduce interface for StorageFactory to allow mocks in tests * ros2GH-113 Add test for SequentialReader for using converters - Added mocks for storage and converters (and factories) * ros2GH-113 Implement skeleton convert function - Use convert only if necessary (different input and output formats), converters are only loaded if really necessary. - Allocate_ros2_message is public to enable extensive tests for this function. - Helper function to get any typesupport by name - Helper function for empty ros2_message * ros2GH-113 Implement allocate_ros2_message - Treats most messages already. - Some combinations of nested messages with arrays are still missing - Cleanup of DynamicArrayNested messages is failing - Main difficulty is the cleanup of the allocated ros2_message which needs to be done manually - The test_ros2_message is intended to be run with valgrind and there should be no leaks or problems with free! * ros2GH-113 Fix DynamicArrayNested deallocation Swapping with empty container seems more stable than deleting the data pointer of the container. * ros2GH-113 Add test for BoundedArrayNested deallocation * ros2GH-113 Refactoring of deallocation code * ros2GH-113 Fix string initialization in all types * ros2GH-113 Fix vector<bool> initialization * ros2GH-113 Add test for deallocation of topic name + Refactoring * ros2GH-113 Minor refactoring of converter * ros2GH-113 Make sure to throw an error if converters do not exist * ros2GH-113 Delete superfluous imports * ros2GH-113 Small fix for deleting vectors * ros2GH-113 Fix build after rebase * ros2GH-30 Minor refactoring - The TODO comments have been removed because they're no longer relevant: they have been discussed in the PR review * ros2GH-30 Give an allocator as parameter to allocate_ros2_message() * ros2GH-111 Add missing test dependencies for CDR converter test * ros2GH-128 Extend message allocation test to also cover big strings - Big strings are not treated with small string optimization and need to be checked, too. * ros2GH-128 Add tests for nested arrays * ros2GH-128 always initialize vectors with a placement new * pass by ref * use new getter functions * consistent function naming * uncrustify * ros2GH-30 Fix windows build * use visibility macros on all functions

Jackie Kay added 3 commits July 13, 2015 12:01

First pass at dependency injection of memory strategy objects

ba91c8d

get next executable instantiation, move any_executable to own file, r…

41cc532

…emove dynamic_memory_strategy

Uncrustified

0557b11

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

jacquelinekay mentioned this pull request Jul 13, 2015

Virtual functions to handle memory allocation in Executor::wait_for_work #55

Closed

jacquelinekay self-assigned this Jul 13, 2015

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

jacquelinekay mentioned this pull request Jul 13, 2015

Refactor for real-time code safety ros2/ros2#68

Closed

7 tasks

wjwwood reviewed Jul 13, 2015
View reviewed changes

Jackie Kay added 2 commits July 13, 2015 15:39

William's suggetions

541385a

namespace tags

d3f5614

dirk-thomas reviewed Jul 13, 2015
View reviewed changes

Dirk's feedback: use auto, use trailing underscore, reorder includes

e751530

esteve reviewed Jul 15, 2015
View reviewed changes

jacquelinekay added a commit that referenced this pull request Jul 15, 2015

Merge pull request #56 from ros2/memory_dependency_injection

5a426d3

Add "MemoryStrategy" to Executor with dep. injection

jacquelinekay merged commit 5a426d3 into master Jul 15, 2015

jacquelinekay removed the in review Waiting for review (Kanban column) label Jul 15, 2015

jacquelinekay deleted the memory_dependency_injection branch July 15, 2015 16:47

jacquelinekay mentioned this pull request Jul 22, 2015

avoid redundant memory allocations in wait_for_work #5

Closed

mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Apr 1, 2021

Merge pull request ros2#56 from mauropasse/mauro/remove-use-previous-…

5ecfad0

…events Remove use_previous_event

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Merge pull request ros2#56 from ros2/ctest_build_testing

2c33b23

use CTest BUILD_TESTING

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "MemoryStrategy" to Executor with dep. injection #56

Add "MemoryStrategy" to Executor with dep. injection #56

jacquelinekay commented Jul 13, 2015

wjwwood Jul 13, 2015

wjwwood commented Jul 13, 2015

jacquelinekay commented Jul 13, 2015

dirk-thomas Jul 13, 2015

wjwwood commented Jul 13, 2015

dirk-thomas Jul 13, 2015

dirk-thomas Jul 13, 2015

jacquelinekay Jul 13, 2015

dirk-thomas commented Jul 13, 2015

jacquelinekay commented Jul 14, 2015

tfoote commented Jul 15, 2015

esteve Jul 15, 2015

esteve commented Jul 15, 2015

Add "MemoryStrategy" to Executor with dep. injection #56

Add "MemoryStrategy" to Executor with dep. injection #56

Conversation

jacquelinekay commented Jul 13, 2015

wjwwood Jul 13, 2015

Choose a reason for hiding this comment

wjwwood commented Jul 13, 2015

jacquelinekay commented Jul 13, 2015

dirk-thomas Jul 13, 2015

Choose a reason for hiding this comment

wjwwood commented Jul 13, 2015

dirk-thomas Jul 13, 2015

Choose a reason for hiding this comment

dirk-thomas Jul 13, 2015

Choose a reason for hiding this comment

jacquelinekay Jul 13, 2015

Choose a reason for hiding this comment

dirk-thomas commented Jul 13, 2015

jacquelinekay commented Jul 14, 2015

tfoote commented Jul 15, 2015

esteve Jul 15, 2015

Choose a reason for hiding this comment

esteve commented Jul 15, 2015