GIMP 2.3 uses a rather large number of tooltips for each image window.  Opening 50 image windows takes between 1 and 2 minutes depending on the machine used.  The time taken to open these windows increases as the total number of windows gets larger.  Closing these 50 windows takes another 2 minutes and it is clearly visible that they get closed at a faster rate as the total number of windows decreases.  These times are excessive and smell like side-effects of the usage of linear lists.

Created attachment 89904 [details] [review]
use a hash-table instead of the linked list

This patch changes GtkTooltips to use a hash-table instead of a linked list. I haven't actually compiled this code so there are likely a few problems. But the patch shows that it is possible to change the code in an API compatible way. Except of course someone actually used the public GList.

Created attachment 89910 [details] [review]
updated patch

In the meantime I compiled and tested the patch. Attached is a version that compiles cleanly.

Without the patch, closing 50 image windows takes 61 seconds on my box. With the patch applied, this time goes down to 15 seconds.

I request approval to commit this patch to trunk and also to backport it to the gtk-2-10 branch.

Created attachment 89913 [details] [review]
fixes gtk_tooltips_destroy

This update fixes a problem in gtk_tooltips_destroy(). It no longer attempts to remove entries from the hash table in g_hash_table_foreach(). Instead all values are first destroyed, then removed using g_hash_table_remove_all().

