Created attachment 72257 [details] [review]
Proposed patch

Created attachment 75169 [details] [review]
Revised patch

I made the output format easier to parse.  It now looks like:

GLib-GObject-Message: <type-name> | created <n> | destroyed <n> | max population <n>

Created attachment 75170 [details]
Sample output

Sample output of an Evolution session.

Comment on attachment 75169 [details] [review]
Revised patch

thanks, very interesting submission. i'd still like to see some details reworked however.

> @G_TYPE_DEBUG_OBJECTS: Print messages about object bookkeeping.

this should better be changed to "Print messages about stale GObjects" to differentiate from your new bookkeeping.

> @G_TYPE_DEBUG_SIGNALS: Print messages about signal emissions.
>+@G_TYPE_DEBUG_TYPES: Print messages about type bookkeeping.

you're really bookkeeping instance and destruction so this should better say:
  +@G_TYPE_DEBUG_TYPES: Print messages about instance bookkeeping.

>+static gboolean
>+debug_types_foreach (gpointer key,
>+                     gpointer value,
>+                     gpointer user_data)
>+{
>+  GType type = GPOINTER_TO_SIZE (key);
>+  DebugTypeStats *stats = value;
>+
>+  g_message ("%s | created %u | destroyed %u | max population %u",
>+             g_type_name (type), stats->num_created, stats->num_destroyed,
>+             stats->max_population);

i don't think the current output format is very useful. while the collected stats are definitely interesting, i think they should be presented in a different format to make the interesting data more easily accessible.
i'd suggest to sort the output by 1) max_population and 2) n_created, and then print these out like:
  GLib-GObject-Message: max_population | n_created | n_destroyed | Instance Type
  GLib-GObject-Message:             11 |        86 |          86 | AtkRelationSet
  GLib-GObject-Message:             10 |        68 |          68 | AtkNoOpObject
  GLib-GObject-Message:              1 |         1 |           0 | AddressbookComponent
  GLib-GObject-Message:              1 |         0 |           1 | AtkNoOpObjectFactory

>@@ -3463,6 +3544,14 @@ g_type_init_with_debug_flags (GTypeDebug
>   /* Signal system
>    */
>   g_signal_init ();
>+
>+#ifdef G_ENABLE_DEBUG
>+  IF_DEBUG (TYPES)
>+    {
>+      debug_types_tree = g_tree_new (debug_types_compare);
>+      g_atexit (debug_types_atexit);

hm, i'd really expect a HashTable to be faster for these kind of lookups, which is why the otehr debugging features use hash tables to store this kind of information. have you tried this by any chance or are willing to reworke the patch some and profile if a hash table is faster?

>+    }
>+#endif
>   
>   G_UNLOCK (type_init_lock);
> }

Tim, thanks for reviewing.  I'll clarify some of my rationale, though I'm willing to rework any aspect of the patch as necessary.

I used a GTree in order to easily display the type names alphabetically at program termination, since one may only be interested in types from a particular library or application.  Alphabetized type names tend to be grouped this way due to our naming convention.

I agree that a GHashTable is likely faster for lookups and insertions.  But this being a debugging feature that has to be explicitly turned on, performance was not my highest priority.  Keeping the additional code simple and unobtrusive was.

With respect to formatting, I expect the output would likely be post-processed.  So I was trying to format the output in such a way that it could easily be parsed or piped to the appropriate tools, though I'm not sure my formatting choices are optimal for that purpose.  Admittedly the output looks a bit raw.  The "stale GObject" output appears to be more parsable than readable, and is also unsorted. That served as a precedent.

That said, I'll revise the patch with your suggestions.

(In reply to comment #5)
> I used a GTree in order to easily display the type names alphabetically at
> program termination, since one may only be interested in types from a
> particular library or application.  Alphabetized type names tend to be grouped
> this way due to our naming convention.

oh, very good point, that didn't occour to me.

> With respect to formatting, I expect the output would likely be post-processed.

you can make me shut up about hashtable/sorting if you also attach something like e.g. a python script for post-processing. that'd ideally produce something like the output i suggested above ;)

> Admittedly the output looks a bit raw. 
> The "stale GObject" output appears to be more parsable than readable, and is
> also unsorted. That served as a precedent.

i pled guilty for committing the output formatting of the stale object output. i still think the output i suggested above (maybe after post-processing) would make the result presentaiton of your stats much more useful for users.

Created attachment 83710 [details] [review]
Revised patch

Revised patch with suggested documentation changes.

Created attachment 83711 [details] [review]
Post-processing script

Post-processing script reads instance statistics from stdin and prints a report similar to what Tim suggested in comment #4.  The script is pretty bare bones; it has no command-line options and poor error handling.

Tim: ping, are there any other details you'd like me to rework?

hm, i'm a lazy bumm and was hoping for the script to improve a bit by waiting ;)
do you think you can clean it up a bit (e.g. have help, get a manual page) so it's good enough to be included in the glib tarball and installed under /usr/bin// ?

Ah, I didn't realize that was the intent.

I can work on making the Python script production quality.  But would packaging it with GLib introduce a dependency on Python?  Should I first rewrite the script as a C program?

(In reply to comment #11)
> Ah, I didn't realize that was the intent.
> 
> I can work on making the Python script production quality.  But would packaging
> it with GLib introduce a dependency on Python?

strictly speaking, yes.

>  Should I first rewrite the
> script as a C program?

no. we already "depend" on perl by installing perl scripts as build tools (glib-mkenum). i think the script should just be held reasonably portable in python (i.e. support python2.4) and the manual page should say that you need python for this. that's good enough then.

the reason to not have this in C is that it should be easily adaptable in the future and string (post-)processing is *much* simpler in python than in C. also, for an occasioanlly used devel-phase post processing script, C is simply not worth the effort.

(In reply to comment #12)
> the reason to not have this in C is that it should be easily adaptable in the
> future and string (post-)processing is *much* simpler in python than in C.
> also, for an occasioanlly used devel-phase post processing script, C is simply
> not worth the effort.

Definitely agree (GRegex notwithstanding).  Thanks for clarifying.

(In reply to comment #11)
> Ah, I didn't realize that was the intent.
> 
> I can work on making the Python script production quality.

(In reply to comment #13)
> (In reply to comment #12)
> > the reason to not have this in C is that it should be easily adaptable in the
> > future and string (post-)processing is *much* simpler in python than in C.
> 
> Definitely agree (GRegex notwithstanding).  Thanks for clarifying.

ok, for the record, i'm still waiting on the production quality python script and manual page then ;)

Right, still trying to find time.  Perhaps this weekend...

Created attachment 90227 [details] [review]
Patch including Python script

Here's a revised patch that includes a first cut at a production quality Python script (gobjectdebugtypes) to post process the debug messages.

$ gobjectdebugtypes --help
Usage: gobjectdebugtypes [options] [FILE...]

Generate a nicely formatted report from the debugging messages triggered by
setting GObject's G_TYPE_DEBUG_TYPES debug flag.  See the GTypeDebugFlags
section of the GObject Reference Manual to learn how to set this flag.

Options:
  -h, --help            show this help message and exit
  -s COL, --sort-ascending=COL
                        sort column COL in ascending order, where COL is one
                        of "type", "population", "created", or "destroyed"
  -S COL, --sort-descending=COL
                        sort column COL in descending order, where COL is one
                        of "type", "population", "created", or "destroyed"


If you don't provide a filename argument, the script reads from stdin.

You can build the sorting criteria by giving multiple sort options.  The default sorting behavior follows your original suggestion, and is equivalent to invoking:

   $ gobjectdebugtypes -S population -S created -s type

Can you think of any other options that would be useful?

Forgot to mention that I'm still working on a manual page, but I wanted to get your thoughts on the script first.  I'll post another patch when it's done.

Created attachment 95860 [details]
Manual page (DocBook)

Here's a first cut at a long overdue manual page for the Python utility.  Despite my best efforts, docbook2man chokes on it and I can't make sense of the gibbish it spews:

Using catalogs: /etc/sgml/sgml-docbook-3.0-1.0-31.cat
Using stylesheet: /usr/share/sgml/docbook/utils-0.6.14/docbook-utils.dsl#print
Working on: /home/mbarnes/svn/work/gobject-debug/glib/docs/reference/gobject/gobjectdebugtypes.xml
nsgmls:/home/mbarnes/svn/work/gobject-debug/glib/docs/reference/gobject/gobjectdebugtypes.xml:1:0:E: prolog can't be omitted unless CONCUR NO and LINK EXPLICIT NO and either IMPLYDEF ELEMENT YES or IMPLYDEF DOCTYPE YES
nsgmls:/home/mbarnes/svn/work/gobject-debug/glib/docs/reference/gobject/gobjectdebugtypes.xml:1:0:E: no document type declaration; will parse without validation
Can't call method "value" on an undefined value at /usr/share/sgml/docbook/utils-0.6.14/helpers/docbook2man-spec.pl line 249, <STDIN> line 2.

Tim, did you ever get a chance to look at the Python script?

Created attachment 98837 [details] [review]
Revised patch

This patch contains everything discussed above:

  - GObject source code changes
  - Report generation utility (gobject-stats)
  - Manual page (both docbook and generated troff)
  - Gtk-Doc updates

Let me know if you spot any outstanding issues.

I had occasion to wish for this feature again recently as I was debugging some reference counting leaks in Evolution.

Tim, ping.

Matthias, any interest in picking this up for GLib 2.17?

I never got any feedback on the Python script that Tim asked for.

> I never got any feedback on the Python script that Tim asked for.

Sorry, it's still in my queue though ;)

This feature is so useful.  Can we get it committed in whatever state it is please?

Humm, Matthew, the patch seems to be missing the python script.

Just want to add that the output is extremely useful even without any post-processing script.

(In reply to comment #25)
> Humm, Matthew, the patch seems to be missing the python script.

Sorry about that, trying now to pull it off a dying laptop.

Patch in comment #16 contains a mostly up-to-date version of the script under a different filename.  I had since renamed it to 'gobject-stats'.

Created attachment 125648 [details] [review]
Revised patch

Patch against current SVN trunk.  Includes the Python script this time.

(In reply to comment #28)
> Patch against current SVN trunk.  Includes the Python script this time.

Grrr, but now I left out the SGML files for the API docs.
Those are available in the previous patch.

Can we get this committed please?

Hello bugzilla CC list,

I needed to debug some GNOME Shell memory usage and was pointed at this patch, and I found it *very* useful.  However, the data it gave made me want more data.

There are a few things I want to improve with this:

1) Specifically for the shell, the environment variable approach is a bit painful because it gets inherited (well, I suppose we could unset it), and a flag for g_type_init is hard because we're a plugin

2) I want the data at multiple points in the program, not just at exit.  For example, how does the just-started state compare to at exit?

3) I really, really want backtraces.

To solve 1) and 2), I rewrote this design as an LD_PRELOADable library.  It temporarily lives here:

http://gitorious.org/gobject-memtrace/gobject-memtrace/commits/master

I'd like to rewrite this as a glib patch; the basic idea is that we'd have:

/usr/bin/gobject-memtrace
/usr/lib64/glib/libgobject-memtrace.so

Where gobject-memtrace is a small shell script which just sets up LD_PRELOAD and probably sets up print-on-exit by default.


What I'm going to work on now is 3), backtraces sorted by frequency.

Hmm, if it's going into GLib, I dont' think it should be a preload. LD_PRELOAD is inherently unreliable (especially with hiding of symbols from the PLT, etc) and specific to particular platforms.

GLib can unset the variable after reading it at GObject initialization if you want to avoid inheritance.

And doing it in GLib is going to make it much easier to add g_object_dump_type_statistics() hooks that can be run by an application. I don't actually see how writing a LD_PRELOAD library solves 2).

A separate wrapper program that sets an enviroment variable (maybe pointing to an extra FD to avoid overloading stdout) collects the backtraces and saves them nicely formatted might still make sense.

(In reply to comment #32)
> Hmm, if it's going into GLib, I dont' think it should be a preload. LD_PRELOAD
> is inherently unreliable (especially with hiding of symbols from the PLT, etc)
> and specific to particular platforms.

Well, we can also add specific hooks.

> GLib can unset the variable after reading it at GObject initialization if you
> want to avoid inheritance.
> 
> And doing it in GLib is going to make it much easier to add
> g_object_dump_type_statistics() hooks that can be run by an application. I
> don't actually see how writing a LD_PRELOAD library solves 2).

The way I saw this working was an application author wants control over access to the data during application runtime (say in shell, do "lg", "global.dump_perf_data()") they would sets up their launching script to run "gobject-memtrace myapp", then inside their C do do dlsym(NULL, "gobject_memtrace_dump") and call it.

This is a bit more painful for app authors, but on the other hand we avoid adding API to glib.  The other minor point is that gobject_memtrace_dump takes a GOutputStream, which would be hard to do in gobject.

> A separate wrapper program that sets an enviroment variable (maybe pointing to
> an extra FD to avoid overloading stdout) collects the backtraces and saves them
> nicely formatted might still make sense.

Hm, the wrapper program would open a fd itself or something, make sure it's not cloexec, then exec the app?   That won't work on Windows either...

Certainly I don't know what the right way to do backtraces on Windows/OS X are, and for that matter nor the BSDs.  So if the constraint is that all debugging tools in GLib need to be fully crossplatform, then yes, that would be a definite blocker.

Colin, also see cairo/util/malloc-stats.c

Re backtraces, see glib/glib/gbacktrace.c

I think I came to the conclusion that adding debugging APIs to glib is bad, because they are almost never good enough to solve the problem that you are currently looking at.

It's almost always preferrable to use other means, because those aren't affected by API stability and don't need to be maintained if it turns out that they are not useful (Anyone still doing "free (malloc (4))" in main()?).

The same goes for bug 588139.

Colin, did you mean to up the importance of the bug?

(In reply to comment #35)
> Colin, also see cairo/util/malloc-stats.c

Interesting.  Sort of similar to what I want to do; GLib could provide a __gobject_allocation_hook, and then I could write arbitrary code using that.

> Re backtraces, see glib/glib/gbacktrace.c

Ah, uses gdb.  A bit more crossplatform, however it would be hideously slow for what I want.  Though it would also likely be more accurate than what libc's backtrace_symbols() does.  Hmm...maybe that could be reworked into a persistent "slave gdb process" mode.

(In reply to comment #36)
> I think I came to the conclusion that adding debugging APIs to glib is bad,
> because they are almost never good enough to solve the problem that you are
> currently looking at.

Yeah, there are a variety of concerns; my main one is that dumping to stdout is just not good enough.

> Colin, did you mean to up the importance of the bug?

I accidentally hit the "bad_stacktrace" button and that had all sorts of weird effects.

My experience with malloc-stat.c is that even backtrace_symbols() is really slow.

(In reply to comment #38)
> My experience with malloc-stat.c is that even backtrace_symbols() is really
> slow.

I'm using just backtrace() in the fast path, and only converting with backtrace_symbols when printing.  Surely backtrace() is relatively fast, it just walks the stack, doesn't need to do any ELF processing.  At least I assume that's the reason for the separation.

Ok, new code landed in gobject-memtrace.  Sample output from:

$ env GMEMTRACE_ATEXIT=1 LD_PRELOAD=./gobject-memtrace.so gedit

FileInfo | created 61 | destroyed 61
  (27 stack traces below threshold 20 omitted)

GtkSourceStyle | created 430 | destroyed 0
  Stack: 21 hits
    ./gobject-memtrace.so [0x7fe6e5a9e171]
    ./gobject-memtrace.so(g_object_new+0xec) [0x7fe6e5a9e388]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_copy+0x1e) [0x3c09e23f5e]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_scheme_get_style+0x131) [0x3c09e25ec1]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e26479]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e24c98]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_scheme_manager_get_scheme+0x65) [0x3c09e24fa5]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e10992]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0xe8) [0x3c06e2ba58]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e10127]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(gedit_document_new+0x32) [0x430d22]
    gedit [0x4436fa]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(_gedit_tab_new+0x1a) [0x442f8a]
    gedit(gedit_window_create_tab+0x7d) [0x44e07d]
    gedit(main+0x55e) [0x4284de]
    /lib64/libc.so.6(__libc_start_main+0xfd) [0x3fa721ea2d]
    gedit [0x427849]
  Stack: 194 hits
    ./gobject-memtrace.so [0x7fe6e5a9e171]
    ./gobject-memtrace.so(g_object_new+0xec) [0x7fe6e5a9e388]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e2690e]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e24c98]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_scheme_manager_get_scheme+0x65) [0x3c09e24fa5]
    gedit [0x431f57]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e10127]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(gedit_document_new+0x32) [0x430d22]
    gedit [0x4436fa]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(_gedit_tab_new+0x1a) [0x442f8a]
    gedit(gedit_window_create_tab+0x7d) [0x44e07d]
    gedit(main+0x55e) [0x4284de]
    /lib64/libc.so.6(__libc_start_main+0xfd) [0x3fa721ea2d]
    gedit [0x427849]
  Stack: 194 hits
    ./gobject-memtrace.so [0x7fe6e5a9e171]
    ./gobject-memtrace.so(g_object_new+0xec) [0x7fe6e5a9e388]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e2690e]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e24c98]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_scheme_manager_get_scheme+0x65) [0x3c09e24fa5]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e10992]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0xe8) [0x3c06e2ba58]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e10127]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(gedit_document_new+0x32) [0x430d22]
    gedit [0x4436fa]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(_gedit_tab_new+0x1a) [0x442f8a]
    gedit(gedit_window_create_tab+0x7d) [0x44e07d]
    gedit(main+0x55e) [0x4284de]
    /lib64/libc.so.6(__libc_start_main+0xfd) [0x3fa721ea2d]
    gedit [0x427849]
  Stack: 21 hits
    ./gobject-memtrace.so [0x7fe6e5a9e171]
    ./gobject-memtrace.so(g_object_new+0xec) [0x7fe6e5a9e388]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_copy+0x1e) [0x3c09e23f5e]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_scheme_get_style+0x131) [0x3c09e25ec1]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e26479]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e24c98]
    /usr/lib64/libgtksourceview-2.0.so.0(gtk_source_style_scheme_manager_get_scheme+0x65) [0x3c09e24fa5]
    gedit [0x431f57]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /usr/lib64/libgtksourceview-2.0.so.0 [0x3c09e10127]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(gedit_document_new+0x32) [0x430d22]
    gedit [0x4436fa]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit(_gedit_tab_new+0x1a) [0x442f8a]
    gedit(gedit_window_create_tab+0x7d) [0x44e07d]
    gedit(main+0x55e) [0x4284de]
    /lib64/libc.so.6(__libc_start_main+0xfd) [0x3fa721ea2d]
    gedit [0x427849]

