Re: [Patch] Cursor cache

["Dr. David Alan Gilbert" <gilbertd treblig org>]

* Matthias Clasen (matthias clasen gmail com) wrote:
> On Sat, Jan 17, 2009 at 8:19 PM, Dr. David Alan Gilbert
> <gilbertd treblig org> wrote:
> > * Matthias Clasen (matthias clasen gmail com) wrote:
> >> Thanks for the patch. It looks good in general, just a couple of notes:
> >>
> >> * There are some stylistic issues (mostly missing spaces here and there)
> >
> > No problem; please point out a few of the ones that need fixing and
> > I'll try and make they all get done.
> 

OK, attached is a new patch that hopefully fixes the formatting;
if you spot any others I'll be happy to fix them (even if I do
feel sorry for the {'s lonely on the line by themselves)

However it's rather untested - but I've got some questions which go with it;
and I've pretty much run out of time this weekend - so I'll put it out
for comments and fix anything next weekend.

> * We need to purge cursors from the cache when their display is closed
> >
> > OK - is there a hook for that? Also is there an easy way to test
> > multidisplay code like that - what can I easily get to open something
> > on two displays?
> 
> gtk-demo has a 'change display'  demo.
> gdk/x11/gdkdisplay-x11.c:gdk_display_x11_finalize is the right place, I think.

OK, the attached patch has untested code for that - is there an easy
way to delete everything in an slist that matches a particular requirement?
(a foreach where the function returned a flag to say delete?)
I'll test it next week.
(Also is the GDK_DISPLAY_OBJECT the right way of getting the GdkDisplay*
pointer in gdkdisplay-x11.c ?)

> >> * Do you have any data how big the cache grows in cursor-heavy apps ?
> >
> > No, but I've not found anything that uses more than 3 or 4 cursors;
> > they just open the same damn ones over and over again.  The overhead
> > for each cache entry is small, cursors seem to vary on disc, but seem
> > to be between 15 and 35k except for some big ones but you are only paying
> > a penalty for those that are used rarely.
> 
> Ah, I was really interested in the length of the list. That sounds fine then.

Yeh, the only one that worries me a bit is the watch animations, they seem
to be quite big.  (It's a pity there is no way to store this data on the Xserver
and share between all apps).

> GDK_BLANK_CURSOR = -2 should work fine.

OK, that patch has code in there to do that - which seems to work
(tried with a patched libvte - patch attached)
However it needs some work.  It works out quite nicely
because it just ends up getting cached like anything else created
through gdk_cursor_new_for_display; my 'get_blank_cursor' function
doesn't try to do anything smart - so the first lookup still ends
up going through XCursor's theme code; I'll look at that again next week.
But I've got some questions about how things should work:

    1) I'm confused by GdkColor's - the code in Vte initialises black by
       doing:

       GdkColor black = {0,0,0}

       but that type has 4 elements - including 'pixel'; so that initialiser
       initialises pixel, red, and green - but not blue.  What's the right
       way of getting a GdkColor for black?

    2) The code seems schizophrenic as to whether we care about the
       screen or not;
       it looks like bitmaps should be created with a drawable for the
       correct screen since we could have two screens with different
       depths - but there are other places (like the code I copied)
       that just use the default screen; and most of the code in
       gdkcursor just takes a Display.  libXCursor seems the same, but
       gdk_window_get_pointer has a check for getting a valid window to
       be multihead safe.  What's the right thing to do?


Also, before GDK_BLANK_CURSOR can be used I guess the none-x11 versions
have to be fixed up as well - I'll need a hand with that since I just
have Linux.

As I say, this version hasn't had much testing - so use at your own risk;
I'll fix and test it more next weekend - all comments welcome.