(In reply to comment #4)
> I request approval to commit this patch to trunk and also to backport it to the
> gtk-2-10 branch.

i don't think backporting this is a good idea, since people can really use the tips-data list at the moment and that's something that shouldn't be broken in the middle of a stable branch.
for trunk/2.12, however we can simply add /*< private >*/ to the tooltips structure and then fidle with its fields. maybe the current tooltips.h code is going to be deprecated alltogether in favor of the new tooltip.h code. Kris, any word on deprecation? 

Comment on attachment 89913 [details] [review]
fixes gtk_tooltips_destroy

while the patch in general will be good to have in trunk, i have a couple nit picks/improvements for it:

>Index: gtk/gtktooltips.c
>===================================================================
>--- gtk/gtktooltips.c	(revision 18125)
>+++ gtk/gtktooltips.c	(working copy)
>@@ -48,8 +48,18 @@
> #define STICKY_REVERT_DELAY 1000    /* Delay before sticky tooltips revert
> 				     * to normal
>                                      */
>+#define GTK_TOOLTIPS_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE ((obj), GTK_TYPE_TOOLTIPS, GtkTooltipsPrivate))
> 
>-static void gtk_tooltips_destroy           (GtkObject        *object);
>+typedef struct _GtkTooltipsPrivate GtkTooltipsPrivate;
>+
>+struct _GtkTooltipsPrivate
>+{
>+  GHashTable *tips_data_table;
>+};
>+
>+
>+static void gtk_tooltips_finalize          (GObject     *object);
>+static void gtk_tooltips_destroy           (GtkObject   *object);
> 
> static void gtk_tooltips_event_handler     (GtkWidget   *widget,
>                                             GdkEvent    *event);
>@@ -75,26 +85,41 @@ G_DEFINE_TYPE (GtkTooltips, gtk_tooltips
> static void
> gtk_tooltips_class_init (GtkTooltipsClass *class)
> {
>-  GtkObjectClass *object_class;
>+  GtkObjectClass *object_class = (GtkObjectClass *) class;
>+  GObjectClass *gobject_class = (GObjectClass *) class;
> 
>-  object_class = (GtkObjectClass*) class;
>+  gobject_class->finalize = gtk_tooltips_finalize;
> 
>   object_class->destroy = gtk_tooltips_destroy;
>+
>+  g_type_class_add_private (gobject_class, sizeof (GtkTooltipsPrivate));
> }
> 
> static void
> gtk_tooltips_init (GtkTooltips *tooltips)
> {
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
>+
>   tooltips->tip_window = NULL;
>   tooltips->active_tips_data = NULL;
>-  tooltips->tips_data_list = NULL;
>-  
>+  tooltips->tips_data_list = NULL;  /* unused */

this comment should be moved to the header file, and the field name should change to _tips_data_list or similar, to break source compatibility for applications that actually use this field.

>+
>   tooltips->delay = DEFAULT_DELAY;
>   tooltips->enabled = TRUE;
>   tooltips->timer_tag = 0;
>   tooltips->use_sticky_delay = FALSE;
>   tooltips->last_popdown.tv_sec = -1;
>   tooltips->last_popdown.tv_usec = -1;
>+
>+  private->tips_data_table = g_hash_table_new (g_direct_hash, g_direct_equal);

using NULL/NULL here is faster than g_direct_*

>+}
>+
>+static void
>+gtk_tooltips_finalize (GObject *object)
>+{
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (object);
>+

users can conceivably add tooltips between destroy and finalize, so you should either use g_hash_table_foreach_remove here as well, or install destroy handlers on the hash table.

>+  g_hash_table_destroy (private->tips_data_table);
> }
> 
> GtkTooltips *
>@@ -153,11 +178,18 @@ gtk_tooltips_unset_tip_window (GtkToolti
> }
> 
> static void
>+gtk_tooltips_table_destroy_data (GtkWidget       *widget,
>+                                 GtkTooltipsData *tooltipsdata)
>+{
>+  gtk_tooltips_widget_unmap (widget, tooltipsdata);
>+  gtk_tooltips_destroy_data (tooltipsdata);
>+}
>+
>+static void
> gtk_tooltips_destroy (GtkObject *object)
> {
>   GtkTooltips *tooltips = GTK_TOOLTIPS (object);
>-  GList *current;
>-  GtkTooltipsData *tooltipsdata;
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
> 
>   g_return_if_fail (tooltips != NULL);
> 
>@@ -167,16 +199,9 @@ gtk_tooltips_destroy (GtkObject *object)
>       tooltips->timer_tag = 0;
>     }
> 
>-  if (tooltips->tips_data_list != NULL)
>-    {
>-      current = g_list_first (tooltips->tips_data_list);
>-      while (current != NULL)
>-	{
>-	  tooltipsdata = (GtkTooltipsData*) current->data;
>-	  current = current->next;
>-	  gtk_tooltips_widget_remove (tooltipsdata->widget, tooltipsdata);
>-	}
>-    }
>+  g_hash_table_foreach (private->tips_data_table,
>+                        (GHFunc) gtk_tooltips_table_destroy_data, NULL);
>+  g_hash_table_remove_all (private->tips_data_table);

this is easier written with g_hash_table_foreach_remove (only one pass for destruction) or by adding value/key destroy handlers to the hash table.

> 
>   gtk_tooltips_unset_tip_window (tooltips);
> 

>@@ -728,10 +746,12 @@ gtk_tooltips_widget_remove (GtkWidget *w
> {
>   GtkTooltipsData *tooltipsdata = (GtkTooltipsData*) data;
>   GtkTooltips *tooltips = tooltipsdata->tooltips;
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
> 
>   gtk_tooltips_widget_unmap (widget, data);
>-  tooltips->tips_data_list = g_list_remove (tooltips->tips_data_list,
>-					    tooltipsdata);
>+
>+  g_hash_table_remove (private->tips_data_table, tooltipsdata->widget);
>+
>   gtk_tooltips_destroy_data (tooltipsdata);

this gtk_tooltips_destroy_data() call can also be omitted by using a destroy handler.

> }

If it is OK to change the field name, then the pointer to the hash table could as well take the place of the tips_data_list field. Or do you think it's better to keep it entirely private?

(In reply to comment #8)
> If it is OK to change the field name, then the pointer to the hash table could
> as well take the place of the tips_data_list field. Or do you think it's better
> to keep it entirely private?

no it couldn't. changing the field name breaks source compat, which is good because people get a chance to fix their code that way. using it for a hash table would also break ABI, in that old applications would try to walk a hashtable structure as list and that'd be bad. using your approach where the hash table is private, and the list is NULL, we actually do preserve ABI, because NULL is a valid list.

Whatever you end up doing, please make sure you mention it in the release notes (thats the 2.12 section in README.in)

(In reply to comment #6)
> for trunk/2.12, however we can simply add /*< private >*/ to the tooltips
> structure and then fidle with its fields. maybe the current tooltips.h code is
> going to be deprecated alltogether in favor of the new tooltip.h code. Kris,
> any word on deprecation? 

I guess the plan is to deprecate all of GtkTooltips in 2.12. 

(As a side note, can we really break source compat in 2.x???)

(In reply to comment #11)
> I guess the plan is to deprecate all of GtkTooltips in 2.12. 
> 
> (As a side note, can we really break source compat in 2.x???)

we do that all the time when we deprecate something ;)
also subtler changes that force people to add/remove signal handlers or use new properties etc. are actually source incompatibilities, most often they unfortunately can't be caught by the compiler though. in this case, we have the ability to let the compiler verify that code was correctly ported though, so we should use it (making the change non-subtle). 
breaking source compat within a branch like 2.10.x would be bad though, people shouldn't need to "port" their code within a stable branch.

Created attachment 90187 [details] [review]
updated patch

This patch folds the call to gtk_tooltips_widget_unmap() into gtk_tooltips_destroy_data() and uses the latter as the destroy function for the hash table. It also marks the GtkTooltips struct as private and changes the tips_data_list field to _tips_data_list. The changes are documented in README.in.

Comment on attachment 90187 [details] [review]
updated patch

>Index: gtk/gtktooltips.c
>===================================================================
>--- gtk/gtktooltips.c	(revision 18180)
>+++ gtk/gtktooltips.c	(working copy)
>@@ -75,26 +87,43 @@ G_DEFINE_TYPE (GtkTooltips, gtk_tooltips
> static void
> gtk_tooltips_class_init (GtkTooltipsClass *class)
> {
>-  GtkObjectClass *object_class;
>+  GtkObjectClass *object_class = (GtkObjectClass *) class;
>+  GObjectClass *gobject_class = (GObjectClass *) class;
> 
>-  object_class = (GtkObjectClass*) class;
>+  gobject_class->finalize = gtk_tooltips_finalize;
> 
>   object_class->destroy = gtk_tooltips_destroy;
>+
>+  g_type_class_add_private (gobject_class, sizeof (GtkTooltipsPrivate));
> }
> 
> static void
> gtk_tooltips_init (GtkTooltips *tooltips)
> {
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
>+
>   tooltips->tip_window = NULL;
>   tooltips->active_tips_data = NULL;
>-  tooltips->tips_data_list = NULL;
>-  
>+  tooltips->_tips_data_list = NULL;
>+
>   tooltips->delay = DEFAULT_DELAY;
>   tooltips->enabled = TRUE;
>   tooltips->timer_tag = 0;
>   tooltips->use_sticky_delay = FALSE;
>   tooltips->last_popdown.tv_sec = -1;
>   tooltips->last_popdown.tv_usec = -1;
>+
>+  private->tips_data_table =
>+    g_hash_table_new_full (NULL, NULL,
>+                           NULL, (GDestroyNotify) gtk_tooltips_destroy_data);

hm, this would probably better be wrapped as "NULL, NULL, NULL,\n"

>+}
>+
>+static void
>+gtk_tooltips_finalize (GObject *object)
>+{
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (object);
>+
>+  g_hash_table_destroy (private->tips_data_table);

you forgot to chain to the parent class here.

> }
> 
> GtkTooltips *
>@@ -106,6 +135,8 @@ gtk_tooltips_new (void)
> static void
> gtk_tooltips_destroy_data (GtkTooltipsData *tooltipsdata)
> {
>+  gtk_tooltips_widget_unmap (tooltipsdata->widget, tooltipsdata);
>+
>   g_free (tooltipsdata->tip_text);
>   g_free (tooltipsdata->tip_private);
> 
>@@ -156,8 +187,7 @@ static void
> gtk_tooltips_destroy (GtkObject *object)
> {
>   GtkTooltips *tooltips = GTK_TOOLTIPS (object);
>-  GList *current;
>-  GtkTooltipsData *tooltipsdata;
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
> 
>   g_return_if_fail (tooltips != NULL);
> 
>@@ -167,16 +197,7 @@ gtk_tooltips_destroy (GtkObject *object)
>       tooltips->timer_tag = 0;
>     }
> 
>-  if (tooltips->tips_data_list != NULL)
>-    {
>-      current = g_list_first (tooltips->tips_data_list);
>-      while (current != NULL)
>-	{
>-	  tooltipsdata = (GtkTooltipsData*) current->data;
>-	  current = current->next;
>-	  gtk_tooltips_widget_remove (tooltipsdata->widget, tooltipsdata);
>-	}
>-    }
>+  g_hash_table_remove_all (private->tips_data_table);
> 
>   gtk_tooltips_unset_tip_window (tooltips);
> 
>@@ -327,8 +348,9 @@ gtk_tooltips_set_tip (GtkTooltips *toolt
>       tooltipsdata->tip_text = g_strdup (tip_text);
>       tooltipsdata->tip_private = g_strdup (tip_private);
> 
>-      tooltips->tips_data_list = g_list_append (tooltips->tips_data_list,
>-                                                tooltipsdata);
>+      g_hash_table_insert (GTK_TOOLTIPS_GET_PRIVATE (tooltips)->tips_data_table,
>+                           widget, tooltipsdata);
>+
>       g_signal_connect_after (widget, "event_after",
>                               G_CALLBACK (gtk_tooltips_event_handler),
> 			      tooltipsdata);
>@@ -478,23 +500,15 @@ gtk_tooltips_set_active_widget (GtkToolt
>   
>   tooltips->active_tips_data = NULL;
>   
>-  if (widget)
>+  if (widget && GTK_WIDGET_DRAWABLE (widget))
>     {
>-      GList *list;
>-      
>-      for (list = tooltips->tips_data_list; list; list = list->next)
>-	{
>-	  GtkTooltipsData *tooltipsdata;
>-	  
>-	  tooltipsdata = list->data;
>-	  
>-	  if (tooltipsdata->widget == widget &&
>-	      GTK_WIDGET_DRAWABLE (widget))
>-	    {
>-	      tooltips->active_tips_data = tooltipsdata;
>-	      break;
>-	    }
>-	}
>+      GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
>+      GtkTooltipsData    *tooltipsdata;
>+
>+      tooltipsdata = g_hash_table_lookup (private->tips_data_table, widget);
>+
>+      if (tooltipsdata)
>+        tooltips->active_tips_data = tooltipsdata;
>     }
>   else
>     {
>@@ -728,11 +742,9 @@ gtk_tooltips_widget_remove (GtkWidget *w
> {
>   GtkTooltipsData *tooltipsdata = (GtkTooltipsData*) data;
>   GtkTooltips *tooltips = tooltipsdata->tooltips;
>+  GtkTooltipsPrivate *private = GTK_TOOLTIPS_GET_PRIVATE (tooltips);
> 
>-  gtk_tooltips_widget_unmap (widget, data);
>-  tooltips->tips_data_list = g_list_remove (tooltips->tips_data_list,
>-					    tooltipsdata);
>-  gtk_tooltips_destroy_data (tooltipsdata);
>+  g_hash_table_remove (private->tips_data_table, tooltipsdata->widget);
> }
> 
> void
>Index: gtk/gtktooltips.h

looks good.

>Index: README.in

thanks for adding the note here.
the patch can go in once the finalize chaining is fixed.

I've fixed the upchaining in the finalize method and committed the change to trunk. Closing as FIXED.

2007-06-19  Sven Neumann  <sven@gimp.org>

	* gtk/gtktooltips.[ch]: mark the GtkTooltips struct as private.
	Keep the tooltips in a hash table instead of a linked list.
	Improves performance when using large amounts of tooltips (#447214).

	* README.in: document the GtkTooltips changes.

since the deliberate field rename that was meant as an aid to developers to adapt their code seems to be received in mixed ways and turns out to be quite controversial, so i've talked/listened to kris/mclasen/sven about this and the deprecation and the new plan/suggestion now is to:
- deprecate all of gtktooltips.h ASAP in favour of GtkTooltip (Kris will handle that);
- revert the field rename before starting the next stable branch (i.e. around the upcoming API freeze), so developers testing the current devel branch can adapt their code, but the general build (distro) community upgrading to the next stable gtk branch won't be affected.

GtkTooltips has been deprecated. (r18241)

(In reply to comment #16)
> since the deliberate field rename that was meant as an aid to developers to
> adapt their code seems to be received in mixed ways and turns out to be quite
> controversial, so i've talked/listened to kris/mclasen/sven about this and the
> deprecation and the new plan/suggestion now is to:
> - deprecate all of gtktooltips.h ASAP in favour of GtkTooltip (Kris will handle
> that);
> - revert the field rename before starting the next stable branch (i.e. around
> the upcoming API freeze), so developers testing the current devel branch can
> adapt their code, but the general build (distro) community upgrading to the
> next stable gtk branch won't be affected.

Waiting to revert the rename is unfortunately not a good solution for the release team: this change seems to be a blocker for the GNOME 2.19.x releases (we couldn't release 2.19.4 because of this). So, from the GNOME point of view, the sooner this gets reverted, the better.

(In reply to comment #18)
> (In reply to comment #16)
> > - revert the field rename before starting the next stable branch (i.e. around
> > the upcoming API freeze), so developers testing the current devel branch can
> > adapt their code, but the general build (distro) community upgrading to the
> > next stable gtk branch won't be affected.
> 
> Waiting to revert the rename is unfortunately not a good solution for the
> release team: this change seems to be a blocker for the GNOME 2.19.x releases
> (we couldn't release 2.19.4 because of this). So, from the GNOME point of view,
> the sooner this gets reverted, the better.

oh, i was under the impression that a new pygtk release fixes this situation. is that assumption mistaken?

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > - revert the field rename before starting the next stable branch (i.e. around
> > > the upcoming API freeze), so developers testing the current devel branch can
> > > adapt their code, but the general build (distro) community upgrading to the
> > > next stable gtk branch won't be affected.
> > 
> > Waiting to revert the rename is unfortunately not a good solution for the
> > release team: this change seems to be a blocker for the GNOME 2.19.x releases
> > (we couldn't release 2.19.4 because of this). So, from the GNOME point of view,
> > the sooner this gets reverted, the better.
> 
> oh, i was under the impression that a new pygtk release fixes this situation.
> is that assumption mistaken?

Elijah will know, since he's the one handling the GNOME release. Cc'ing him.

No, the latest pygtk release is dated 05-Feb-2007, which obviously doesn't fix this.  They really need a good swift kick to start making more timely releases...  ;-)  So, yes, the 2.19.4 release is still on hold for this issue; we need a fix in a released tarball from either pygtk or gtk+ before we can move on.

(And actually, Tim's suggestion sounds reasonable to me, so I should probably concentrate my pestering on some pygtk person.  I believe they have a fix in SVN...)

Are we done here ? 

Any reason to keep this bug open ?

gtktooltips.h has been fully deprecated now, I think this bug should stay open until the rename has been undone.

Ok, lets rename the bug, then.

2007-07-21  Matthias Clasen  <mclasen@redhat.com>

        * gtk/gtktooltips.[hc]: Rename the tips_data_list field
        back.  (#447214)