-
Notifications
You must be signed in to change notification settings - Fork 417
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 memory leak in tracetools::get_symbol() #2104
Fix memory leak in tracetools::get_symbol() #2104
Conversation
This is the simplest fix, but it could be improved. For example, we could first check if the tracepoint is actually enabled before calling |
Isn't the macro sufficient to prevent using this code if tracetools is disabled? |
When building with tracepoints enabled, there will be some runtime associated here, even if the tracepoints are not actively being used. I think this is more important as we are discussing building with tracepoints by default in the binary packages. |
In this case, this tracepoint is only triggered during initialization, so the overhead is fairly small. But, yes, being able to not do this kind of processing if a tracepoint is disabled at runtime would be much better. Since that change (as described here ros2/ros2_tracing#43 (comment)) requires modifications to |
93bbc61
to
70d0953
Compare
Testing Includes: |
70d0953
to
41e046a
Compare
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
41e046a
to
432d1f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approval for good measure
@mjcarroll thanks 😆 I can't merge this PR, so I'll let you merge it. |
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
Part of ros2/ros2_tracing#34
Requires ros2/ros2_tracing#43
Requires ros2/ros2_tracing#46 to help avoid memory allocations when the tracepoint isn't enabled.
Signed-off-by: Christophe Bedard christophe.bedard@apex.ai