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

Partial memory leak fix #325

Merged
merged 3 commits into from
Nov 15, 2024
Merged

Partial memory leak fix #325

merged 3 commits into from
Nov 15, 2024

Conversation

jimmy-mcelwain
Copy link
Collaborator

A partial fix for #35.

There was a member of the QueueTrajPoint response that was getting initialized twice.

g_messages_QueueTrajPoint.response = motoros2_interfaces__srv__QueueTrajPoint_Response__create();
rosidl_runtime_c__String__init(&g_messages_QueueTrajPoint.response->message);

The response creation call already initialized g_messages_QueueTrajPoint.response->message, so the init call that followed caused a memory leak.

This will not close out the issue, as there is still a trickier memory leak as mentioned in this comment. But from my testing, this does seem to recoup somewhere in the range of 24-48 bytes each connect/disconnect cycle.

Added a memory trace that tracks how the memory changes through a full connection/disconnect cycle so that memory leaks can be more easily detected.
A rosidl_runtime_c__String that was a member of a message was having an init() function called on it after the message was being created. But creating a message automatically initializes the string, so it was getting double initialized.
@jimmy-mcelwain jimmy-mcelwain changed the title Memory leak fix Partial memory leak fix Nov 13, 2024
@ted-miller
Copy link
Collaborator

Are you sure that one line is the only 'relevant' fix? Your other branch had a lot more changes.

I just did 7 disconnects in a row and got these results. It seems to conflict with what you stated during your tests.

Initialization complete. Memory available: (992672) bytes. Memory in use: (55904) bytes
Initialization complete. Memory available: (992632) bytes. Memory in use: (55944) bytes
Initialization complete. Memory available: (992616) bytes. Memory in use: (55960) bytes
Initialization complete. Memory available: (992592) bytes. Memory in use: (55984) bytes
Initialization complete. Memory available: (992560) bytes. Memory in use: (56016) bytes
Initialization complete. Memory available: (992512) bytes. Memory in use: (56064) bytes
Initialization complete. Memory available: (992536) bytes. Memory in use: (56040) bytes

@jimmy-mcelwain
Copy link
Collaborator Author

Ignore the memory available/memory in use printout. That is printed out in the middle of a connect/disconnect cycle. I don't know exactly why it varies (it does on my system too), but I'd guess that it has to do with the still-present memory leak (could be messing with struct byte alignment or something like that). The relevant statistic is the full_connection_cycle delta, which is printed out at the actual end of the connect/disconnect cycle. The other changes that I took out were almost entirely logging statements to nail down the second memory leak.

There are two parallel tasks which are still spooling up when this is printed.
Copy link
Collaborator

@ted-miller ted-miller left a comment

Choose a reason for hiding this comment

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

I see. There are two tasks still starting up. So yes, you are correct.

@ted-miller ted-miller merged commit 7fa3d6c into main Nov 15, 2024
5 of 14 checks passed
@ted-miller ted-miller deleted the memory_leak_fix branch November 15, 2024 15:13
Comment on lines -146 to +147
Ros_Debug_BroadcastMsg("Initialization complete. Memory available: (%d) bytes. Memory in use: (%d) bytes",
mpNumBytesFree(), MP_MEM_PART_SIZE - mpNumBytesFree());
Ros_Debug_BroadcastMsg("Initialization complete.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we not move this to a place where initialisation was really complete?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, there isn't anywhere to put it where it will give a deterministic value. We will need to add some semaphores to indicate init-complete on each task and wait for those before dropping into the main loop.

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.

3 participants