Merge lp:~unity-team/unity/plugging-leaks-with-a-nailgun into lp:unity

[Mikkel Kamstrup Erlandsen]

This branch plugs some leaks we could have in places. These leaks where normally only exposed after several minutes of exercising the places (especially visible on low end graphics hardware like my i945).

While the virt mem can still spike up to dangerous levels causing massive slowdowns it seems that it's eventually garbage collected by the driver and the mem usage goes down to a (somewhat) sane level.

[Neil J. Patel (njpatel)]

Went through and checked all the nooks & crannies of places and they all seem to work fine. It definitely feels faster and mem usage seems reasonable on my system even after a bunch of place switches.

review: Approve

=== modified file 'unity-private/places/places-button.vala'
--- unity-private/places/places-button.vala	2010-09-17 15:19:02 +0000
+++ unity-private/places/places-button.vala	2010-10-01 09:55:56 +0000
@@ -95,11 +95,14 @@
         if (glow is Ctk.EffectGlow)
           glow.set_invalidate_effect_cache (true);
       });
-
+      
+      /* Must not point to a lambda or non-static method on 'this'.
+       * Causes self-refs, hence leaks, otherwise */
       outline_func = rounded_rect;
     }
-
-    public void rounded_rect (Cairo.Context cr, int width, int height)
+    
+    /* This method *must* be static. Otherwise we'll create a circular ref to self */
+    private static void rounded_rect (Cairo.Context cr, int width, int height)
     {
       var padding = 1;
       var x = padding;
@@ -125,7 +128,8 @@
       cr.curve_to (x, height,
                    x, height,
                    x, height - radius);
-      cr.close_path ();    }
+      cr.close_path ();
+    }
 
     private void paint_bg (Cairo.Context cr, int width, int height)
       requires (this is Button)
 
=== modified file 'unity-private/places/places-default-renderer-group.vala'
--- unity-private/places/places-default-renderer-group.vala	2010-09-22 18:20:59 +0000
+++ unity-private/places/places-default-renderer-group.vala	2010-10-01 09:55:56 +0000
@@ -59,7 +59,10 @@
     private Ctk.Image     icon;
     private Ctk.Text      text;
     private Expander      expander;
-    private unowned Ctk.IconView  renderer;
+    
+    /* If renderer is null it indicates that we have been disposed,
+     * and that all changes to the results should be ignored */
+    private unowned Ctk.IconView?  renderer;
 
     private MoreResultsButton? more_results_button;
 
@@ -73,8 +76,17 @@
 
     public signal void activated (string uri, string mimetype);
     
-    private GLib.List<Tile?> cleanup_tiles = new GLib.List<Tile?> ();
+    private Gee.Queue<Tile> cleanup_tiles = new Gee.LinkedList<Tile> ();
     private uint cleanup_operation = 0;
+    
+    private TileFactory tile_factory;
+    
+    private delegate Tile TileFactory (Dee.ModelIter iter,
+                                       string uri,
+                                       string icon_hint,
+                                       string mimetype,
+                                       string display_name,
+                                       string comment);
 
     public DefaultRendererGroup (uint      group_id,
                                  string    group_renderer,
@@ -86,7 +98,7 @@
               group_renderer:group_renderer,
               display_name:display_name,
               icon_hint:icon_hint,
-              results:results);
+              results:results);      
     }
 
     construct
@@ -98,6 +110,22 @@
         allow_expand = false;
       else if (group_renderer == "UnityFolderGroupRenderer")
         bin_state = ExpandingBinState.EXPANDED;
+      
+      /* Find the right tile factory for our rendering mode
+       * IMPORANT: The file_factory must *not* point to a lambda or
+       * non-static method on 'this'. That leads to self-refs (aka leaks) */
+      if (group_renderer == "UnityFileInfoRenderer")
+        {
+          tile_factory = file_info_tile_factory;
+        }
+      else if (group_renderer == "UnityShowcaseRenderer")
+        {
+          tile_factory = showcase_tile_factory;
+        }
+      else
+        {
+          tile_factory = default_tile_factory;
+        }
 
       vbox = new Ctk.VBox (SPACING);
       vbox.spacing = SPACING;
@@ -240,6 +268,28 @@
       results.row_removed.connect (on_result_removed);
 
     }