GdkWindow | created 39 | destroyed 0
  (23 stack traces below threshold 20 omitted)

GtkCheckMenuItem | created 78 | destroyed 77
  Stack: 67 hits
    ./gobject-memtrace.so [0x7fe6e5a9e171]
    ./gobject-memtrace.so(g_object_new+0xec) [0x7fe6e5a9e388]
    /usr/lib64/libgtk-x11-2.0.so.0(gtk_action_create_menu_item+0x7c) [0x3c08e7034c]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c09061151]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c0905fead]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c0905fead]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c0905fead]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c0905fead]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c0905fead]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c0905fead]
    /usr/lib64/libgtk-x11-2.0.so.0(gtk_ui_manager_ensure_update+0x31) [0x3c090611e1]
    /usr/lib64/libgtk-x11-2.0.so.0 [0x3c09063ca9]
    gedit [0x450349]
    gedit [0x45119a]
    /lib64/libgobject-2.0.so.0(g_type_create_instance+0x137) [0x3c06e2baa7]
    /lib64/libgobject-2.0.so.0 [0x3c06e10a0c]
    /lib64/libgobject-2.0.so.0(g_object_newv+0x291) [0x3c06e11a01]
    /lib64/libgobject-2.0.so.0(g_object_new_valist+0x345) [0x3c06e12555]
    ./gobject-memtrace.so(g_object_new+0xd6) [0x7fe6e5a9e372]
    gedit [0x429661]
    gedit(gedit_app_create_window+0x1d) [0x42992d]
    gedit(main+0x379) [0x4282f9]
    /lib64/libc.so.6(__libc_start_main+0xfd) [0x3fa721ea2d]
    gedit [0x427849]
  (5 stack traces below threshold 20 omitted)