Dave
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    | Running GNU/Linux on Alpha,68K| Happy  \ 
\ gro.gilbert @ treblig.org | MIPS,x86,ARM,SPARC,PPC & HPPA | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Index: gdk/gdkcursor.h
===================================================================
--- gdk/gdkcursor.h	(revision 22084)
+++ gdk/gdkcursor.h	(working copy)
@@ -120,7 +120,8 @@
   GDK_WATCH 		  = 150,
   GDK_XTERM 		  = 152,
   GDK_LAST_CURSOR,
-  GDK_CURSOR_IS_PIXMAP 	= -1
+  GDK_BLANK_CURSOR        = -2,
+  GDK_CURSOR_IS_PIXMAP 	  = -1
 } GdkCursorType;
 
 struct _GdkCursor
Index: gdk/x11/gdkdisplay-x11.c
===================================================================
--- gdk/x11/gdkdisplay-x11.c	(revision 22084)
+++ gdk/x11/gdkdisplay-x11.c	(working copy)
@@ -838,6 +838,8 @@
       g_free (display_x11->motif_target_lists);
     }
 
+  _gdk_x11_cursor_display_finalize (GDK_DISPLAY_OBJECT(display_x11));
+
   /* Atom Hashtable */
   g_hash_table_destroy (display_x11->atom_from_virtual);
   g_hash_table_destroy (display_x11->atom_to_virtual);
Index: gdk/x11/gdkprivate-x11.h
===================================================================
--- gdk/x11/gdkprivate-x11.h	(revision 22084)
+++ gdk/x11/gdkprivate-x11.h	(working copy)
@@ -191,6 +191,7 @@
 				      GdkGC       *gc);
 
 void _gdk_x11_cursor_update_theme (GdkCursor *cursor);
