GNOME Bugzilla – Bug 765052
tracer: add (mini) object leak tracer
Last modified: 2016-06-20 09:41:55 UTC
I was interested playing a bit with tracers and wrote a refcounting tracer while doing so. :)
Created attachment 325998 [details] [review] tracer: add hooks when GstOject are reffed/unreffed
Created attachment 325999 [details] [review] tracerutils: include gstutils in header The GST_TRACER_TS macro uses gst_util_get_timestamp() which is defined in gstutils.
Created attachment 326000 [details] [review] tracer: add hooks when GstMiniObject are reffed/unreffed
Created attachment 326001 [details] [review] tracers: also trigger "ref count" hooks when objects are created I'm not sure if it's worth having another tracer hook for it. Tracking refcounting seems good enough to track creation and destruction of objects.
Created attachment 326002 [details] [review] structure: hande pointers when creating template formats Tracers may want to display the address of an object.
Created attachment 326004 [details] [review] tracers: add refcounts tracer
Created attachment 326005 [details] [review] part-tracing: update as refcounts tracer is now implemented
Created attachment 326236 [details] [review] tracers: add refcounts tracer
Out of curiosity - did you actually use this to track down anything or was the primary goal to play with the tracing system a bit? The reason I'm asking is that I'm not entirely sure if tracing refcounts like this is really useful, and if it's just for learning tracer but of limited use in practice then I'd be wondering about the possible additional overhead. I think what we really need is something else, namely tracing ownership *transfers*. Now you could say that is very interesting and we should definitely do that as well, but it's orthogonal to this bug, and that'd be fair, just thought I'd mention it ;) There's also a problem with this kind of tracing, namely that it only works reliably for mini object, for GObjects we won't be able to trace stuff that goes through GObject directly, such as GValue stuff, property setters/getters, bindings stuff etc.
I actually used this kind of things when I added valgrind support to gst-validate and was tracking leaks in various scenarios. It wasn't this code (didn't have tracers yet) but the idea was the same: tracking gst_(mini_)object_ref/unref calls using printf and parsing them using a script. I also did some magic with gdb at this time: http://blog.desmottes.be/?post/2015/03/30/Tracking-the-ref-count-of-a-GstMiniObject This was especially useful when tracking events and caps leaks for example. So yeah, I do think this kind of tool can actually be useful. I agree with you that it is not perfect, especially with GObject, and tools like refdbg or gobject-list may be a better fit. But I think this may be a nice addition to the existing tools, if only because of the convenience to have it embedded with gst core as using external tools can sometime not be easily doable on some hardware/platform. On the other hand I can see the point of the GObject limitation being a problem. So it could make sense to reduce the scope of this tracer to GstMiniObject only.
Created attachment 326276 [details] [review] tracers: add refcounts tracer
Created attachment 326310 [details] [review] tracer: add hooks when GstOject are reffed/unreffed
Created attachment 326311 [details] [review] tracer: add hooks when GstMiniObject are reffed/unreffed
Created attachment 326312 [details] [review] tracers: add refcounts tracer
I pushed all those patches to https://git.collabora.com/cgit/user/cassidy/gstreamer/gstreamer.git/log/?h=refcounts as well
Review of attachment 326310 [details] [review]: The problem with this GstObject refcount trace is that it's totally valid to just use g_object_ref/unref() so a lot of refcount changes will be missed.. I'd stick to miniobject for now unless we can trace into GLib.
Ok, fair enough. I removed GObject support. The tracer tracks only GstMiniObject now. This is still very useful to track caps and events leaks.
Created attachment 326662 [details] [review] tracer: add hooks when GstMiniObject are reffed/unreffed
Created attachment 326663 [details] [review] tracers: also trigger "ref count" hooks when mini objects are created I'm not sure if it's worth having another tracer hook for it. Tracking refcounting seems good enough to track creation and destruction of objects.
Created attachment 326664 [details] [review] tracers: add refcounts tracer
(In reply to Guillaume Desmottes from comment #2) > Created attachment 325999 [details] [review] [review] > tracerutils: include gstutils in header > > The GST_TRACER_TS macro uses gst_util_get_timestamp() which is defined > in gstutils. (In reply to Guillaume Desmottes from comment #5) > Created attachment 326002 [details] [review] [review] > structure: hande pointers when creating template formats > > Tracers may want to display the address of an object. Those 2 patches can be reviewed/merged even if we finally decide to not merge the tracer.
As mentioned here https://bugzilla.gnome.org/show_bug.cgi?id=766008#c1 it might be still useful to have instance_init/finalize tracers for GstObjects if we can't have ref/unref tracers Also are init/finalize tracers enough for GstMiniObject or is there a use-case for having tracers for each ref/unref?
Created attachment 327494 [details] [review] use LEAKED (mini) object flag
Comment on attachment 327494 [details] [review] use LEAKED (mini) object flag wrong bug, sorry.
As discussed on bug #766008 let's turn this tracer to one only tracking creation and destruction of objects and mini objects. We could always re-add ref/unref hooks later if needed.
Created attachment 327589 [details] [review] (mini)object: add tracer hooks when init and finalized
Created attachment 327590 [details] [review] add leaks tracer
Created attachment 327792 [details] [review] add leaks tracer
Comment on attachment 327589 [details] [review] (mini)object: add tracer hooks when init and finalized Looks generally fine to me. I wonder if we should override GObject::constructed() and call the OBJECT_INIT trace macro from there, because in the _init() function the object is not fully set up yet and the class won't be the final class yet iirc, in case it's a sub-class.
Comment on attachment 327792 [details] [review] add leaks tracer Looks okay at first glance (haven't tried it yet), we can optimise it later I suppose. >+static void >+set_filtering (GstLeaksTracer * self) >+ ... >+ self->filter = g_list_prepend (self->filter, GUINT_TO_POINTER (type)); >+ } >+ ... >+} and >+static gboolean >+should_handle_object (GstLeaksTracer * self, gpointer object) >+{ >+ ... >+ for (l = self->filter; l != NULL; l = g_list_next (l)) { >+ GType type = GPOINTER_TO_UINT (l->data); >+ ... >+} I think this is not quite right, sizeof(GType) can be > sizeof(guint).
Yes, use GPOINTER_TO_GSIZE() and the other way around. See the GValue GType handling in e.g. glib's gparamspecs.c Also this should really happen in GObject::constructed. What Tim remembered about the class not being the real class yet in instance_init is correct (which is why there is a second, hidden parameter with the real class struct). In GstObject's instance_init you will *always* get the original GObjectClass (that is, G_TYPE_FROM_CLASS() == G_TYPE_OBJECT).
I was also wondering, your leak patches for the tests etc, are they generated with the help of this? Can you document somewhere how you did it exactly? :)
(In reply to Tim-Philipp Müller from comment #29) > Comment on attachment 327589 [details] [review] [review] > (mini)object: add tracer hooks when init and finalized > > I wonder if we should override GObject::constructed() and call the > OBJECT_INIT trace macro from there, because in the _init() function the > object is not fully set up yet and the class won't be the final class yet > iirc, in case it's a sub-class. Would you still call the tracer OBJECT_INIT then? Maybe (MINI_)_OBJECT_CREATED/DESTROYED would be more generic?
(In reply to Sebastian Dröge (slomo) from comment #31) > Yes, use GPOINTER_TO_GSIZE() and the other way around. See the GValue GType > handling in e.g. glib's gparamspecs.c > > Also this should really happen in GObject::constructed. What Tim remembered > about the class not being the real class yet in instance_init is correct > (which is why there is a second, hidden parameter with the real class > struct). In GstObject's instance_init you will *always* get the original > GObjectClass (that is, G_TYPE_FROM_CLASS() == G_TYPE_OBJECT). Fixed. I'll attach the new patch once we'll have agreed on the tracer hooks and flag names. (In reply to Sebastian Dröge (slomo) from comment #32) > I was also wondering, your leak patches for the tests etc, are they > generated with the help of this? Can you document somewhere how you did it > exactly? :) Yep, I just ran 'GST_TRACERS="leaks" make check' and the debugged each failing test. :) My plan would be to have this automatically done by a Jenkins bot as part of our existing QA tests.
> Would you still call the tracer OBJECT_INIT then? Maybe > (MINI_)_OBJECT_CREATED/DESTROYED would be more generic? Sounds like a good idea to me.
Created attachment 327962 [details] [review] (mini)object: add tracer hooks when created and destroyed
Guillaume, i'd like to try this with 1.6.4 but i have trouble finding the correct order in which to apply that abundance of patches.
(In reply to Andreas Frisch from comment #37) > Guillaume, i'd like to try this with 1.6.4 but i have trouble finding the > correct order in which to apply that abundance of patches. Take this branch, much easier :) https://git.collabora.com/cgit/user/cassidy/gstreamer/gstreamer.git/log/?h=leak-tracer
Created attachment 328244 [details] [review] (mini)object: add tracer hooks when created and destroyed
Created attachment 328245 [details] [review] add leaks tracer
Created attachment 328246 [details] [review] plugin: remove feature refcount check This check fails if one, or more, tracers are loaded while running the test. My "leaks" tracer will check for leaks anyway.
Created attachment 328247 [details] [review] gst_deinit: move down tracers cleaning We want the tracer detecting leaks to be finalized as late as possible to give the chance to other gst components to be properly cleaned first.
Created attachment 328257 [details] [review] add leaks tracer
(In reply to Guillaume Desmottes from comment #42) > Created attachment 328247 [details] [review] [review] > gst_deinit: move down tracers cleaning > > We want the tracer detecting leaks to be finalized as late as possible > to give the chance to other gst components to be properly cleaned first. This seems dangerous as the tracers might also want to use other GStreamer API :)
(In reply to Sebastian Dröge (slomo) from comment #44) > (In reply to Guillaume Desmottes from comment #42) > > Created attachment 328247 [details] [review] [review] [review] > > gst_deinit: move down tracers cleaning > > > > We want the tracer detecting leaks to be finalized as late as possible > > to give the chance to other gst components to be properly cleaned first. > > This seems dangerous as the tracers might also want to use other GStreamer > API :) What kind of API will they not be able to use? The caps stuffs are still alive at this point so those are safe.
(In reply to Guillaume Desmottes from comment #39) > Created attachment 328244 [details] [review] [review] > (mini)object: add tracer hooks when created and destroyed Humm actually I don't really need the destroyed tracer, a weak ref could do the job as well. Actually it may even be better, I'm having troubles when using the leak tracer with gst-validate which has its own tracer as well: object destroyed while its tracer are not reported by the destroy hooks as tracing has been disabled (_priv_tracer_enabled = FALSE). What do you guys think? Should we keep only the creation hooks or have destruction as well to stay symetric even if not used by the leaks tracer?
It seems nicer to keep the symmetry.
Created attachment 328718 [details] [review] add leaks tracer
Created attachment 328808 [details] [review] add leaks tracer
Review of attachment 325999 [details] [review]: Can you merge that into the first patch that needs it for context?
Created attachment 328867 [details] [review] (mini)object: add tracer hooks when created and destroyed
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #50) > Review of attachment 325999 [details] [review] [review]: > > Can you merge that into the first patch that needs it for context? Done. That's the patch adding the tracer hooks.
Review of attachment 328808 [details] [review]: ::: plugins/tracers/Makefile.am @@ +23,3 @@ gststats.c \ + gsttracers.c \ + gstleaks.c please keep list sorted @@ +38,3 @@ gstrusage.h \ + gststats.h \ + gstlatency.h please keep list sorted ::: plugins/tracers/gstleaks.c @@ +70,3 @@ + + self->filter = g_list_prepend (self->filter, GSIZE_TO_POINTER (type)); + } consider a GPtrArray since this is created once and never modifies, you can even know the size beforehand. @@ +98,3 @@ + if (GST_IS_MINI_OBJECT_TYPE (object, type)) + return TRUE; + } What about getting the object/mini-object type outside of the loop, so that you only compare types inside the loop @@ +130,3 @@ + + if (G_IS_OBJECT (object)) + g_object_weak_ref (G_OBJECT (object), object_weak_cb, self); maybe (GObject *) for perf reasons
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #53) > ::: plugins/tracers/gstleaks.c > @@ +70,3 @@ > + > + self->filter = g_list_prepend (self->filter, GSIZE_TO_POINTER (type)); > + } > > consider a GPtrArray since this is created once and never modifies, you can > even know the size beforehand. GArray<GType> would be even cleaner :)
Review of attachment 328808 [details] [review]: ::: plugins/tracers/Makefile.am @@ +23,3 @@ gststats.c \ + gsttracers.c \ + gstleaks.c done. @@ +38,3 @@ gstrusage.h \ + gststats.h \ + gstlatency.h done. ::: plugins/tracers/gstleaks.c @@ +70,3 @@ + + self->filter = g_list_prepend (self->filter, GSIZE_TO_POINTER (type)); + } Switched to a Garray<GType> as suggested. @@ +98,3 @@ + if (GST_IS_MINI_OBJECT_TYPE (object, type)) + return TRUE; + } good idea; done. @@ +130,3 @@ + + if (G_IS_OBJECT (object)) + g_object_weak_ref (G_OBJECT (object), object_weak_cb, self); done.
Created attachment 328873 [details] [review] add leaks tracer
Review of attachment 328873 [details] [review]: ::: plugins/tracers/Makefile.am @@ +24,2 @@ gststats.c \ + gsttracers.c almost, I ignored $(), so that '$(LOG...)' is sorted as 'gstlog...' ::: plugins/tracers/gstleaks.c @@ +26,3 @@ + * alive when program is exiting and raising a warning. + * The type of objects tracked can be filtered using the parameters of the + * tracer, for example: GST_TRACERS="leaks(GstEvent,GstMessage) nit: missing " at the end of line @@ +132,3 @@ +handle_object_created (GstLeaksTracer * self, gpointer object, GType type) +{ + if (!should_handle_object (self, object)) just pass the GType here as well, then you don't need to determine it from the object again. @@ +152,3 @@ + GstLeaksTracer *self = GST_LEAKS_TRACER (tracer); + + handle_object_created (self, object, GST_MINI_OBJECT_TYPE (object)); maybe inline the function to avoid the if G_IS_OBJECT check. it is not duplication a lot of code.
Review of attachment 328873 [details] [review]: ::: plugins/tracers/Makefile.am @@ +24,2 @@ gststats.c \ + gsttracers.c done. ::: plugins/tracers/gstleaks.c @@ +26,3 @@ + * alive when program is exiting and raising a warning. + * The type of objects tracked can be filtered using the parameters of the + * tracer, for example: GST_TRACERS="leaks(GstEvent,GstMessage) fixed. @@ +132,3 @@ +handle_object_created (GstLeaksTracer * self, gpointer object, GType type) +{ + if (!should_handle_object (self, object)) done. @@ +152,3 @@ + GstLeaksTracer *self = GST_LEAKS_TRACER (tracer); + + handle_object_created (self, object, GST_MINI_OBJECT_TYPE (object)); I hate duplicating code so I added an extra arg instead to avoid the extra check.
Created attachment 328885 [details] [review] add leaks tracer
Created attachment 328892 [details] [review] add leaks tracer
Seems to work nicely, let's get this in! Thanks for your patience! commit 0bb7fa985528bc94feddd9ebbece482078797c6e Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Tue May 10 09:29:12 2016 +0200 tracers: add leaks tracer https://bugzilla.gnome.org/show_bug.cgi?id=765052 commit 21070c811469ad392d7a0f3af0b0de69bacdc153 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon May 9 16:31:36 2016 +0200 tracing: add hooks when objects or miniobjects are created and destroyed https://bugzilla.gnome.org/show_bug.cgi?id=765052 commit 194964b4e03def69fc4c6c04bfa8e4e7daa2522e Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon May 9 16:56:56 2016 +0200 gst_deinit: move down tracers cleaning We want the tracer detecting leaks to be finalized as late as possible to give the chance to other gst components to be properly cleaned first. https://bugzilla.gnome.org/show_bug.cgi?id=765052 commit d479eefde06430b67291fcad4d27fc73ecb499a1 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Tue May 10 11:06:42 2016 +0200 tests: plugin: remove feature refcount assert This check fails if one, or more, tracers are loaded while running the test. The new "leaks" tracer will be able to check for leaks anyway. https://bugzilla.gnome.org/show_bug.cgi?id=765052 commit aa336008b378ebe24d56896f96240fc3bd96d5d4 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Thu Apr 14 12:25:43 2016 +0300 tracerrecord: allow G_TYPE_POINTER for field types Tracers may want to display the address of an object. https://bugzilla.gnome.org/show_bug.cgi?id=765052 (Some tests in core seem to sometimes leak, there's some raciness; but I think the same happens under valgrind.)
Two more things: 1) Any idea why valgrind did not catch the leaks you found? 2) Does it make sense to get backtrace() from glib and store/log is with the object creation to be able to report the origin of the leaks?
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #62) > Two more things: > 1) Any idea why valgrind did not catch the leaks you found? They were still reachable, at least for the ones I remember. > 2) Does it make sense to get backtrace() from glib and store/log is with the > object creation to be able to report the origin of the leaks? GLib has a portable way of getting (useful!) backtraces? That would indeed be great to add :)
I have more ideas for improvements as well, just thought we should get this in first :) My main question is this: why use weak refs rather than the object destroyed trace handler? weak refs cause additional memory allocations and locking, no? (still, something that can be improved later if we find that's the case.)
I agree that using weak refs for the destroy notify is not a great idea, it involves additional overhead that is not needed and it actually seems more code in the end than in the marked-as-committed patch here. Can we change that? :)
Code says: > /* We rely on weak pointers rather than (mini-)object-destroyed hooks so we > * are notified of objects being destroyed even during the shuting down of > * the tracing system. */ Would still be good if we could fix that though.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #62) > 2) Does it make sense to get backtrace() from glib and store/log is with the > object creation to be able to report the origin of the leaks? I tried using backtrace() a while ago but it wasn't that helpful. I don't remember exactly why. My post merge plan was to try using http://www.nongnu.org/libunwind/ to get traces.
(In reply to Guillaume Desmottes from comment #67) > (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #62) > > 2) Does it make sense to get backtrace() from glib and store/log is with the > > object creation to be able to report the origin of the leaks? > > I tried using backtrace() a while ago but it wasn't that helpful. I don't > remember exactly why. > My post merge plan was to try using http://www.nongnu.org/libunwind/ to get > traces. I opened bug #767862 to discuss this feature.