+    
+    private override void dispose ()
+    {
+      if (renderer != null)
+        {           
+          /* Drop refs to renderer since we have a cycle:
+           * self->renderer vs self->vbox->renderer */
+          renderer = null;
+          remove_actor (vbox);
+          
+          /* Disconnect signals, so we don't start doing funky stuff
+           * after we've been disposed */
+          results.row_added.disconnect (on_result_added);
+          results.row_removed.disconnect (on_result_removed);
+          
+          /* Drain the cleanup queue */
+          cleanup_tiles.clear ();
+        }
+      
+      /* Chain up */
+      base.dispose ();
+    }
 
     private override void allocate (Clutter.ActorBox        box,
                                     Clutter.AllocationFlags flags)
@@ -264,68 +314,76 @@
     /*
      * Private Methods
      */
+    
+    /* This method *must* be static in order to avoid self refs */
+    private static Tile file_info_tile_factory (Dee.ModelIter iter,
+                                                string uri,
+                                                string icon_hint,
+                                                string mimetype,
+                                                string display_name,
+                                                string comment)
+    {
+      return new FileInfoTile (iter, uri, icon_hint, mimetype, display_name, comment);
+    }
+    
+    /* This method *must* be static in order to avoid self refs */
+    private static Tile showcase_tile_factory (Dee.ModelIter iter,
+                                               string uri,
+                                               string icon_hint,
+                                               string mimetype,
+                                               string display_name,
+                                               string comment)
+    {
+      return new ShowcaseTile (iter, uri, icon_hint, mimetype, display_name, comment);
+    }
+    
+    /* This method *must* be static in order to avoid self refs */
+    private static Tile default_tile_factory (Dee.ModelIter iter,
+                                              string uri,
+                                              string icon_hint,
+                                              string mimetype,
+                                              string display_name,
+                                              string comment)
+    {
+      return new DefaultTile (iter, uri, icon_hint, mimetype, display_name, comment);
+    }
+     
     private void on_result_added (Dee.ModelIter iter)
     {
+      /* Ignore result set changes if we are disposed */
+      if (renderer == null)
+        return;
+    
       if (!interesting (iter))
         return;
 
       if (n_results == OMG_FOOTEL_SUCKS_CANT_HANDLE_MANY_TEXTURES)
         return;
-      
-      Tile button;
-      
-      if (cleanup_tiles.length () > 0)
+
+      /* Try to reuse a tile from the cleanup queue. We are guaranteed
+       * that it's the right type because all tiles are created from the same
+       * tile_factory instance */
+      Tile? button = cleanup_tiles.poll ();
+      
+      if (button is Tile)
         {
-          button = cleanup_tiles.nth_data (0);
           button.update_details (results.get_string (iter, 0),
                          results.get_string (iter, 1),
                          results.get_string (iter, 3),
                          results.get_string (iter, 4),
                          results.get_string (iter, 5));
           button.iter = iter;
-          cleanup_tiles.remove (button);
-          button.unref (); /* Because Vala holds references when it shouldn't*/;
-        }
-      else if (group_renderer == "UnityFileInfoRenderer")
-        {
-          button = new FileInfoTile (iter,
-                                     results.get_string (iter, 0),
-                                     results.get_string (iter, 1),
-                                     results.get_string (iter, 3),
-                                     results.get_string (iter, 4),
-                                     results.get_string (iter, 5));
-
-          renderer.add_actor (button);
-          button.show ();
-          button.unref (); /* Because Vala holds references when it shouldn't*/;
-          button.activated.connect ((u, m) => { activated (u, m); });
-        }
-      else if (group_renderer == "UnityShowcaseRenderer")
-        {
-          button = new ShowcaseTile (iter,
-                                     results.get_string (iter, 0),
-                                     results.get_string (iter, 1),
-                                     results.get_string (iter, 3),
-                                     results.get_string (iter, 4),
-                                     results.get_string (iter, 5));
-          renderer.add_actor (button);
-          button.show ();
-          button.unref (); /* Because Vala holds references when it shouldn't*/;
-
-          button.activated.connect ((u, m) => { activated (u, m); });
         }
       else
         {
-          button = new DefaultTile (iter,
-                                    results.get_string (iter, 0),
-                                    results.get_string (iter, 1),
-                                    results.get_string (iter, 3),
-                                    results.get_string (iter, 4),
-                                    results.get_string (iter, 5));
+          button = tile_factory (iter,
+                                 results.get_string (iter, 0),
+                                 results.get_string (iter, 1),
+                                 results.get_string (iter, 3),
+                                 results.get_string (iter, 4),
+                                 results.get_string (iter, 5));
           renderer.add_actor (button);
-          button.show ();
-          button.unref (); /* Because Vala holds references when it shouldn't*/;
-
+          button.show ();          
           button.activated.connect ((u, m) => { activated (u, m); });
         }
 
@@ -350,21 +408,27 @@
 
     private bool cleanup_operation_callback ()
     {
-      foreach (Tile? tile in cleanup_tiles)
+      Tile? tile;
+      
+      /* Drain the cleanup queue */
+      while ((tile = cleanup_tiles.poll ()) != null)
         {
-          renderer.remove_actor (tile);
+          /* If we are disposed renderer will be null */
+          if (renderer == null)
+            renderer.remove_actor (tile);
         }
 
-      cleanup_tiles = null;
-      cleanup_tiles = new GLib.List<Tile?> ();
-
       cleanup_operation = 0;
       return false;
     }
 
     private void on_result_removed (Dee.ModelIter iter)
     {
-     if (!interesting (iter))
+      /* Ignore result set changes if we are disposed */
+      if (renderer == null)
+        return;
+     
+      if (!interesting (iter))
         return;
 
       var children = renderer.get_children ();
@@ -374,7 +438,7 @@
 
           if (tile.iter == iter)
             {
-              cleanup_tiles.append (tile);
+              cleanup_tiles.offer (tile);
               if (cleanup_operation == 0)
                 cleanup_operation = Timeout.add (200, cleanup_operation_callback);
 
@@ -402,6 +466,10 @@
 
     private void add_to_n_results (int i)
     {
+      /* Ignore result set changes if we are disposed */
+      if (renderer == null)
+        return;
+      
       n_results += i;
 
       if (n_results > renderer.get_n_cols ())
@@ -430,6 +498,10 @@
 
     private void on_n_cols_changed ()
     {
+      /* Ignore result set changes if we are disposed */
+      if (renderer == null)
+        return;
+    
       var n_cols = renderer.get_n_cols ();
 
       if (bin_state == ExpandingBinState.UNEXPANDED)
 
=== modified file 'unity-private/places/places-default-renderer.vala'
--- unity-private/places/places-default-renderer.vala	2010-09-22 12:04:58 +0000
+++ unity-private/places/places-default-renderer.vala	2010-10-01 09:55:56 +0000
@@ -51,8 +51,18 @@
       Object ();
     }
 
-    ~DefaultRenderer ()
+    /* For some reason we must manually drop these in dispose() otherwise
+     * they end up leaked... And please note that finalize is not good enough
+     * either - so there is probably a ref cycle somewhere caused by all the
+     * actor.animate() calls... */
+    private override void dispose ()
     {
+      remove_actor (scroll);
+      remove_actor (search_empty);
+      remove_actor (section_empty);
+      
+      /* Chain up */
+      base.dispose ();
     }
 
     private static double
@@ -418,6 +428,7 @@
                   group.always_expanded = true;
                 }
             }
+          
         }
     }
 
 