+void _gdk_x11_cursor_display_finalize (GdkDisplay *display);
 
 gboolean _gdk_x11_get_xft_setting (GdkScreen   *screen,
 				   const gchar *name,
Index: gdk/x11/gdkcursor-x11.c
===================================================================
--- gdk/x11/gdkcursor-x11.c	(revision 22084)
+++ gdk/x11/gdkcursor-x11.c	(working copy)
@@ -49,6 +49,136 @@
 
 static guint theme_serial = 0;
 
+/* cursor_cache holds a cache of non-pixmap cursors to avoid expensive libXcursor searches,
+   cursors are added to it but never removed.  I make the assumption that since there are a small
+   number of GdkDisplay's and a small number of cursor's that this list will stay small enough
+   not to be a problem.
+ */
+static GSList* cursor_cache = NULL;
+
+struct cursor_cache_key
+{
+  GdkDisplay* display;
+  GdkCursorType type;
+  const char* name;
+};
+
+/* Caller should check if there is already a match first
+ * Cursor MUST be either a typed cursor or a pixmap with a non-Null name
+ */
+static void
+add_to_cache (GdkCursorPrivate* cursor)
+{
+  cursor_cache = g_slist_prepend (cursor_cache, cursor);
+
+  /* Take a ref so that if the caller frees it we still have it */
+  gdk_cursor_ref ((GdkCursor*) cursor);
+}
+
+/* Returns 0 on a match
+ */
+static gint
+cache_compare_func (gconstpointer listelem, gconstpointer target)
+{
+  GdkCursorPrivate* curs = (GdkCursorPrivate*)listelem;
+  struct cursor_cache_key* key = (struct cursor_cache_key*)target;
+
+  if ((curs->cursor.type != key->type) ||
+      (curs->display != key->display))
+    return 1; /* No match */
+  
+  /* Elements marked as pixmap must be named cursors (since we don't store normal
+     pixmap cursors */
+  if (key->type == GDK_CURSOR_IS_PIXMAP)
+    return strcmp (key->name, curs->name);
+
+  return 0; /* Match */
+}
+
+/* Returns the cursor if there is a match, NULL if not
+   For named cursors type shall be GDK_CURSOR_IS_PIXMAP
+   For unnamed, typed cursors, name shall be NULL
+ */
+static GdkCursorPrivate*
+find_in_cache (GdkDisplay* display, GdkCursorType type, const char* name)
+{
+  GSList* res;
+  struct cursor_cache_key key;
+
+  key.display = display;
+  key.type = type;
+  key.name = name;
+
+  res = g_slist_find_custom (cursor_cache, &key, cache_compare_func);
+
+  return res?(GdkCursorPrivate*)(res->data):NULL;
+}
+
+/* Called by gdk_display_x11_finalize to flush any cached cursors
+   for a dead display.
+ */
+void 
+_gdk_x11_cursor_display_finalize (GdkDisplay *display)
+{
+  /* Untested! There must be an easier way to do this, a foreach that
+     took a flag back to say whether to delete an item would be nice */
+
+  GSList* item;
+  GSList** itemp; /* Pointer to the thing to fix when we delete an item */
+  item = cursor_cache;
+  itemp = &cursor_cache;
+  while (item)
+    {
+      GdkCursorPrivate* cursor = (GdkCursorPrivate*)(item->data);
+      if (cursor->display == display)
+        {
+	  GSList* olditem;
+          gdk_cursor_unref ((GdkCursor*) cursor);
+	  /* Remove this item from the list */
+	  *(itemp) = item->next;
+	  olditem = item;
+	  item = g_slist_next (item);
+	  g_slist_free_1 (olditem);
+        } else {
+	  itemp = &(item->next);
+	  item = g_slist_next (item);
+	}
+    }
+}
+
+static Cursor
+get_blank_cursor (GdkDisplay *display)
+{
+  /* TODO - figure out some magic to stop XCursor doing a theme search
+     if we have XCursor  */
+  GdkScreen *screen;
+  GdkPixmap *pixmap;
+  Pixmap source_pixmap;
+  XColor xfg, xbg;
+  Cursor xcursor;
+
+  screen = gdk_display_get_default_screen (display);
+  pixmap = gdk_bitmap_create_from_data (gdk_screen_get_root_window (screen), 
+					"\0\0\0\0\0\0\0\0", 1, 1);
+ 
+  source_pixmap = GDK_PIXMAP_XID(pixmap);
+
+  xfg.pixel = 0; /* Check?! */
+  xfg.red = xfg.blue = xfg.green = 0;
+  xbg.pixel = 0; /* Check?! */
+  xbg.red = xbg.blue = xbg.green = 0;
+  
+  if (display->closed)
+    xcursor = None;
+  else
+    xcursor = XCreatePixmapCursor (GDK_DISPLAY_XDISPLAY (display),
+				   source_pixmap, source_pixmap,
+                                   &xfg, &xbg, 1, 1);
+
+  g_object_unref (pixmap);
+  return xcursor;
+}
+
 /**
  * gdk_cursor_new_for_display:
  * @display: the #GdkDisplay for which the cursor will be created
@@ -108,11 +238,11 @@
  * <listitem><para>
  * <inlinegraphic format="PNG" fileref="sb_v_double_arrow.png"></inlinegraphic> #GDK_SB_V_DOUBLE_ARROW (move horizontal splitter)
  * </para></listitem>
+ * <listitem><para>
+ * #GDK_BLANK_CURSOR (Blank cursor)
+ * </para></listitem>
  * </itemizedlist>
  *
- * To make the cursor invisible, use gdk_cursor_new_from_pixmap() to create
- * a cursor with no pixels in it.
- * 
  * Return value: a new #GdkCursor
  *
  * Since: 2.2
@@ -128,9 +258,25 @@
   g_return_val_if_fail (GDK_IS_DISPLAY (display), NULL);
 
   if (display->closed)
-    xcursor = None;
-  else
-    xcursor = XCreateFontCursor (GDK_DISPLAY_XDISPLAY (display), cursor_type);
+    {
+      xcursor = None;
+    } else {
+      private = find_in_cache (display, cursor_type, NULL);
+
+      if (private)
+        {
+          /* Cache had it, add a ref for this user */
+          gdk_cursor_ref ((GdkCursor*) private);
+       
+          return (GdkCursor*) private;
+        } else {
+	  if (cursor_type != GDK_BLANK_CURSOR)
+            xcursor = XCreateFontCursor (GDK_DISPLAY_XDISPLAY (display),
+                                         cursor_type);
+	  else
+	    xcursor = get_blank_cursor (display);
+       }
+    }
   
   private = g_new (GdkCursorPrivate, 1);
   private->display = display;
