After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775541 - leaks: Implement ref/unref tracing in the leaks tracer
leaks: Implement ref/unref tracing in the leaks tracer
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 775365
Blocks:
 
 
Reported: 2016-12-02 23:28 UTC by Thibault Saunier
Modified: 2016-12-21 20:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
leaks: Allow passing a GstStructure to configure the tracer (4.24 KB, patch)
2016-12-02 23:28 UTC, Thibault Saunier
committed Details | Review
leaks: Allow tracing Gst(Mini)Object reffing operations (15.88 KB, patch)
2016-12-02 23:28 UTC, Thibault Saunier
committed Details | Review
leaks: Allow user to set the flags to use to retrieve stack traces (3.64 KB, patch)
2016-12-02 23:28 UTC, Thibault Saunier
committed Details | Review
design: Update part-tracing about the leaks tracer (1.83 KB, patch)
2016-12-05 14:17 UTC, Thibault Saunier
accepted-commit_now Details | Review

Description Thibault Saunier 2016-12-02 23:28:11 UTC
Making it much more usefull to investigate leaks
Comment 1 Thibault Saunier 2016-12-02 23:28:15 UTC
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
Comment 2 Thibault Saunier 2016-12-02 23:28:21 UTC
Created attachment 341282 [details] [review]
leaks: Allow tracing Gst(Mini)Object reffing operations

It makes it much simpler to later debug refcount issues.
Comment 3 Thibault Saunier 2016-12-02 23:28:27 UTC
Created attachment 341283 [details] [review]
leaks: Allow user to set the flags to use to retrieve stack traces
Comment 4 Tim-Philipp Müller 2016-12-03 09:56:09 UTC
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 :)).
Comment 5 Thibault Saunier 2016-12-03 11:51:33 UTC
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.
Comment 6 Thibault Saunier 2016-12-03 12:35:25 UTC
(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? :)
Comment 7 Guillaume Desmottes 2016-12-05 08:12:14 UTC
(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.
Comment 8 Thibault Saunier 2016-12-05 14:17:18 UTC
Created attachment 341399 [details] [review]
design: Update part-tracing about the leaks tracer
Comment 9 Thibault Saunier 2016-12-12 14:28:35 UTC
Guillaume do you think you could review those patches please :)
Comment 10 Guillaume Desmottes 2016-12-20 15:49:51 UTC
Review of attachment 341281 [details] [review]:

++
Comment 11 Guillaume Desmottes 2016-12-20 15:57:56 UTC
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?
Comment 12 Guillaume Desmottes 2016-12-20 16:06:01 UTC
Review of attachment 341283 [details] [review]:

++
Comment 13 Guillaume Desmottes 2016-12-20 16:07:04 UTC
Review of attachment 341399 [details] [review]:

++
Comment 14 Thibault Saunier 2016-12-20 18:28:46 UTC
(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.
Comment 15 Thibault Saunier 2016-12-20 18:29:55 UTC
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
Comment 16 Thibault Saunier 2016-12-20 18:36:02 UTC
Thanks for the review!
Comment 17 Stefan Sauer (gstreamer, gtkdoc dev) 2016-12-21 20:58:09 UTC
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?