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 767857 - leaks tracer: list alive objects on demand and add 'checkpointing' support
leaks tracer: list alive objects on demand and add 'checkpointing' support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 767862 770373
 
 
Reported: 2016-06-20 08:29 UTC by Guillaume Desmottes
Modified: 2016-08-25 09:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
leaks tracer: log alive objects when receiving SIGUSR1 (2.63 KB, patch)
2016-06-20 08:30 UTC, Guillaume Desmottes
none Details | Review
leaks: add checkpoint support using SIGUSR2 (6.33 KB, patch)
2016-06-20 08:30 UTC, Guillaume Desmottes
none Details | Review
leaks tracer: log alive objects when receiving SIGUSR1 (2.83 KB, patch)
2016-06-22 10:45 UTC, Guillaume Desmottes
none Details | Review
leaks: add checkpoint support using SIGUSR2 (6.54 KB, patch)
2016-06-22 10:46 UTC, Guillaume Desmottes
none Details | Review
leaks tracer: log alive objects when receiving SIGUSR1 (3.90 KB, patch)
2016-06-23 10:22 UTC, Guillaume Desmottes
committed Details | Review
leaks: add checkpoint support using SIGUSR2 (6.33 KB, patch)
2016-06-23 10:22 UTC, Guillaume Desmottes
committed Details | Review
leaks: use G_OS_UNIX to check for signal support (3.95 KB, patch)
2016-07-08 09:16 UTC, Guillaume Desmottes
none Details | Review
leaks: use G_OS_UNIX to check for signal support (3.93 KB, patch)
2016-07-08 09:21 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2016-06-20 08:29:50 UTC
A while ago I implemented a couple of useful features to https://github.com/danni/gobject-list :
- List currently alive objects when receiving SIGUSR1
- Set 'checkpoints' when receiving SIGUSR2 and display objects created/destroyed since the last checkpoint

I figured this may be an useful addition to the leaks tracer so I implemented the same logic there.
Comment 1 Guillaume Desmottes 2016-06-20 08:30:22 UTC
Created attachment 330051 [details] [review]
leaks tracer: log alive objects when receiving SIGUSR1

We don't want to automatically catch signals so use an env variable to
enable this feature.
Comment 2 Guillaume Desmottes 2016-06-20 08:30:27 UTC
Created attachment 330052 [details] [review]
leaks: add checkpoint support using SIGUSR2
Comment 3 Sebastian Dröge (slomo) 2016-06-21 08:02:12 UTC
Review of attachment 330051 [details] [review]:

::: plugins/tracers/gstleaks.c
@@ +165,3 @@
   self->objects = g_hash_table_new (NULL, NULL);
+
+  instances = g_list_append (instances, self);

You probably want to use a GQueue here, for nicer API and O(1) append :)

