-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: Converter memleak #21
Conversation
Thank you, changes are looking reasonable! I have tried to perform some super simple testing, but perhaps it's not detailed enough, see below. I might still try to test this further. I have used
This is the result on the current main (5922481), before/after:
This is the result including your improvements, before/after:
In both cases, the VSZ and RSS numbers grow, although they grow less on your branch. |
Thanks for testing @lreiher. Looks better indeed, however I'm also afraid it is not detailed enough. This increase in memory usage can be normal or not. |
I just ran Leak summary (main)
Leak summary (these improvements on main)
Full output (main)
Full output (these improvements on main)
|
Here's the reported leaks where the two versions differ.
|
|
Yep, took me some time, but also just identified that! Freeing
|
I'm going to quickly commit these changes and resolve the conflicts with main. :) |
Hm, weird, I don't have permissions to push to this branch of your fork. I have now created a PR to your fork that you should merge, afterwards this PR here should be ready to go: Don't worry about all the changes, that's mostly updating with the current main here. The only relevant change is that I have added this line 6 times. |
Resolve conflicts with ika-rwth-aachen/etsi_its_messages and free ret.buffer
Fix missing cpm_ts typos after pr #21
Frees the memory of the allocations done by asn1c.
I'm gonna ask you guys to please test this one as I don't have an UDP-based setup to test this node.