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 447214 - rename the tips_data_list field back
rename the tips_data_list field back
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
2.10.x
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-06-13 16:42 UTC by Sven Neumann
Modified: 2011-02-04 16:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
use a hash-table instead of the linked list (4.86 KB, patch)
2007-06-13 19:38 UTC, Sven Neumann
none Details | Review
updated patch (4.86 KB, patch)
2007-06-13 21:13 UTC, Sven Neumann
none Details | Review
fixes gtk_tooltips_destroy (5.24 KB, patch)
2007-06-13 21:53 UTC, Sven Neumann
none Details | Review
updated patch (7.99 KB, patch)
2007-06-18 07:20 UTC, Sven Neumann
committed Details | Review

Description Sven Neumann 2007-06-13 16:42:28 UTC
We wondered why closing an image window in GIMP 2.3 became so slow. Sysprof shows us that roughly 2/3 of the time is spent in gtk_tooltips_widget_remove(). This function boils down to a call to g_list_remove(). Since there can be a lot of tooltips in an application, the use a linked list becomes a performance problem here.

Same holds true for gtk_tooltips_set_tip() as it uses g_list_append()! I don't have profiling data for this but I suspect that a similar amount of time is spent whenever an image window is opened.

This code could easily be changed to use a hash table instead of a linked list.
Comment 1 Raphaël Quinet 2007-06-13 16:57:09 UTC
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.
Comment 2 Sven Neumann 2007-06-13 19:38:11 UTC
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.
Comment 3 Sven Neumann 2007-06-13 21:13:14 UTC
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.
Comment 4 Sven Neumann 2007-06-13 21:21:55 UTC
I request approval to commit this patch to trunk and also to backport it to the gtk-2-10 branch.
Comment 5 Sven Neumann 2007-06-13 21:53:15 UTC
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().
Comment 6 Tim Janik 2007-06-14 09:47:58 UTC
(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 7 Tim Janik 2007-06-14 10:00:24 UTC
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.

> }
Comment 8 Sven Neumann 2007-06-14 10:48:55 UTC
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?
Comment 9 Tim Janik 2007-06-14 11:19:45 UTC
(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.
Comment 10 Matthias Clasen 2007-06-14 11:36:09 UTC
Whatever you end up doing, please make sure you mention it in the release notes (thats the 2.12 section in README.in)
Comment 11 Kristian Rietveld 2007-06-15 09:32:22 UTC
(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???)


Comment 12 Tim Janik 2007-06-15 09:42:46 UTC
(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.
Comment 13 Sven Neumann 2007-06-18 07:20:59 UTC
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 14 Tim Janik 2007-06-19 12:01:05 UTC
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.
Comment 15 Sven Neumann 2007-06-19 15:09:49 UTC
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.
Comment 16 Tim Janik 2007-06-25 13:01:21 UTC
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.
Comment 17 Kristian Rietveld 2007-06-26 10:27:36 UTC
GtkTooltips has been deprecated. (r18241)
Comment 18 Vincent Untz 2007-07-03 14:52:10 UTC
(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.
Comment 19 Tim Janik 2007-07-03 23:03:32 UTC
(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?
Comment 20 Vincent Untz 2007-07-03 23:06:47 UTC
(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.
Comment 21 Elijah Newren 2007-07-04 00:41:23 UTC
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.
Comment 22 Elijah Newren 2007-07-04 00:47:18 UTC
(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...)
Comment 23 Matthias Clasen 2007-07-10 04:27:40 UTC
Are we done here ? 

Any reason to keep this bug open ?
Comment 24 Kristian Rietveld 2007-07-10 08:42:31 UTC
gtktooltips.h has been fully deprecated now, I think this bug should stay open until the rename has been undone.
Comment 25 Matthias Clasen 2007-07-10 13:32:43 UTC
Ok, lets rename the bug, then.
Comment 26 Matthias Clasen 2007-07-21 13:20:44 UTC
2007-07-21  Matthias Clasen  <mclasen@redhat.com>

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