GNOME Bugzilla – Bug 766008
(mini)object: add flag marking "leaked" objects
Last modified: 2016-07-13 09:30:56 UTC
As suggested on bug #765540 I tried a new approach to filter out objects which are intentionally "leaked". I did so by introducing a new flag on Gst(Mini)Object allowing us to explicitly mark such objects in the code. I then played with GST_TRACE in order to determine if it could be used as a proper leaks detection tool. I'm pretty happy with the results so far! I fixed quiet a bunch of leaks in gst-validate and core. All the core tests are now passing with no leak detected. See this branch for the code: https://git.collabora.com/cgit/user/cassidy/gstreamer/gstreamer.git/log/?h=trace-leaks This kind of tool is not going to be a full replacements of valgrind but I think it could be a nice addition to our toolbox. I see the following advantages in this approach: - Much lighter to use than valgrind, which should make it usable on embedded device with low ressources (I didn't try this yet). - Integrated in core so no need to build external tools which can sometime be problematic on some platform/devices. - Much faster than valgrind so can easily be run with "make check" to detect/prevent leak regressions. - Better control on what is being filtered out; valgrind suppression files can be a pretty big hammer. - Track only leaks in gst code so should be more robust and consistent accross different platforms or system lib versions. This is important to reduce the "noise" if we integrate this as part of our automatic QA. I'm still not sure if GST_TRACE is the proper tool for that, but we could easily implement similar features using a GstTracer. For the record here is the list of bugs I found during this experimentation: https://phabricator.freedesktop.org/D958 https://phabricator.freedesktop.org/D959 https://bugzilla.gnome.org/show_bug.cgi?id=765706 https://phabricator.freedesktop.org/D979 https://phabricator.freedesktop.org/D980 https://bugzilla.gnome.org/show_bug.cgi?id=765719 https://bugzilla.gnome.org/show_bug.cgi?id=765720 https://bugzilla.gnome.org/show_bug.cgi?id=765903 https://bugzilla.gnome.org/show_bug.cgi?id=765904 https://bugzilla.gnome.org/show_bug.cgi?id=765957 https://bugzilla.gnome.org/show_bug.cgi?id=765958 https://bugzilla.gnome.org/show_bug.cgi?id=765961 https://bugzilla.gnome.org/show_bug.cgi?id=765976 https://bugzilla.gnome.org/show_bug.cgi?id=765978 So, is that something we could consider adding to core at some point?
Having a GstTracer for that seems nicer, than parsing free-form GST_TRACE output :) Otherwise seems very useful to have, just not sure what to do about GObjects... as we don't have control over g_object_ref/unref and can't add a tracer in there. We could however add a GstObject init/finalize hook for GstTracer
Yeah we can just track creation/destruction of GObject and GstMiniObject as GST_TRACE is already doing atm.
Also having a GstMiniObjectFlag/GstObjectFlag for objects that are certainly going to be leaked seems useful. Can you create a separate bug about this? And also it seems like that new bug, and bug #765052 would then become "sub-bugs" of this one?
Ok then we can just use this bug for the new flag thing and turn bug #765052 to a tracer checking for leaks instead of my GST_TRACE hack.
Created attachment 327493 [details] [review] (mini)object: add LEAKED flag
Created attachment 327495 [details] [review] use LEAKED (mini) object flag
Comment on attachment 327493 [details] [review] (mini)object: add LEAKED flag This looks fine to me, but I'd like to brainstorm/bikeshed the name a little bit more. It's not guaranteed to be leaked, so perhaps FLAG_PERSISTENT or FLAG_GLOBAL or FLAG_MAY_BE_LEAKED or something? :) Also, please add (Since: 1.10) at the end of the new flag description line.
(In reply to Tim-Philipp Müller from comment #7) > Comment on attachment 327493 [details] [review] [review] > (mini)object: add LEAKED flag > > This looks fine to me, but I'd like to brainstorm/bikeshed the name a little > bit more. It's not guaranteed to be leaked, so perhaps FLAG_PERSISTENT or > FLAG_GLOBAL or FLAG_MAY_BE_LEAKED or something? :) I agree LEAKED is a bit miss-leading, any one you suggested is fine with me, just pick one and I'll use it. :) > Also, please add (Since: 1.10) at the end of the new flag description line. Will do.
(In reply to Guillaume Desmottes from comment #8) > (In reply to Tim-Philipp Müller from comment #7) > > Comment on attachment 327493 [details] [review] [review] [review] > > (mini)object: add LEAKED flag > > > > This looks fine to me, but I'd like to brainstorm/bikeshed the name a little > > bit more. It's not guaranteed to be leaked, so perhaps FLAG_PERSISTENT or > > FLAG_GLOBAL or FLAG_MAY_BE_LEAKED or something? :) I think I'd vote for FLAG_MAY_BE_LEAKED as some may not be "global" (in the C sense of it) and it's clearer about the actual use of this flag.
Created attachment 328169 [details] [review] (mini)object: add MAY_BE_LEAKED flag
Comment on attachment 327495 [details] [review] use LEAKED (mini) object flag Needs to be updated for the new name
Comment on attachment 328169 [details] [review] (mini)object: add MAY_BE_LEAKED flag Let's get this in together with the tracer
Comment on attachment 327495 [details] [review] use LEAKED (mini) object flag You might want to add GstSystemClock here, but only the singleton instance not all instances
(In reply to Sebastian Dröge (slomo) from comment #13) > Comment on attachment 327495 [details] [review] [review] > use LEAKED (mini) object flag > > You might want to add GstSystemClock here, but only the singleton instance > not all instances Actually I don't, gst_deinit() already takes care of destroying it: https://cgit.freedesktop.org/gstreamer/gstreamer/tree/gst/gst.c#n1021
Created attachment 328243 [details] [review] use MAY_BE_LEAKED (mini) object flag
Comment on attachment 328243 [details] [review] use MAY_BE_LEAKED (mini) object flag Re. all this set_leaked_foreach() / set_leaked_on_value() / priv_gst_buffer_set_may_be_leaked() - did you have a use case where this was required in practice? It's technically correct of course, but I would've thought it should only be needed if we have buffer fields in static caps, and I don't think one would ever do that. Do you remember where this was needed?
(In reply to Tim-Philipp Müller from comment #16) > Comment on attachment 328243 [details] [review] [review] > use MAY_BE_LEAKED (mini) object flag > > Re. all this set_leaked_foreach() / set_leaked_on_value() / > priv_gst_buffer_set_may_be_leaked() - did you have a use case where this was > required in practice? Yes, all this code has been added after loads of testing in order to have "make check" passing with all gst components. > It's technically correct of course, but I would've thought it should only be > needed if we have buffer fields in static caps, and I don't think one would > ever do that. Do you remember where this was needed? We have some here with the 'codec_data' key https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/tests/check/elements/qtmux.c#n49
Hrm ok. It's a bit unfortunate to add all this for something that's just needed in one or two unit tests imho.
I can move the code to the test itself I suppose, but that means it will just be duplicated each time we need it. I don't think it's that bad to have it in core tbh.
I think the qtmux test should be changed, it's not a normal scenario really. The code does have a small performance impact, and it also doesn't handle all theoretical or practical scenarios, so I'd prefer to just skip it for now if it's just one or two tests that need changing.
(In reply to Tim-Philipp Müller from comment #20) > I think the qtmux test should be changed, it's not a normal scenario really. Ok I'll let you take a look at this test then. Btw, I have the same problem here: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/tests/check/elements/matroskamux.c#n36 (In reply to Tim-Philipp Müller from comment #20) > The code does have a small performance impact, and it also doesn't handle > all theoretical or practical scenarios, so I'd prefer to just skip it for > now if it's just one or two tests that need changing. Ok, I'll remove it for now then.
Created attachment 328719 [details] [review] use MAY_BE_LEAKED_FLAG This helps having "make check" passing with the leaks tracer enabled.
commit 4a41468ce7aa8abbf354c19c7d1bf5e8d1327048 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon May 30 12:11:13 2016 +0200 Use MAY_BE_LEAKED_FLAG This helps having "make check" passing with the leaks tracer enabled. https://bugzilla.gnome.org/show_bug.cgi?id=766008
(In reply to Tim-Philipp Müller from comment #20) > I think the qtmux test should be changed, it's not a normal scenario really. > > The code does have a small performance impact, and it also doesn't handle > all theoretical or practical scenarios, so I'd prefer to just skip it for > now if it's just one or two tests that need changing. For the record, I opened bug #768762 about this.