@@ -142,6 +288,9 @@
   cursor->type = cursor_type;
   cursor->ref_count = 1;
   
+  if (xcursor != None)
+    add_to_cache (private);
+
   return cursor;
 }
 
@@ -427,26 +576,21 @@
 	new_cursor = XcursorShapeLoadCursor (xdisplay, cursor->type);
       
       if (new_cursor != None)
-	XFixesChangeCursor (xdisplay, new_cursor, private->xcursor);
+	{
+	  XFixesChangeCursor (xdisplay, new_cursor, private->xcursor);
+ 	  private->xcursor = new_cursor;
+	}
     }
 }
 
 static void
-update_cursor (gpointer key,
-	       gpointer value,
-	       gpointer data)
+update_cursor (gpointer data,
+	       gpointer user_data)
 {
-  XID *xid = key;
   GdkCursor *cursor;
 
-  if (*xid & XID_FONT_BIT)
-    return;
+  cursor = (GdkCursor*)(data);
 
-  if (!GDK_IS_WINDOW (value))
-    return;
-
-  cursor = _gdk_x11_window_get_cursor (GDK_WINDOW (value));
-
   if (!cursor)
     return;
   
@@ -503,7 +647,7 @@
   if (size > 0)
     XcursorSetDefaultSize (xdisplay, size);
     
-  g_hash_table_foreach (display_x11->xid_ht, update_cursor, NULL);
+  g_slist_foreach (cursor_cache, update_cursor, NULL);
 }
 
 #else
@@ -679,6 +823,16 @@
     xcursor = None;
   else 
     {
+      private = find_in_cache (display, GDK_CURSOR_IS_PIXMAP, name);
+
+      if (private)
+        {
+          /* Cache had it, add a ref for this user */
+          gdk_cursor_ref ((GdkCursor*) private);
+
+          return (GdkCursor*) private;
+        }
+
       xdisplay = GDK_DISPLAY_XDISPLAY (display);
       xcursor = XcursorLibraryLoadCursor (xdisplay, name);
       if (xcursor == None)
@@ -694,7 +848,8 @@
   cursor = (GdkCursor *) private;
   cursor->type = GDK_CURSOR_IS_PIXMAP;
   cursor->ref_count = 1;
-  
+  add_to_cache (private);
+
   return cursor;
 }
 

Index: src/vte.c
===================================================================
--- src/vte.c	(revision 2362)
+++ src/vte.c	(working copy)
@@ -8637,8 +8637,7 @@
 {
 	VteTerminal *terminal;
 	GdkWindowAttr attributes;
-	GdkPixmap *bitmap;
-	GdkColor black = {0,0,0}, color;
+	GdkColor color;
 	guint attributes_mask = 0, i;
 
 	_vte_debug_print(VTE_DEBUG_LIFECYCLE, "vte_terminal_realize()\n");
@@ -8736,13 +8735,8 @@
 	terminal->pvt->modifiers = 0;
 
 	/* Create our invisible cursor. */
-	bitmap = gdk_bitmap_create_from_data(widget->window, "\0", 1, 1);
-	terminal->pvt->mouse_inviso_cursor = gdk_cursor_new_from_pixmap(bitmap,
-									bitmap,
-									&black,
-									&black,
-									0, 0);
-	g_object_unref(bitmap);
+	terminal->pvt->mouse_inviso_cursor =
+           vte_terminal_cursor_new(terminal,  -2 /* GDK_BLANK_CURSOR */);
 
 	widget->style = gtk_style_attach(widget->style, widget->window);