iceoryx Application Memory Management #1169
Replies: 2 comments
-
@elBoberido I fully support the plans for 3.0. I think we have too much to do until 2.1 and this seems like something which comes with a lot surprises. I see the solution for 2.0 not so critical as you do. First of all I agree that the magic numbers in the types are not an ideal solution and that the memory calculation macro for the waitset/listener is far from a suitable. But we have reliable measures in place that make sure we do not shoot ourself in the foot with that.
Furthermore, the stack blow up problem would then only be solved for C and not C++ since in C++ we still store the listener/waitset on the stack. I fear that your 2.0 solution would replace technical debt with another form of technical debt which may be worse. We are aware of the stack problem and mitigated it for now. If the stack of a C users explodes they can always change this code: iox_listener_storage_t listenerStorage;
iox_listener_t listener = iox_listener_init(&listenerStorage); into iox_listener_storage_t *listenerStorage = (iox_listener_storage_t*)malloc(sizeof(iox_listener_storage_t));
iox_listener_t listener = iox_listener_init(listenerStorage); But what happens when by some reason someone is relying on the fact that those constructs only use stack memory or what if something else breaks in the code freeze week with this approach? And do we have the time to implement and test this new approach? With the To summarize: The current approach has flaws which we have to solve but it is completely tested and in my opinion we are unable to destroy something by accident when listener/waitset sizes are changing. We do not have to put further work in it. |
Beta Was this translation helpful? Give feedback.
-
@elBoberido I have only read it quickly and not fully thought about it. I fully I agree we should change something but I would like us to agree on the constraints, most importantly such as do we want to make it possible to construct these objects on stack. If this is not a requirement everything else is much easier (handles, configurable allocators, factory functions ...). Another important consideration is whether we want to keep the We can discuss potential solutions once this is more clear, but I agree with you preference with allocator based approaches. |
Beta Was this translation helpful? Give feedback.
-
I think all of us know that our application memory management is not ideal, some may even state it is stupid. Basically, we put everything on the stack and regularly blow up the stack. Currently there is also the problem with the C binding and the
do_not_touch_me
buffers with magic numbers.A fault confessed is half redressed. Here is my proposal for v2.0:
*_init
function allocate with the system allocator (malloc) and stores the pointer in the storage*_deinit
deallocate with the system allocator (free)For v2.1 or 3.0:
initRuntime
with an optionalMemoryConfig
struct to be able to select an allocatorMemoryConfig
specifies an allocatorListener
,WaitSet
,ServiceDiscovery
, etc.Listener
,WaitSets
, etc. in the memory blockallocate
method to the runtime (alternativelyloan
)cxx::unique_ptr<T> PoshRuntime::allocate<T>()
, e.g.auto listener = getRuntime().allocate<Listener>()
iox_runtime_allocate_listener()
for the C binding*_init
/*_deinit
functions and make them useiox_runtime_allocate_listener()
before they are removed at a later releasecxx::unique_ptr<Listener> Listener::allocate()
/cxx::unique_ptr<Listener> Listener::loan()
iox_listener_loan()
for the C bindingMemoryConfig
@budrus @elfenpiff @MatthiasKillat @FerdinandSpitzschnueffler @dkroenke @mossmaurice what do you think? Shall I create an issue for this?
Beta Was this translation helpful? Give feedback.
All reactions