@@ +366,3 @@
+
+  if (g_getenv ("GST_LEAKS_TRACER_SIG")) {
+    signal (SIGUSR1, sig_usr1_handler);

This probably has to be put into some #ifdefs and corresponding configure checks, also you need to #include signal.h?
Comment 4 Sebastian Dröge (slomo) 2016-06-21 08:07:45 UTC
Review of attachment 330052 [details] [review]:

::: plugins/tracers/gstleaks.c
@@ +413,3 @@
+  log_checkpoint (self->added, tr_added);
+  GST_TRACE_OBJECT (self, "listing objects removed since last checkpoint");
+  log_checkpoint (self->added, tr_removed);

Shouldn't this use self->removed?
Comment 5 Guillaume Desmottes 2016-06-22 10:44:47 UTC
Review of attachment 330051 [details] [review]:

::: plugins/tracers/gstleaks.c
@@ +165,3 @@
   self->objects = g_hash_table_new (NULL, NULL);
+
+  instances = g_list_append (instances, self);

done.

@@ +366,3 @@
+
+  if (g_getenv ("GST_LEAKS_TRACER_SIG")) {
+    signal (SIGUSR1, sig_usr1_handler);

What would be the best way to check that? CHECK_FUNCTION_EXISTS() ?
Comment 6 Guillaume Desmottes 2016-06-22 10:45:06 UTC
Review of attachment 330052 [details] [review]:

::: plugins/tracers/gstleaks.c
@@ +413,3 @@
+  log_checkpoint (self->added, tr_added);
+  GST_TRACE_OBJECT (self, "listing objects removed since last checkpoint");
+    gst_tracer_record_log (record, obj->type_name, obj->object);

indeed; fixed.
Comment 7 Guillaume Desmottes 2016-06-22 10:45:21 UTC
Created attachment 330186 [details] [review]
leaks tracer: log alive objects when receiving SIGUSR1

We don't want to automatically catch signals so use an env variable to
enable this feature.
Comment 8 Guillaume Desmottes 2016-06-22 10:46:20 UTC
Created attachment 330187 [details] [review]
leaks: add checkpoint support using SIGUSR2
Comment 9 Sebastian Dröge (slomo) 2016-06-23 06:39:37 UTC
(In reply to Guillaume Desmottes from comment #5)

> What would be the best way to check that? CHECK_FUNCTION_EXISTS() ?

That should work, yes. AC_CHECK_FUNC() and AC_CHECK_HEADERS().
Comment 10 Sebastian Dröge (slomo) 2016-06-23 06:40:25 UTC
Comment on attachment 330186 [details] [review]
leaks tracer: log alive objects when receiving SIGUSR1

You also should #include <signal.h> for signal()
Comment 11 Sebastian Dröge (slomo) 2016-06-23 06:41:34 UTC
Comment on attachment 330187 [details] [review]
leaks: add checkpoint support using SIGUSR2

This one seems good
Comment 12 Guillaume Desmottes 2016-06-23 10:22:47 UTC
Created attachment 330244 [details] [review]
leaks tracer: log alive objects when receiving SIGUSR1

We don't want to automatically catch signals so use an env variable to
enable this feature.
Comment 13 Guillaume Desmottes 2016-06-23 10:22:52 UTC
Created attachment 330245 [details] [review]
leaks: add checkpoint support using SIGUSR2
Comment 14 Sebastian Dröge (slomo) 2016-06-24 08:19:06 UTC
Comment on attachment 330244 [details] [review]
leaks tracer: log alive objects when receiving SIGUSR1

Looks good to me. Other opinions?
Comment 15 Guillaume Desmottes 2016-07-08 08:16:46 UTC
No objection? Can we merge this? :)
Comment 16 Sebastian Dröge (slomo) 2016-07-08 08:28:48 UTC
commit 3bb5c1e73ad30877db78f5f9628eeedc294c430c
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Wed Jun 1 11:08:39 2016 +0200

    leaks tracer: add checkpoint support using SIGUSR2
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767857

commit 53d2e8c9772eac2769d7ebec016b1c6e1748b9a4
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Tue May 31 16:56:26 2016 +0200

    leaks tracer: log alive objects when receiving SIGUSR1
    
    We don't want to automatically catch signals so use an env variable to
    enable this feature.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767857
Comment 17 Guillaume Desmottes 2016-07-08 09:16:24 UTC
Created attachment 331061 [details] [review]
leaks: use G_OS_UNIX to check for signal support

Checking for signal.h is not good enough as it's present in Windows.
Those signals are UNIX specific anyway.
Comment 18 Guillaume Desmottes 2016-07-08 09:21:48 UTC
Created attachment 331062 [details] [review]
leaks: use G_OS_UNIX to check for signal support

Checking for signal.h is not good enough as it's present in Windows.
Those signals are UNIX specific anyway.
Comment 19 Sebastian Dröge (slomo) 2016-07-08 09:39:14 UTC
commit 1ed4140d0068791f8993403a11c2c961552fd7e0
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Fri Jul 8 11:15:06 2016 +0200

    leaks tracer: use G_OS_UNIX to check for signal support
    
    Checking for signal.h is not good enough as it's present in Windows.
    Those signals are UNIX specific anyway.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767857