Merge lp:~unity-team/unity/mmaped-icons into lp:unity

[Mikkel Kamstrup Erlandsen]

Improves speed and system load of icon loading by making use of Gtk+'s mmap()ed icon-theme.cache. It also batches the loads into clusters of 10 icons.

While this doesn't completely fix the rendering speed of the places on a cold cache, it is definitely a step in the right direction.

[Neil J. Patel (njpatel)]

Looks good, and works (testing on a maverick install). Feel free to merge into 0.2 series. Also, is it worth asking didrocks to SRU?

review: Approve

[Mikkel Kamstrup Erlandsen (kamstrup)]

Branch was merged into lp:unity/0.2. Launchpad is unable to reflect that change. Sorry.

=== modified file 'unity/unity-pixbuf-cache.vala'
--- unity/unity-pixbuf-cache.vala	2010-09-22 09:55:24 +0000
+++ unity/unity-pixbuf-cache.vala	2010-10-05 16:24:42 +0000
@@ -126,15 +126,18 @@
       cache.clear ();
     }
 
-    private async void process_icon_queue_async ()
+    /* Do the actual work of loading icons.
+     * IMPORTANT: Must be run with G_PRIORITY_LOW in order to allow Clutter
+     *            and other libs to have time on the mainloop. Like syncing
+     *            to vblank to have smooth rendering */
+    private bool process_icon_queue_real ()
     {
-      /* Hardcoded numbers make gord cry - but so be it - we are taking
-       * a second guess of the inode size here, for the better of mankind */
-      uchar[] buf = new uchar[4096];
+      PixbufCacheTask task;
+      int batch_size = 10;
       int count = 0;
-      var timer = new Timer ();
-      PixbufCacheTask task;
       
+      /* Load in batches of 10 (choosen by fair dice rool - honest!),
+       * then yield the mainloop */
       while ((task = queue.poll()) != null)
         {
           if (task.image is Ctk.Image)
@@ -142,53 +145,50 @@
               switch (task.type)
                 {
                   case PixbufRequestType.ICON_NAME:
-                    yield set_image_from_icon_name_real (task, buf, buf.length);
+                    set_image_from_icon_name_real (task);
                     break;
                   case PixbufRequestType.GICON_STRING:
-                    yield set_image_from_gicon_string_real (task, buf, buf.length);
+                    set_image_from_gicon_string_real (task);
                     break;
                   default:
                     critical ("Internal error. Unknown PixbufRequestType: %u",
                               task.type);
                     break;
                 }
-            }          
+            }
+          
           count++;
+          if (count >= batch_size)
+            {
+              /* Yield the mainloop for a bit more than 1 frame
+               * so we can draw the new icons */
+              Timeout.add (1000/Clutter.get_default_frame_rate() + 5,
+                           process_icon_queue_real);
+              return false;
+            }
+          
         }
-        
-      timer.stop ();
-      debug ("Loaded %i icons in %fms", count, timer.elapsed()*1000);
       
       /* Queue depleted */
       queue_source = 0;
+      return false;        
     }
 
     private void process_icon_queue ()
     {
       if (queue_source == 0)
         {
-          /* queue_source is set to 0 by process_icon_queue_async()
+          /* queue_source is set to 0 by process_icon_queue_real()
            * when the queue is depleted.
            * It's important that we dispatch in an idle call here since
            * it gives clients a chance to queue more icons before we
-           * hammer the IO */
-          queue_source = Idle.add (dispatch_process_icon_queue_async);          
+           * hammer the IO.
+           * MUCHO IMPORTANTE: We want G_PRIORITY_LOW in order to not steal the
+           *                   mainloop from Clutter and friends */
+          queue_source = Idle.add_full (Priority.LOW,
+                                        process_icon_queue_real);
         }
     }
-    
-    private bool dispatch_process_icon_queue_async ()
-    {
-      /* Run three processings in "parallel". While we are not multithreaded,
-       * it still makes sense to do since one branch may be waiting in
-       * 'yield' for async IO, while the other can be parsing image data.
-       * The number of parallel workers has been chosen by best perceived
-       * responsiveness for bringing up the Applications place */
-       process_icon_queue_async.begin ();
-       process_icon_queue_async.begin ();
-       process_icon_queue_async.begin ();
-       
-       return false;
-    }
 
     /**
      * If the icon is already cached then set it immediately on @image. Otherwise
@@ -224,9 +224,7 @@
       process_icon_queue ();
     }
 
-    private async void set_image_from_icon_name_real (PixbufCacheTask task,
-                                                      void* buf,
-                                                      size_t buf_length)
+    private void set_image_from_icon_name_real (PixbufCacheTask task)
     {
       Pixbuf? ret = cache[task.key];
 
@@ -242,8 +240,7 @@
         var info = theme.lookup_icon (task.data, task.size, 0);
         if (info != null)
           {
-            var filename = info.get_filename ();
-            ret = yield load_from_filepath (filename, task.size, buf, buf_length);
+            ret = info.load_icon ();
           }
 
         if (ret is Pixbuf)
@@ -295,9 +292,7 @@
       process_icon_queue ();
     }
 
-    private async void set_image_from_gicon_string_real (PixbufCacheTask task,
-                                                         void* buf, 
-                                                         size_t buf_length)
+    private void set_image_from_gicon_string_real (PixbufCacheTask task)
     {
       Pixbuf? ret = cache[task.key];
 
@@ -309,16 +304,6 @@
           return;
         }
       
-      if (task.data[0] == '/')
-        {
-          try {
-            ret = yield load_from_filepath (task.data, task.size, buf, buf_length);
-          } catch (Error err) {
-            message (@"Unable to load $(task.data) as file: %s",
-                     err.message);
-          }
-        }
-
       if (ret == null)
         {
           try {
@@ -326,8 +311,7 @@
             var info = theme.lookup_by_gicon (icon, task.size, 0);
             if (info != null)
               {
-                var filename = info.get_filename ();
-                ret = yield load_from_filepath (filename, task.size, buf, buf_length);
+                ret = info.load_icon ();
               }
 
             if (ret == null)
@@ -346,8 +330,7 @@
                      info = theme.lookup_icon (real_name, task.size, 0);
                      if (info != null)
                        {
-                         var fname = info.get_filename ();
-                         ret = yield load_from_filepath (fname, task.size, buf, buf_length);
+                         ret = info.load_icon ();
                        }
                    }
               }