GNOME Bugzilla – Bug 767857
leaks tracer: list alive objects on demand and add 'checkpointing' support
Last modified: 2016-08-25 09:35:55 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.
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.
Created attachment 330052 [details] [review] leaks: add checkpoint support using SIGUSR2
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?
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?
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() ?
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.
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.
Created attachment 330187 [details] [review] leaks: add checkpoint support using SIGUSR2
(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 on attachment 330186 [details] [review] leaks tracer: log alive objects when receiving SIGUSR1 You also should #include <signal.h> for signal()
Comment on attachment 330187 [details] [review] leaks: add checkpoint support using SIGUSR2 This one seems good
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.
Created attachment 330245 [details] [review] leaks: add checkpoint support using SIGUSR2
Comment on attachment 330244 [details] [review] leaks tracer: log alive objects when receiving SIGUSR1 Looks good to me. Other opinions?
No objection? Can we merge this? :)
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
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.
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.
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