Merge lp:~mhr3/unity/background-icon-loading into lp:unity

[Michal Hruby]

Move all icon loading into background gio threads. (there was mainly issue with gtk_icon_info_load_icon which is doing synchronous IO and noticeably blocks the UI when using some themes)

[Sam Spilsbury (smspillaz)]

All ok on the code and testing.

161 + // careful here this is running in non-main thread
162 + if (task->icon_info)
163 + {

Maybe here you might want to use mutexes to synchronize the threads ? I'm not sure about the exact semantics of g_io_scheduler_push_job - if it processes jobs in serial or parallel.

review: Approve

[Michal Hruby (mhr3)]

> All ok on the code and testing.
>
> 161 + // careful here this is running in non-main thread
> 162 + if (task->icon_info)
> 163 + {
>
> Maybe here you might want to use mutexes to synchronize the threads ? I'm not
> sure about the exact semantics of g_io_scheduler_push_job - if it processes
> jobs in serial or parallel.

There is no synchronization needed, once this method starts icon_info is either already set to valid GtkIconInfo instance (now owned by the task), or NULL in case of URI request. This is merely a reminder for anyone wanting to touch that code.

As for serial vs parallel - it's both, since GIOScheduler uses a thread pool.

[Gord Allott (gordallott)]

review: Approve

=== modified file 'plugins/unityshell/src/IconLoader.cpp'
--- plugins/unityshell/src/IconLoader.cpp	2011-09-23 15:06:46 +0000
+++ plugins/unityshell/src/IconLoader.cpp	2012-01-23 11:29:26 +0000
@@ -85,6 +85,9 @@
     IconLoaderCallback    slot;
     Handle                handle;
     Impl*                 self;
+    GtkIconInfo*          icon_info;
+    GdkPixbuf*            result;
+    glib::Error           error;
 
     IconLoaderTask(IconLoaderRequestType type_,
                    std::string const& data_,
@@ -95,6 +98,7 @@
                    Impl* self_)
       : type(type_), data(data_), size(size_), key(key_)
       , slot(slot_), handle(handle_), self(self_)
+      , icon_info(NULL), result(NULL)
       {}
   };
 
@@ -116,17 +120,19 @@
                    unsigned size,
                    IconLoaderCallback slot);
 
+  // these methods might run asynchronously
   bool ProcessTask(IconLoaderTask* task);
   bool ProcessIconNameTask(IconLoaderTask* task);
   bool ProcessGIconTask(IconLoaderTask* task);
-
-  // URI processing is async.
   bool ProcessURITask(IconLoaderTask* task);
-  void ProcessURITaskReady(IconLoaderTask* task, char* contents, gsize length);
-  static void LoadContentsReady(GObject* object, GAsyncResult* res, IconLoaderTask* task);
+
+  // Loading/rendering of pixbufs is done in a separate thread
+  static gboolean LoaderJobFunc(GIOSchedulerJob* job, GCancellable *canc,
+                                IconLoaderTask *task);
+  static gboolean LoadIconComplete(IconLoaderTask* task);
 
   // Loop calls the iteration function.
-  static bool Loop(Impl* self);
+  static gboolean Loop(Impl* self);
   bool Iteration();
 
 private:
@@ -312,26 +318,17 @@
 
 bool IconLoader::Impl::ProcessIconNameTask(IconLoaderTask* task)
 {
-  GdkPixbuf* pixbuf = nullptr;
   GtkIconInfo* info = gtk_icon_theme_lookup_icon(theme_,
                                                  task->data.c_str(),
                                                  task->size,
                                                  (GtkIconLookupFlags)0);
   if (info)
   {
-    glib::Error error;
+    task->icon_info = info;
+    g_io_scheduler_push_job ((GIOSchedulerJobFunc) LoaderJobFunc,
+                             task, NULL, G_PRIORITY_HIGH_IDLE, NULL);
 
-    pixbuf = gtk_icon_info_load_icon(info, &error);
-    if (GDK_IS_PIXBUF(pixbuf))
-    {
-      cache_[task->key] = pixbuf;
-    }
-    else
-    {
-      LOG_WARNING(logger) << "Unable to load icon " << task->data
-                          << " at size " << task->size << ": " << error;
-    }
-    gtk_icon_info_free(info);
+    return false;
   }
   else
   {
@@ -339,14 +336,12 @@
                         << " at size " << task->size;
   }
 
-  task->slot(task->data, task->size, pixbuf);
+  task->slot(task->data, task->size, nullptr);
   return true;
 }
 
 bool IconLoader::Impl::ProcessGIconTask(IconLoaderTask* task)
 {
-  GdkPixbuf*   pixbuf = NULL;
-
   glib::Error error;
   glib::Object<GIcon> icon(::g_icon_new_for_string(task->data.c_str(), &error));
 
@@ -368,18 +363,11 @@
                                                          (GtkIconLookupFlags)0);
     if (info)
     {
-      pixbuf = gtk_icon_info_load_icon(info, &error);
+      task->icon_info = info;
+      g_io_scheduler_push_job ((GIOSchedulerJobFunc) LoaderJobFunc,
+                               task, NULL, G_PRIORITY_HIGH_IDLE, NULL);
 
-      if (GDK_IS_PIXBUF(pixbuf))
-      {
-        cache_[task->key] = pixbuf;
-      }
-      else
-      {
-        LOG_WARNING(logger) << "Unable to load icon " << task->data
-                            << " at size " << task->size << ": " << error;
-      }
-      gtk_icon_info_free(info);
+      return false;
     }
     else
     {
@@ -409,58 +397,91 @@
                         << " at size " << task->size << ": " << error;
   }
 
