[Patch] Cursor cache

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

Hi,
  Attached is a patch that adds a cache to the cursor code in
gdk/x11/gdkcursor-x11.c - I'd appreciate comments.  Thanks
for Matthias Clasen for some pointers after my initial mail.

  As I mentioned in a mail a few weeks ago I'd been looking at the
code that deals with themeing of cursors and was interested to see
if something could be done to make it a bit more efficient.

  The cache holds any cursor looked up by ID or by name - but ignores
custom ones set from pixmaps; this avoids having to search all the theme
directories.

  I've done some timings on my Ubuntu 64bit laptop (Core 2 Duo 1.66GHz)
and the time for a gdk_cursor_new_from_name drops from around 600us to
about 3us - although I don't think my timing is better than about 1us.
The cache lookup adds a couple of us in the miss-case.  My guess
is that the improvement is even better when the home directory is
on NFS or in the case where the machine is busy (although
the OS should cache most of the 'open' calls that fail anyway).

  So what's 0.5ms between friends? Well there are a couple of cache hits
opening subsequent gnome-terminal instances, and for some reason
gnome-text-editor really really likes setting the cursor quite a lot
(not quite every time you enter a character but quite often).
Rhythmbox looks up the same cursor (GDK_SB_H_DOUBLE_ARROW) about 73
times during startup - of which 72 of those now cache.   An 'Open'
dialog on gnome-text-editor takes 9 hits.  So these add up to a few
ms here and there; although perhaps it would be better to fix the app?
If anyone has a good 'startup time for a desktop' I'd be interested
to see some results.

One thing I'm not too clear on is the theme change code; I've
changed it to walk the cache and apply the new theme to it - it seems
to work OK but there are cases with drag-and-drop where after
a cursor theme change I still see the old composite drag cursors;
but I see this with an unmodified library and the gtkdnd code
has code that's supposed to fix up drag-and-drop cases by
deleting cursors each time - is that an existing bug somewhere?

The other observation is that various things get a blank cursor
by doing a 1x1 pixmap cursor fully transparent; this ends up going
all the way through the theming code looking for a file called
00...00  which is a hash of the pixmap; that causes about a dozen
or more open(2)'s and reading of the theme indexes etc - this
seems mad for a blank cursor!  I wonder what the easiest fix is for
this - possibly adding another GdkCursorType as a special? Or is it
better to check for that special case pixmap lower down? Or add a 
gdk_cursor_new_blank?

The patch is against svn revision 22084 from about a week ago;
tested on Ubuntu Linux (latest 'Jaunty' version).

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/x11/gdkcursor-x11.c
===================================================================
--- gdk/x11/gdkcursor-x11.c	(revision 22084)
+++ gdk/x11/gdkcursor-x11.c	(working copy)
@@ -49,6 +49,71 @@
 
 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;
+}
+
 /**
  * gdk_cursor_new_for_display:
  * @display: the #GdkDisplay for which the cursor will be created
@@ -127,10 +192,21 @@
 
   g_return_val_if_fail (GDK_IS_DISPLAY (display), NULL);
 
-  if (display->closed)
+  if (display->closed) {
     xcursor = None;
-  else
-    xcursor = XCreateFontCursor (GDK_DISPLAY_XDISPLAY (display), cursor_type);
+  } 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 {
+      xcursor = XCreateFontCursor (GDK_DISPLAY_XDISPLAY (display), cursor_type);
+    }
+  }
   
   private = g_new (GdkCursorPrivate, 1);
   private->display = display;
@@ -142,6 +218,8 @@
   cursor->type = cursor_type;
   cursor->ref_count = 1;
   
+  add_to_cache(private);
+
   return cursor;
 }
 
@@ -427,26 +505,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 +576,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 +752,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 +777,8 @@
   cursor = (GdkCursor *) private;
   cursor->type = GDK_CURSOR_IS_PIXMAP;
   cursor->ref_count = 1;
-  
+  add_to_cache(private);
+
   return cursor;
 }