=== modified file 'unity-private/places/places-view.vala'
--- unity-private/places/places-view.vala	2010-09-20 22:16:46 +0000
+++ unity-private/places/places-view.vala	2010-10-01 09:55:56 +0000
@@ -33,10 +33,10 @@
     private Ctk.VBox   content_box;
     private LayeredBin layered_bin;
 
-    public  PlaceHomeEntry       home_entry;
-    public  PlaceSearchBar       search_bar;
-    private Unity.Place.Renderer renderer;
-    private unowned PlaceEntry?  active_entry = null;
+    public  PlaceEntry            home_entry;
+    public  PlaceSearchBar        search_bar;
+    private Unity.Place.Renderer? renderer;
+    private unowned PlaceEntry?   active_entry = null;
 
     private bool is_showing = false;
 
@@ -81,7 +81,10 @@
       search_bar.entry.text.captured_event.connect (on_stage_event_captured);
       search_bar.entry.text.activatable = true;
       search_bar.entry.text.activate.connect (() => {
-        renderer.activate_default ();                                        
+        if (renderer is Clutter.Actor)
+          {
+            renderer.activate_default ();
+          }
       });
 
       layered_bin = new LayeredBin ();
@@ -107,6 +110,7 @@
           active_entry.active = false;
         }
       active_entry = null;
+      remove_current_renderer ();
     }
 
     public void on_entry_view_activated (PlaceEntry entry, uint section_id)
@@ -126,30 +130,34 @@
       entry.renderer_info_changed.connect (on_entry_renderer_info_changed);
     }
 
