GNOME Bugzilla – Bug 775541
leaks: Implement ref/unref tracing in the leaks tracer
Last modified: 2016-12-21 20:58:09 UTC
Making it much more usefull to investigate leaks
Created attachment 341281 [details] [review] leaks: Allow passing a GstStructure to configure the tracer But keep understanding the simple synthax with a comma separated list of filters
Created attachment 341282 [details] [review] leaks: Allow tracing Gst(Mini)Object reffing operations It makes it much simpler to later debug refcount issues.
Created attachment 341283 [details] [review] leaks: Allow user to set the flags to use to retrieve stack traces
This is going to cause a lot of overhead, I wonder if it's really worth it? The problem with this data is that it's just a LOT of data - does it really help figure out leaks in non-trivial pipelines? The difficult part in GStreamer is usually to figure out leaks when ownership transfer occured, and this patch doesn't help with that (not meant as a criticism, that was not the goal I presume, just an observation :)).
Basically my workflow when using it is to first run without tracing refs nor backtrace to find out there is a leak and then filter out precisely what was leaked with both refs and backtrace on. The list of backtrace on each ref/unref definitely helps figuring out when ownership transfer happens.
(In reply to Tim-Philipp Müller from comment #4) > This is going to cause a lot of overhead, I wonder if it's really worth it? Did you check the patch and notice it is an opt-in feature btw? :)
(In reply to Tim-Philipp Müller from comment #4) > The problem with this data is that it's just a LOT of data - does it really > help figure out leaks in non-trivial pipelines? I think it does yeah. When I was debugging loads of gst leaks to test my leas tracer, I sometimes ended up tracking individual ref/unref calls using gdb ( https://blog.desmottes.be/?post/2015/03/30/Tracking-the-ref-count-of-a-GstMiniObject ). So this may be an useful addition. Would be good to update docs/design/part-tracing.txt with an example.
Created attachment 341399 [details] [review] design: Update part-tracing about the leaks tracer
Guillaume do you think you could review those patches please :)
Review of attachment 341281 [details] [review]: ++
Review of attachment 341282 [details] [review]: ::: gst/gsttracerutils.h @@ +600,3 @@ + * @self: the tracer instance + * @ts: the current timestamp + * @object: the object being unreffed Double spaces here. @@ +617,3 @@ + * @self: the tracer instance + * @ts: the current timestamp +typedef void (*GstTracerHookObjectUnreffed) (GObject *self, GstClockTime ts, here too. ::: plugins/tracers/gstleaks.c @@ -57,0 +58,10 @@ +typedef struct +{ + gboolean reffed; ... 7 more ... Call it 'creation_trace' to avoid confusion with ObjectRefingInfo->trace? Or at least add a comment. @@ -426,2 +535,3 @@ Leak *leak = l->data; + for (ref = leak->infos->refing_infos; ref; ref = ref->next) { Shouldn't you reverse the reffing_infos list first so they are listed in the right order?
Review of attachment 341283 [details] [review]: ++
Review of attachment 341399 [details] [review]: ++
(In reply to Guillaume Desmottes from comment #11) > Review of attachment 341282 [details] [review] [review]: > > ::: gst/gsttracerutils.h > @@ +600,3 @@ > + * @self: the tracer instance > + * @ts: the current timestamp > + * @object: the object being unreffed > > Double spaces here. FIXED > @@ +617,3 @@ > + * @self: the tracer instance > + * @ts: the current timestamp > +typedef void (*GstTracerHookObjectUnreffed) (GObject *self, GstClockTime ts, > > here too. FIXED > ::: plugins/tracers/gstleaks.c > @@ -57,0 +58,10 @@ > +typedef struct > +{ > + gboolean reffed; > ... 7 more ... > > Call it 'creation_trace' to avoid confusion with ObjectRefingInfo->trace? Or > at least add a comment. DONE, you are right it was confusing. > @@ -426,2 +535,3 @@ > Leak *leak = l->data; > > + for (ref = leak->infos->refing_infos; ref; ref = ref->next) { > > Shouldn't you reverse the reffing_infos list first so they are listed in the > right order? DONE. Pushing now.
Attachment 341281 [details] pushed as 32b17a8 - leaks: Allow passing a GstStructure to configure the tracer Attachment 341282 [details] pushed as 3013390 - leaks: Allow tracing Gst(Mini)Object reffing operations Attachment 341283 [details] pushed as 29f0a79 - leaks: Allow user to set the flags to use to retrieve stack traces
Thanks for the review!
A downside of of this is that it renders the rusage tracer useless. The ref/unref ops are called so frequently that hooking generic probe point to those will have too much overhead. As a short term fix we should probably explicitly register hooks for the rusage tracer. Not sure if it makes any sense in the long run to have more flxible registration (e.g. globs or provide a list of 'detail' strings). WDYT?