(In reply to comment #37)
> (In reply to comment #36)
> > Colin, did you mean to up the importance of the bug?
> 
> I accidentally hit the "bad_stacktrace" button and that had all sorts of weird
> effects.

Resetting the priority to "Normal" then.

(In reply to comment #32)
> Hmm, if it's going into GLib, I dont' think it should be a preload. LD_PRELOAD
> is inherently unreliable (especially with hiding of symbols from the PLT, etc)
> and specific to particular platforms.

> A separate wrapper program that sets an enviroment variable (maybe pointing to
> an extra FD to avoid overloading stdout) collects the backtraces and saves them
> nicely formatted might still make sense.

I have to agree with Owen here, LD_PRELOAD is quite fragile. I'd rather see something like support for a debugging file descriptor, e.g. G_DEBUG_INFO_FD which produces machine parsable debugging information *and* uses and extensible format for any future debugging info we might come up with. SO we could use, e.g.:
  G_DEBUG_INFO_FD=122 SomeGLibProgram 122>debug.info
  glibdebuganalyze debug.info

As far as I'm concerned this type of thing is obsoleted by

https://bugzilla.gnome.org/show_bug.cgi?id=606044

So I won't be updating my patch.

I'm sure that's really sexy, but there are command lines and sample output here for mbarnes' stuff, while I have no idea at all how to use the stuff in bug 606044. Is there documentation anywhere? All I want is the simple stats that Matt's patch gave us...

Created attachment 183030 [details] [review]
Original patch, updated for GLib 2.28

I know this is never going to land because the systemtap way is better, but I still find my original patch simple and useful when I want a quick overview of how badly object references are leaking.  So as a public service here's an updated patch for GLib 2.28.

To use:

  1. GOBJECT_DEBUG=types $MY_APPLICATION 2>&1 | gobject-stats
  2. Do stuff in your app, then quit.
  3. Read the GType stats.

Created attachment 183031 [details] [review]
Enhanced patch for GLib 2.28

Actually, long as I'm hacking on this again, here's an enhanced version that adds a "# Leaked" column to the gobject-stats output and sorts on it by default in descending order.

See https://bugzilla.gnome.org/show_bug.cgi?id=614619 for a sample script using SystemTap on glib, GObject, and gjs.

Created attachment 183544 [details]
glib memtrace systemtap script

So in the big picture, I think it's worth considering having a separate --enable-deveoper-mode mode for GLib which changes a bunch of things (e.g. turns off -Bsymbolic so you can LD_PRELOAD, and enables more assertions, etc.)  Think of this as moving GLib a bit more towards "kernel style" where you can recompile with things like lockdep.

However, we should still have a hard set of defaults that we expect OS builders to use.  Practically speaking in a "distro" then the above might manifest as a glib2-debugging package which had a separate .so.

In that scenario it would then sense to me to land something like Matt's patch in --enable-developer-mode.

(In reply to comment #49)
> So in the big picture, I think it's worth considering having a separate
> --enable-deveoper-mode mode for GLib which changes a bunch of things.
>
The only thing that leads to is frustration. For two reasons:
1) People cannot run the script you gave them because their computer doesn't have the developer mode version available.
2) People cannot reproduce bugs because they only manifest in one of the versions.

So I'm very strongly in favor of not doing something like this.

Created attachment 217133 [details] [review]
Updated patch for GLib 2.33

Had occasion to use this again.  It helped me easily spot a huge memory leak in Camel.

Here's an updated patch for those who may still find this useful.  Applies against the latest git revision (GLib 2.33.2).

Created attachment 288174 [details] [review]
Add simple instance count facility

Here's a quick patch along the same lines I've been using for some gnome-shell
leak detection. There's no collection, reporting, max, total, etc here - 
just a count of currently live objects available as g_type_get_instance_count().

From the GNOME Shell perspective, what I wanted to be able to do do was have
a hot key that:

 - garbage collected
 - dumped the current counts to a file

The differential between the counts before and after an activity reveals leaks.

As an implementation technique I put the counts right in the GTypeNode; it would
be also possible to put a pointer in the GTypeNode to a block if we were keeping
more sophisticated stats.

Having the statistics available at the app level definitely allows for
interesting possibilities and should be part of anything like this that is landed;
it's also something that you *can't* at all easily do with systemtap. Other
questions/thoughts that come to mind examining the bug history:

 * Can we do something cheap enough to be always on? It would definitely be cool
   if I could ask someone using GNOME Shell who has a bunch of memory allocated to
   go to the looking glass console and see what sort of objects are allocated without
   a need to restart in a special mode.

 * How useful are the high-water-mark and total-instances-allocated stats compared
   to the live object stats?

 * It definitely seems to me like that shouldn't be dependent on --enable-debug - 
   at most a runtime switch - a few well-predicted branches per type instance creation
   and freeing will have almost no effect, and having a useful facility for app-authors
   out of the box would be very appreciated.

(In reply to comment #52)
>  * How useful are the high-water-mark and total-instances-allocated stats
>    compared to the live object stats?

I vaguely recall at the time I suggested this I was trying to track the source of sporadic CPU or memory usage spikes in Evolution where objects were being created and destroyed very rapidly.  The postmortem stats did help on rare occasions, but I don't know if it's worth keeping.

(In reply to comment #52)
> Created an attachment (id=288174) [details] [review]
> Add simple instance count facility
> 
> Here's a quick patch along the same lines I've been using for some gnome-shell
> leak detection. There's no collection, reporting, max, total, etc here - 
> just a count of currently live objects available as
> g_type_get_instance_count().
> 
> From the GNOME Shell perspective, what I wanted to be able to do do was have
> a hot key that:
> 
>  - garbage collected
>  - dumped the current counts to a file
> 
> The differential between the counts before and after an activity reveals leaks.
> 
> As an implementation technique I put the counts right in the GTypeNode; it
> would
> be also possible to put a pointer in the GTypeNode to a block if we were
> keeping
> more sophisticated stats.
> 
> Having the statistics available at the app level definitely allows for
> interesting possibilities and should be part of anything like this that is
> landed;
> it's also something that you *can't* at all easily do with systemtap. Other
> questions/thoughts that come to mind examining the bug history:
> 
>  * Can we do something cheap enough to be always on? It would definitely be
> cool
>    if I could ask someone using GNOME Shell who has a bunch of memory allocated
> to
>    go to the looking glass console and see what sort of objects are allocated
> without
>    a need to restart in a special mode.

I would love to have something like this available in GtkInspector too.

Review of attachment 288174 [details] [review]:

::: gobject/gtype.c
@@ +3836,3 @@
+ *
+ * Returns: the number of instances allocated of the given type;
+ *  if the debug flag is not set or the instance is not instantiable,

s/instance/type/ here

@@ +4379,3 @@
       GDebugKey debug_keys[] = {
         { "objects", G_TYPE_DEBUG_OBJECTS },
+        { "instance_count", G_TYPE_DEBUG_INSTANCE_COUNT },

For the inspector, this would be much more useful if it could be on by default. We can still show the information of course, if the environment variable happens to be set.

Review of attachment 288174 [details] [review]:

::: gobject/gtype.c
@@ +3847,3 @@
+  g_return_if_fail (node != NULL);
+
+  return g_atomic_int_get (&node->instance_count);

This needs an ifdef

An inspector patch using this is in bug 738272

Review of attachment 288174 [details] [review]:

::: gobject/gtype.c
@@ +229,3 @@
+#ifdef G_ENABLE_DEBUG
+  guint volatile instance_count;
+#endif

Would be nice to have a static assertion that this doesn't affect struct layout, but just fills a hole.

Also, once we have asserted that, we could just do away with the ifdef here ?

(In reply to comment #58)
> Would be nice to have a static assertion that this doesn't affect struct
> layout, but just fills a hole.

It only fills a hole on 64-bit architectures. On 32-bit it does affect struct layout.

(In reply to comment #59)
> (In reply to comment #58)
> > Would be nice to have a static assertion that this doesn't affect struct
> > layout, but just fills a hole.
> 
> It only fills a hole on 64-bit architectures. On 32-bit it does affect struct
> layout.

I guess one answer would be to only do it on 64-bit, then.

Created attachment 288254 [details] [review]
Add simple instance count facility

Add GOBJECT_DEBUG=instance_count which enables internal accounting
of the number of instances of each GType, and g_type_get_instance_count()
to retrieve the result. We only support this on architectures where
we can fit an integer into the type node struct without changing ABI.

Here is an iteration of Owens patch that adds a missing ifdef and restricts the functionality to x86-64.

Created attachment 288255 [details] [review]
Make instance counting unconditional

The overhead for maintaining this instance counter should
be small, and there is value in being able to always get
instance counts without having to restart the application.

I ended up going with Owens original patch, mostly. ABI is not actually
a concern for type nodes; they are purely internal. And I've kept the
runtime flag in place; if we convince ourselves that performance is not
affected, we can drop it later.
Attachment 288254 [details] pushed as 011bf87 - Add simple instance count facility