-  task->slot(task->data, task->size, pixbuf);
+  task->slot(task->data, task->size, nullptr);
   return true;
 }
 
 bool IconLoader::Impl::ProcessURITask(IconLoaderTask* task)
 {
-  glib::Object<GFile> file(g_file_new_for_uri(task->data.c_str()));
-
-  g_file_load_contents_async(file,
-                             NULL,
-                             (GAsyncReadyCallback)LoadContentsReady,
-                             task);
+  g_io_scheduler_push_job ((GIOSchedulerJobFunc) LoaderJobFunc,
+                           task, NULL, G_PRIORITY_HIGH_IDLE, NULL);
 
   return false;
 }
 
-void IconLoader::Impl::ProcessURITaskReady(IconLoaderTask* task,
-                                           char* contents,
-                                           gsize length)
-{
-  GInputStream* stream = g_memory_input_stream_new_from_data(contents, length, NULL);
-
-  glib::Error error;
-  glib::Object<GdkPixbuf> pixbuf(gdk_pixbuf_new_from_stream_at_scale(stream,
-                                                                     -1,
-                                                                     task->size,
-                                                                     true,
-                                                                     NULL,
-                                                                     &error));
-  if (error)
-  {
-    LOG_WARNING(logger) << "Unable to create pixbuf from input stream for "
-                        << task->data << " at size " << task->size << ": " << error;
+gboolean IconLoader::Impl::LoaderJobFunc(GIOSchedulerJob* job,
+                                         GCancellable *canc,
+                                         IconLoaderTask *task)
+{
+  // careful here this is running in non-main thread
+  if (task->icon_info)
+  {
+    task->result = gtk_icon_info_load_icon(task->icon_info, &task->error);
+
+    gtk_icon_info_free (task->icon_info);
+    task->icon_info = NULL;
+  }
+  else if (task->type == REQUEST_TYPE_URI)
+  {
+    glib::Object<GFile> file(g_file_new_for_uri(task->data.c_str()));
+    glib::String contents;
+    gsize length = 0;
+
+    if (g_file_load_contents(file, canc, &contents, &length,
+                             NULL, &task->error))
+    {
+      glib::Object<GInputStream> stream(
+          g_memory_input_stream_new_from_data(contents.Value(), length, NULL));
+
+      task->result = gdk_pixbuf_new_from_stream_at_scale(stream,
+                                                         -1,
+                                                         task->size,
+                                                         TRUE,
+                                                         canc,
+                                                         &task->error);
+      g_input_stream_close(stream, canc, NULL);
+    }
+  }
+
+  g_io_scheduler_job_send_to_mainloop_async (job,
+                                             (GSourceFunc) LoadIconComplete,
+                                             task,
+                                             NULL);
+
+  return FALSE;
+}
+
+// this will be invoked back in the thread from which push_job was called
+gboolean IconLoader::Impl::LoadIconComplete(IconLoaderTask* task)
+{
+  if (GDK_IS_PIXBUF(task->result))
+  {
+    task->self->cache_[task->key] = task->result;
   }
   else
   {
-    cache_[task->key] = pixbuf;
+    LOG_WARNING(logger) << "Unable to load icon " << task->data
+                        << " at size " << task->size << ": " << task->error;
   }
 
-  task->slot(task->data, task->size, pixbuf);
-  g_input_stream_close(stream, NULL, NULL);
+  task->slot(task->data, task->size, task->result);
+
+  // this was all async, we need to erase ourselves from the task_map
+  task->self->task_map_.erase(task->handle);
+  delete task;
+
+  return FALSE;
 }
 
 bool IconLoader::Impl::Iteration()
 {
-  static const int MAX_MICRO_SECS = 10000;
+  static const int MAX_MICRO_SECS = 1000;
   util::Timer timer;
 
   bool queue_empty = tasks_.empty();
 
-  while (!queue_empty &&
-         (timer.ElapsedMicroSeconds() < MAX_MICRO_SECS))
+  // always do at least one iteration if the queue isn't empty
+  while (!queue_empty)
   {
     IconLoaderTask* task = tasks_.front();
 
@@ -472,6 +493,8 @@
 
     tasks_.pop();
     queue_empty = tasks_.empty();
+
+    if (timer.ElapsedMicroSeconds() >= MAX_MICRO_SECS) break;
   }
 
   LOG_DEBUG(logger) << "Iteration done, queue size now at " << tasks_.size();
@@ -487,33 +510,10 @@
 }
 
 
-bool IconLoader::Impl::Loop(IconLoader::Impl* self)
-{
-  return self->Iteration();
-}
-
-void IconLoader::Impl::LoadContentsReady(GObject* obj,
-                                         GAsyncResult* res,
-                                         IconLoaderTask* task)
-{
-  glib::String contents;
-  glib::Error error;
-  gsize length = 0;
-
-  if (g_file_load_contents_finish(G_FILE(obj), res, &contents, &length, NULL, &error))
-  {
-    task->self->ProcessURITaskReady(task, contents.Value(), length);
-  }
-  else
-  {
-    LOG_WARNING(logger) << "Unable to load contents of "
-                        << task->data << ": " << error;
-    task->slot(task->data, task->size, nullptr);
-  }
-  task->self->task_map_.erase(task->handle);
-  delete task;
-}
-
+gboolean IconLoader::Impl::Loop(IconLoader::Impl* self)
+{
+  return self->Iteration() ? TRUE : FALSE;
+}
 
 IconLoader::IconLoader()
   : pimpl(new Impl())

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2012-01-17 13:31:52 +0000
+++ tests/CMakeLists.txt	2012-01-23 11:29:26 +0000
@@ -184,10 +184,15 @@
                  test_dashview_impl.cpp
                  test_texture_cache.cpp
                  test_main.cpp
+                 test_icon_loader.cpp
                  ${UNITY_SRC}/DashViewPrivate.cpp
                  ${UNITY_SRC}/DashViewPrivate.h
                  ${UNITY_SRC}/TextureCache.cpp
                  ${UNITY_SRC}/TextureCache.h
+                 ${UNITY_SRC}/IconLoader.cpp
+                 ${UNITY_SRC}/IconLoader.h
+                 ${UNITY_SRC}/Timer.cpp
+                 ${UNITY_SRC}/Timer.h
                 )
   target_link_libraries(test-gtest ${GTEST_BOTH_LIBRARIES})
   add_test(UnityGTest test-gtest)

=== added file 'tests/test_icon_loader.cpp'
--- tests/test_icon_loader.cpp	1970-01-01 00:00:00 +0000
+++ tests/test_icon_loader.cpp	2012-01-23 11:29:26 +0000
@@ -0,0 +1,191 @@
+/*
+ * Copyright 2012 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the  Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 3 along with this program.  If not, see
+ * <http://www.gnu.org/licenses/>
+ *
+ * Authored by: Michal Hruby <[e-mail]>
+ */
+
+#include <gmock/gmock.h>
+
+#include "IconLoader.h"
+
+using namespace testing;
+using namespace unity;
+
+namespace
+{
+bool IsValidPixbuf(GdkPixbuf *pixbuf)
+{
+  return GDK_IS_PIXBUF (pixbuf);
+}
+
+gboolean TimeoutReached (gpointer data)
+{
+  bool *b = static_cast<bool*>(data);
+
+  *b = true;
+
+  return FALSE;
+}
+
+struct LoadResult
+{
+  GdkPixbuf *pixbuf;
+  bool got_callback;
+
+  LoadResult() : pixbuf(NULL), got_callback(false) {}
+  void IconLoaded(std::string const& icon_name, unsigned size,
+                  GdkPixbuf *buf)
+  {
+    pixbuf = buf;
+
+    got_callback = true;
+  }
+};
+
+TEST(TestIconLoader, TestGetDefault)
+{
+  // we need to initialize gtk
+  int args_cnt = 0;
+  gtk_init (&args_cnt, NULL);
+
+  IconLoader::GetDefault();
+}
+
+TEST(TestIconLoader, TestGetOneIcon)
+{
+  LoadResult load_result;
+  IconLoader& icon_loader = IconLoader::GetDefault();
+  volatile bool timeout_reached = false;
+
+  icon_loader.LoadFromIconName("gedit-icon", 48, sigc::mem_fun(load_result,
+        &LoadResult::IconLoaded));
+
+  guint tid = g_timeout_add (10000, TimeoutReached, (gpointer)(&timeout_reached));
+  while (!timeout_reached && !load_result.got_callback)
+  {
+    g_main_context_iteration (NULL, TRUE);
+  }
+
+  EXPECT_TRUE(load_result.got_callback);
+  EXPECT_TRUE(IsValidPixbuf(load_result.pixbuf));
+
+  g_source_remove (tid);
+}
+
+TEST(TestIconLoader, TestGetManyIcons)
+{
+  std::vector<LoadResult> results;
+  IconLoader& icon_loader = IconLoader::GetDefault();
+  volatile bool timeout_reached = false;
+  int i = 0;
+  int icon_count;
+
+  GList *icons = gtk_icon_theme_list_icons (gtk_icon_theme_get_default (),
+                                            "Applications");
+  // loading 100 icons should suffice
+  icon_count = MIN (100, g_list_length (icons));
+  results.resize (icon_count);
+  for (GList *it = icons; it != NULL; it = it->next)
+  {
+    const char *icon_name = static_cast<char*>(it->data);
+    icon_loader.LoadFromIconName(icon_name, 48, sigc::mem_fun(results[i++],
+        &LoadResult::IconLoaded));
+    if (i >= icon_count) break;
+  }
+
+  guint tid = g_timeout_add (30000, TimeoutReached, (gpointer)(&timeout_reached));
+  while (!timeout_reached)
+  {
+    g_main_context_iteration (NULL, TRUE);
+    bool all_loaded = true;
+    for (auto loader: results)
+    {
+      all_loaded &= loader.got_callback;
+      if (!all_loaded) break;
+    }
+    if (all_loaded) break;
+  }
+
+  for (auto load_result: results)
+  {
+    EXPECT_TRUE(load_result.got_callback);
+    EXPECT_TRUE(IsValidPixbuf(load_result.pixbuf));
+  }
+
+  g_source_remove (tid);
+}
+
+TEST(TestIconLoader, TestCancelSome)
+{
+  std::vector<LoadResult> results;
+  std::vector<int> handles;
+  IconLoader& icon_loader = IconLoader::GetDefault();
+  volatile bool timeout_reached = false;
+  int i = 0;
+  int icon_count;
+
+  GList *icons = gtk_icon_theme_list_icons (gtk_icon_theme_get_default (),
+                                            "Emblems");
+  // loading 100 icons should suffice
+  icon_count = MIN (100, g_list_length (icons));
+  results.resize (icon_count);
+  handles.resize (icon_count);
+  for (GList *it = icons; it != NULL; it = it->next)
+  {
+    const char *icon_name = static_cast<char*>(it->data);
+    int handle = icon_loader.LoadFromIconName(icon_name, 48, sigc::mem_fun(
+          results[i], &LoadResult::IconLoaded));
+    handles[i++] = handle;
+    if (i >= icon_count) break;
+  }
+
+  // disconnect every other handler
+  for (i = 0; i < icon_count; i += 2)
+  {
+    icon_loader.DisconnectHandle(handles[i]);
+  }
+
+  guint tid = g_timeout_add (30000, TimeoutReached, (gpointer)(&timeout_reached));
+  while (!timeout_reached)
+  {
+    g_main_context_iteration (NULL, TRUE);
+    bool all_loaded = true;
+    for (int i = 1; i < icon_count; i += 2)
+    {
+      all_loaded &= results[i].got_callback;
+      if (!all_loaded) break;
+    }
+    if (all_loaded) break;
+  }
+
+  for (i = 0; i < icon_count; i++)
+  {
+    if (i % 2)
+    {
+      EXPECT_TRUE(results[i].got_callback);
+      EXPECT_TRUE(IsValidPixbuf(results[i].pixbuf));
+    }
+    else
+    {
+      EXPECT_FALSE(results[i].got_callback);
+    }
+  }
+
+  g_source_remove (tid);
+}
+
+
+}