-    private void update_views (PlaceEntry entry, uint section_id=0)
+    /* Fade out previous results view if we had one, and set
+     * this.renderer = null */
+    private void remove_current_renderer ()
     {
-      search_bar.set_active_entry_view (entry, 0, section_id);
-
-      /* Create the correct results view */
       if (renderer is Clutter.Actor)
         {
-          
           var anim = renderer.animate (Clutter.AnimationMode.EASE_OUT_QUAD,
                                        300,
                                        "opacity", 0);
-          anim.completed.connect ((a)=> {
-              (a.get_object () as Clutter.Actor).destroy ();
-            });
+          
           layered_bin.remove_actor (renderer);
+          renderer = null;
         }
-
+    }
+
+    private void update_views (PlaceEntry entry, uint section_id=0)
+    {
+      search_bar.set_active_entry_view (entry, 0, section_id);
+
+      remove_current_renderer ();
+
+      /* Create the correct results view */
       renderer = lookup_renderer (entry);
+
       layered_bin.add_actor (renderer);
-      renderer.unref (); /* Because lookup_renderer returns it unfloating */
       renderer.opacity = 0;
       renderer.animate (Clutter.AnimationMode.EASE_OUT_QUAD, 300,
                         "opacity", 255);
-
       renderer.set_models (entry.entry_groups_model,
                            entry.entry_results_model,
                            entry.entry_renderer_hints);
 
=== modified file 'unity/unity-layered-bin.vala'
--- unity/unity-layered-bin.vala	2010-09-02 19:19:10 +0000
+++ unity/unity-layered-bin.vala	2010-10-01 09:55:56 +0000
@@ -28,9 +28,9 @@
    */
   public class LayeredBin : Ctk.Actor, Clutter.Container
   {
-    private GLib.List<Clutter.Actor?> _children = null;
+    private Gee.List<Clutter.Actor> _children = new Gee.LinkedList<Clutter.Actor>();
 
-    public unowned GLib.List<Clutter.Actor?> children {
+    public unowned Gee.List<Clutter.Actor> children {
       get { return _children; }
     }
 
@@ -39,12 +39,17 @@
       Object ();
     }
 
-    ~LayeredBin ()
+    private override void dispose ()
     {
       foreach (Clutter.Actor child in _children)
-        child.unparent ();
+        {
+          child.unparent ();
+        }
 
-      _children = null;
+      _children.clear ();
+      
+      /* Chain up */
+      base.dispose ();
     }
 
     construct
@@ -124,7 +129,7 @@
     private void add (Clutter.Actor actor)
     {
       actor.set_parent (this);
-      _children.append (actor);
+      _children.add (actor);
 
       queue_relayout ();
 
@@ -157,30 +162,30 @@
     private new void raise (Clutter.Actor actor, Clutter.Actor sibling)
     {
       if (sibling is Clutter.Actor &&
-          _children.index (sibling) != -1)
+          _children.index_of (sibling) != -1)
         {
           _children.remove (actor);
-          _children.insert (actor, _children.index (sibling) + 1);
+          _children.insert (_children.index_of (sibling) + 1, actor);
         }
       else
         {
           _children.remove (actor);
-          _children.append (actor);
+          _children.add (actor);
         }
     }
 
     private new void lower (Clutter.Actor actor, Clutter.Actor sibling)
     {
       if (sibling is Clutter.Actor &&
-          _children.index (sibling) != -1)
+          _children.index_of (sibling) != -1)
         {
           _children.remove (actor);
-          _children.insert (actor, _children.index (sibling) - 1);
+          _children.insert (_children.index_of (sibling) - 1, actor);
         }
       else
         {
           _children.remove (actor);
-          _children.prepend (actor);
+          _children.insert (0, actor);
         }
     }
 
 
=== modified file 'unity/unity-stripe-texture.vala'
--- unity/unity-stripe-texture.vala	2010-08-26 07:52:44 +0000
+++ unity/unity-stripe-texture.vala	2010-10-01 09:55:56 +0000
@@ -35,16 +35,16 @@
                                                    int           width,
                                                    int           height);
 
-    public float radius { get; construct set; }
-
     public StripeTextureOutlineFunc outline_paint_func;
 
     private Cairo.Surface? pattern;
 
     public StripeTexture (StripeTextureOutlineFunc? func)
     {
-      Object (radius:10.0f);
-
+      Object ();
+      
+      /* Must not point to a lambda or non-static method on 'this'.
+       * It causes self-refs if it does */
       outline_paint_func = rounded_outline;
       if (func != null)
         outline_paint_func = func;
@@ -55,8 +55,10 @@
       paint_func = paint_bg;
     }
 
-    public void rounded_outline (Cairo.Context cr, int width, int height)
+    /* This method must be static in order to avoid self refs */
+    public static void rounded_outline (Cairo.Context cr, int width, int height)
     {
+      float radius = 10;
       var x = 0;
       var y = 0;
       width -= 1;