Created attachment 31940 [details] [review]
patch to speed up signal_connect by keeping a tail pointer

This patch is an improved version of the patch that I attached to bug #143668.
It keeps the overhead in handler_unref_R() to a minimum by only looking up the
handler-list if we actually need to change the tail pointer.

Created attachment 31941 [details] [review]
improved patch that keeps another tail pointer for connecting signals with CONNECT_AFTER

This patch adds another tail pointer that points to the tail of the list of
signal connections. signal handlers connected with CONNECT_AFTER can he
appeneded here directly. This will avoid to run into the same problem again if
someone tries to connect a lot of CONNECT_AFTER signal handlers.

Created attachment 31942 [details]
a simple benchmark

This is a very simple test application to get an impression of the performance
before and after the proposed changes. Here are the numbers I obtained using
this benchmark:


glib-2.4.6:

  connected 100 handlers (52 before, 48 after):  0.000159 seconds
  connected 1000 handlers (500 before, 500 after):  0.046655 seconds
  connected 10000 handlers (5069 before, 4931 after):  0.339064 seconds
  connected 100000 handlers (50165 before, 49835 after):  420.902 seconds


glib CVS patched:

  connected 100 handlers (52 before, 48 after):  0.000156 seconds
  connected 1000 handlers (500 before, 500 after):  0.048044 seconds
  connected 10000 handlers (5069 before, 4931 after):  0.061368 seconds
  connected 100000 handlers (50165 before, 49835 after):  0.145188 seconds

Created attachment 31945 [details]
slightly enhanced benchmark and test

Here's a slightly enhanced version of the benchmark. It disconnects the
handlers in random order. The results show that the disconnect performance is
not hurt by the proposed changes. Of course it also shows that as soon as a lot
of handlers are connected, disconnecting can also become a performance problem
:(

You aren't really doing the GIMP developers a favor if you keep pushing all bugs
to the 2.6.0 milestone that are critical to the GIMP 2.2 release. I hope you
simply aren't aware of this and will consider to apply this (and other changes
needed to resolve #143668) to the stable branch.

I'm not really pushing anything back here. The patch will not be applied until
timj finds time to review it, which can take months or years. 2.6 will likely be
already released by then. If tim finds time earlier, I'll be happy to apply it
to 2.4.x.

It would probably not hurt to have it on the next 2.4 milestone then.

Happy now ? :-)

But is this optimization worthwhile?

With 10000 handlers, you took 0.34 seconds, which is not a large amount of time.

How many handlers does the real problematic case have, and to which object?

See comment #21 in bug #143668. There are a *lot* of signal connections
to GtkSettings. Looks like this patch would speed up every GTK+ application
by a few percent. The speedup would even increase with the number of
widgets used.

Finding "the" optimization to speed up things is IMHO a futile approach,
it's the number of few-percent things that counts.

How many connections are there to GtkSettings?  If it is under 10000, then you
have bigger things to tackle first.  The profile in bug #143668 shows that you
spend 14 seconds in update_smart_separators(), for instance.

update_smart_separators() is the thing I would look at first, personally.

Each GIMP image window adds about 700 connections to GtkSettings and
it's not uncommon to have 10 or more windows open.

Sure, it makes sense to fix the major bottlenecks first, but this
(admitted minor) one has already been looked at, is tracked down,
benchmarked and has a patch that works.

For the records, I added the following piece of code to the bottom
of handler_insert():

  {
    static gint max_handlers = 0;

    Handler *h;
    gint     i = 0;

    for (h = hlist->handlers; h; h = h->next)
      i++;

    if (i > max_handlers)
      {
        max_handlers = i;

        g_print ("handler_insert: %d connections for %s\n",
                 max_handlers, G_OBJECT_TYPE_NAME (instance));
      }
  }

One thing we possibly should consider is whether we should be multiplexing
the connections to GtkSettings ... if all we need is a list of GtkMenuItem
(or whatever) and are doing the same callback on each of them, then having
the full GClosure/signal connection is quite a bit of overhead.

Created attachment 32798 [details] [review]
patch with a real-life benchmark for this patch

Attached patch changes gimp/app/display/gimpdisplayshell.c to create
20 instead of one UI manager for the menubar. It simply GTimer-measures
the time used and g_print()s it.

Created attachment 32799 [details] [review]
putput of above patch without sven's gsignal.c patch applied

Created attachment 32800 [details] [review]
putput of above patch with sven's gsignal.c patch applied

The figures in the last two attachments show clearly that with unpatched
gobject.c, the time to create a UI manager increases constantly and
considerably.

After the patch is applied the time stays almost constant and the overall
time to create the 20 managers drops to a third of the original time.

Heh... and make an "output" out of that "putput" in the patch descriptions ;)

I am raising priority on this bug in the hope that someone can finally get
around to review the proposed changes. We are getting close to a GIMP 2.2
release and this problem is definitely a showstopper for us.

the patch is in CVS HEAD now (plus an additional commant). i'd recommend to give
it some testing in HEAD first before moving it into 2.4. in principle, it should
be save to merge it down to 2.4 though.

I merged this down after testing it for a while.