Merge lp:~cmm2-deactivatedaccount/pantheon-files/memory-leak-fixes into lp:~elementary-apps/pantheon-files/trunk

[cmm2]

Improve memory utilization and reduce memory leaks.

This branch attempts to fix most of the file related memory leaks in Pantheon Files.

The user interface patch is still somewhat experimental and may have side effects on unexpected areas of the program. It breaks some previous inadvertent assumptions that file objects are forever resident in memory. However, most of those issues should be fixed.

Memory utilization comparisons are listed below.

Preparation:
    mkdir -p test/old test/new
    pushd test/old
    for i in $(seq 250000); do echo '' > $i; done
    popd
    ./pantheon-files

Procedure:
    1) Measure memory usage.
    2) Copy test/old to test/new/old.
    3) Measure memory usage.
    4) Create 10 files in test/new/ (this purges the undo manager history).
    5) Delete test/new/*.
    6) Repeat steps #1 through #5.

Results:
    http://pastebin.com/5RNvn4Jn was used for memory usage reporting.

    Comparison:
    http://pastebin.com/raw.php?i=cyBfMLtc

[cmm2 (cmm2-deactivatedaccount)]

My apologies for the delay in submitting these changes. I forgot about this for a while, until Jeremy removed me from the bug listing. I should've mentioned something earlier.

Shouldn't be anything too unusual with the patches. Of course, let me know if something goes wrong.

[Jeremy Wootten (jeremywootten)]

Thanks for continuing to work on this cmm2! The branch substantially reduces memory usage on copy and deleting 250000 files as described. On my system, one cycle of the test described increased memory usage by over 500MB whereas in the branch it increased by about 75MB (Using System Monitor). For comparison, Thunar's memory increased by about 100MB. PCManFM however, used no extra memory (!) provided the files were not displayed. Presumably some information is still cached in Files and Thunar so this is not necessarily a leak?

No problems with the code.

You inserted a TODO - were you going to do this in this branch or later?

I'd like to test the branch for regressions but subject to that I will approve. Thanks again.

[cmm2 (cmm2-deactivatedaccount)]

I need to look into it, but I suspect the initial 70-100M allocation is not actually used, but is simply freed memory that the allocator has not released to the OS. This explains why subsequent operations do not expand the total memory amount.

The blame for this additional memory requirement seems to fall on marlin_file_changes_consume and marlin_undo_manager (or whatever they are named -- sorry, I'm writing this from a tablet). Both can be optimized: file changes should go out immediately, and not queue up infinitely; and the undo tracker should remember user input, not a recursive list of all source files.

Some file managers don't track destination files for undoing if they belong to a created subdirectory, thus making their aftermath memory requirement effectively nil.

> You inserted a TODO - were you going to do this in this branch or later?

Probably sometime later. If the selected_files problem was causing known crashes I'd fix it, but right now it seems like a pedantic hypothetical.

[Jeremy Wootten (jeremywootten)]

I have not found any obvious regressions so I will approve this for merging now. Don't forget to claim your well-deserved bounty!

review: Approve

=== modified file 'libcore/gof-directory-async.vala'
--- libcore/gof-directory-async.vala	2015-03-06 03:36:39 +0000
+++ libcore/gof-directory-async.vala	2015-03-26 02:53:22 +0000
@@ -48,7 +48,7 @@
     private Cancellable cancellable;
     private FileMonitor? monitor = null;
 
-    private List<GOF.File>? sorted_dirs = null;
+    private List<unowned GOF.File>? sorted_dirs = null;
 
     public signal void file_loaded (GOF.File file);
     public signal void file_added (GOF.File file);
@@ -466,6 +466,25 @@
         file_hash.insert (gof.location, gof);
     }
 
+    public GOF.File file_cache_find_or_insert (GLib.File file,
+        bool update_hash = false)
+    {
+        GOF.File? result = file_hash.lookup (file);
+
+        if (result == null) {
+            result = GOF.File.cache_lookup (file);
+
+            if (result == null) {
+                result = new GOF.File (file, location);
+                file_hash.insert (file, result);
+            }
+            else if (update_hash)
+                file_hash.insert (file, result);
+        }
+
+        return (!) result;
+    }
+
     /* TODO move this to GOF.File */
     private delegate void func_query_info (GOF.File gof);
 
@@ -509,7 +528,9 @@
 
         if (!gof.is_hidden && gof.is_folder ()) {
             /* add to sorted_dirs */
-            sorted_dirs.insert_sorted (gof, GOF.File.compare_by_display_name);
+            if (sorted_dirs.find (gof) == null)
+                sorted_dirs.insert_sorted (gof,
+                    GOF.File.compare_by_display_name);
         }
 
         if (track_longest_name && gof.basename.length > longest_file_name.length) {
@@ -523,7 +544,6 @@
     }
 
     private void notify_file_added (GOF.File gof) {
-        file_hash.insert (gof.location, gof);
         query_info_async.begin (gof, add_and_refresh);
     }
 
@@ -533,6 +553,13 @@
 
         if (!gof.is_hidden && gof.is_folder ()) {
             /* remove from sorted_dirs */
+
+            /* Addendum note: GLib.List.remove() does not unreference objects.
+               See: https://bugzilla.gnome.org/show_bug.cgi?id=624249
+                    https://bugzilla.gnome.org/show_bug.cgi?id=532268
+
+               The declaration of sorted_dirs has been changed to contain
+               weak pointers as a temporary solution. */
             sorted_dirs.remove (gof);
         }
 
@@ -619,21 +646,23 @@
 
     public static void notify_files_changed (List<GLib.File> files) {
         foreach (var loc in files) {
-            GOF.File gof = GOF.File.get (loc);
-            Async? dir = cache_lookup (gof.directory);
+            Async? dir = cache_lookup_parent (loc);
 
-            if (dir != null)
+            if (dir != null) {
+                GOF.File gof = dir.file_cache_find_or_insert (loc);
                 dir.notify_file_changed (gof);
+            }
         }
     }
 
     public static void notify_files_added (List<GLib.File> files) {
         foreach (var loc in files) {
-            GOF.File gof = GOF.File.get (loc);
-            Async? dir = cache_lookup (gof.directory);
+            Async? dir = cache_lookup_parent (loc);
 
-            if (dir != null)
+            if (dir != null) {
+                GOF.File gof = dir.file_cache_find_or_insert (loc, true);
                 dir.notify_file_added (gof);
+            }
         }
     }
 
@@ -642,10 +671,10 @@
         bool found;
 
         foreach (var loc in files) {
-            GOF.File gof = GOF.File.get (loc);
-            Async? dir = cache_lookup (gof.directory);
+            Async? dir = cache_lookup_parent (loc);
 
             if (dir != null) {
+                GOF.File gof = dir.file_cache_find_or_insert (loc);
                 dir.notify_file_removed (gof);
                 found = false;
 
@@ -725,6 +754,11 @@
         return cached_dir;
     }
 
+    public static Async? cache_lookup_parent (GLib.File file) {
+        GLib.File? parent = file.get_parent ();
+        return parent != null ? cache_lookup (parent) : cache_lookup (file);
+    }
+
     public bool remove_dir_from_cache () {
         /* we got to increment the dir ref to remove the toggle_ref */
         this.ref ();

=== modified file 'libcore/gof-file.c'
--- libcore/gof-file.c	2015-02-27 16:40:41 +0000
+++ libcore/gof-file.c	2015-03-26 02:53:22 +0000
@@ -114,6 +114,7 @@
     file->basename = g_file_get_basename (file->location);
     //file->parent_dir = g_file_enumerator_get_container (enumerator);
 
+    //g_debug ("%s: create %p", __func__, file);
     return (file);
 }
 
@@ -946,6 +947,8 @@
 }
 
 static void gof_file_finalize (GObject* obj) {
+    //g_debug ("%s: delete %p", __func__, obj);
+
     GOFFile *file;
 
     file = GOF_FILE (obj);
@@ -977,6 +980,13 @@
     /* TODO remove the target_gof */
     _g_free0 (file->thumbnail_path);
 
+#ifndef NDEBUG
+    g_warn_if_fail (file->target_gof == NULL);
+#endif
+
+    _g_free0 (file->owner);
+    _g_free0 (file->group);
+
     G_OBJECT_CLASS (gof_file_parent_class)->finalize (obj);
 }
 

=== modified file 'libcore/marlin-file-operations.c'
--- libcore/marlin-file-operations.c	2015-03-01 10:56:59 +0000
+++ libcore/marlin-file-operations.c	2015-03-26 02:53:22 +0000
@@ -4002,19 +4002,23 @@
 static gboolean
 test_dir_is_parent (GFile *child, GFile *root)
 {
-    GFile *f;
-
-    f = g_file_dup (child);
-    while (f) {
+    GFile *f = child;
+    GFile *prev = NULL;
+
+    if (g_file_equal (child, root))
+        return TRUE;
+
+    while ((f = g_file_get_parent (f))) {
+        if (prev) g_object_unref (prev);
+
         if (g_file_equal (f, root)) {
             g_object_unref (f);
             return TRUE;
         }
-        f = g_file_get_parent (f);
-    }
-    if (f) {
-        g_object_unref (f);
-    }
+        prev = f;
+    }
+    if (prev) g_object_unref (prev);
+
     return FALSE;
 }
 

=== modified file 'libcore/pantheon-files-core-C.vapi'
--- libcore/pantheon-files-core-C.vapi	2015-03-01 10:56:59 +0000
+++ libcore/pantheon-files-core-C.vapi	2015-03-26 02:53:22 +0000
@@ -31,7 +31,7 @@
         public void add_file(GOF.File file, GOF.Directory.Async dir);
         public void remove_file (GOF.File file, GOF.Directory.Async dir);
         public void file_changed (GOF.File file, GOF.Directory.Async dir);
-        public unowned GOF.File file_for_path(Gtk.TreePath path);
+        public GOF.File? file_for_path (Gtk.TreePath path);
         public static GLib.Type get_type ();
         public bool get_first_iter_for_file (GOF.File file, out Gtk.TreeIter iter);
         public bool get_tree_iter_from_file (GOF.File file, GOF.Directory.Async directory, out Gtk.TreeIter iter);

=== modified file 'src/MimeActions.vala'
--- src/MimeActions.vala	2015-02-06 22:57:55 +0000
+++ src/MimeActions.vala	2015-03-26 02:53:22 +0000
@@ -39,7 +39,7 @@
     public static AppInfo? get_default_application_for_files (GLib.List<unowned GOF.File> files) {
         assert (files != null);
         /* Need to make a new list to avoid corrupting the selection */
-        unowned GLib.List<GOF.File> sorted_files = null;
+        GLib.List<unowned GOF.File> sorted_files = null;
         files.@foreach ((file) => {
             sorted_files.prepend (file);
         });
@@ -120,7 +120,7 @@
     public static List<AppInfo>? get_applications_for_files (GLib.List<unowned GOF.File> files) {
         assert (files != null);
         /* Need to make a new list to avoid corrupting the selection */
-        unowned GLib.List<GOF.File> sorted_files = null;
+        GLib.List<unowned GOF.File> sorted_files = null;
         files.@foreach ((file) => {
             sorted_files.prepend (file);
         });

=== modified file 'src/TextRenderer.vala'
--- src/TextRenderer.vala	2014-11-04 14:51:14 +0000
+++ src/TextRenderer.vala	2015-03-26 02:53:22 +0000
@@ -25,7 +25,7 @@
         public Marlin.ZoomLevel zoom_level {get; set;}
         public bool follow_state {get; set;}
         public new string background { set; private get;}
-        public GOF.File file {set; private get;}
+        public GOF.File? file {set; private get;}
         public int text_width;
         public int text_height;
 
@@ -91,6 +91,11 @@
                                          layout);
 
             style_context.restore ();
+
+            /* The render call should always be preceded by a set_property call
+               from GTK. It should be safe to unreference or free the allocated
+               memory here. */
+            file = null;
         }
 
         public void set_up_layout (string? text, Gdk.Rectangle cell_area) {
@@ -211,6 +216,7 @@
 
         private void invalidate () {
             set_widget (null);
+            file = null;
         }
 
         private void on_entry_editing_done () {
@@ -224,6 +230,7 @@
                 string path = entry.get_data ("marlin-text-renderer-path");
                 edited (path, text);
             }
+            file = null;
         }
 
         private bool on_entry_focus_out_event (Gdk.Event event) {

=== modified file 'src/View/AbstractDirectoryView.vala'
--- src/View/AbstractDirectoryView.vala	2015-03-25 08:59:04 +0000
+++ src/View/AbstractDirectoryView.vala	2015-03-26 02:53:22 +0000
@@ -192,7 +192,27 @@
 
         private GLib.List<GLib.AppInfo> open_with_apps;
         protected GLib.List<GOF.Directory.Async>? loaded_subdirectories = null;
-        protected GLib.List<unowned GOF.File> selected_files = null ;
+
+        /* TODO: Remove the "unowned" portion of the declaration for
+                 selected_files and on code that repeats its type.
+
+                 Selected files are originally obtained with
+                 gtk_tree_model_get(): this function increases the reference
+                 count of the file object.
+
+                 In order to prevent the obvious memory leak when inserting
+                 these owned objects into the unowned element container, the
+                 objects are unreferenced upon being inserted.
+
+                 This results in a container filled with weak references to
+                 objects with reference counts of 1.
+
+                 A scenario may occur that the cell holding the original file
+                 object is destroyed, and the container is left with pointers
+                 pointing to freed memory. Reference counting the container
+                 elements would prevent this possibility. */
+        protected GLib.List<unowned GOF.File> selected_files = null;
+
         private GLib.List<unowned GOF.File>? templates = null;
 
         private GLib.AppInfo default_app;

=== modified file 'src/View/AbstractTreeView.vala'
--- src/View/AbstractTreeView.vala	2015-02-12 18:28:34 +0000
+++ src/View/AbstractTreeView.vala	2015-03-26 02:53:22 +0000
@@ -163,7 +163,7 @@
             selected_files = null;
 
             tree.get_selection ().selected_foreach ((model, path, iter) => {
-                unowned GOF.File? file; /* can be null if click on blank row in list view */
+                GOF.File? file; /* can be null if click on blank row in list view */
                 model.@get (iter, FM.ListModel.ColumnID.FILE_COLUMN, out file, -1);
                 if (file != null)
                     selected_files.prepend (file);

=== modified file 'src/View/IconView.vala'
--- src/View/IconView.vala	2015-02-12 18:28:34 +0000
+++ src/View/IconView.vala	2015-03-26 02:53:22 +0000
@@ -172,7 +172,7 @@
             selected_files = null;
 
             tree.selected_foreach ((tree, path) => {
-                unowned GOF.File file;
+                GOF.File? file;
                 file = model.file_for_path (path);
 
                 if (file != null)

=== modified file 'src/View/ListView.vala'
--- src/View/ListView.vala	2015-01-24 12:31:20 +0000
+++ src/View/ListView.vala	2015-03-26 02:53:22 +0000
@@ -89,7 +89,7 @@
         }
 
         private void set_path_expanded (Gtk.TreePath path, bool expanded) {
-            unowned GOF.File? file = model.file_for_path (path);
+            GOF.File? file = model.file_for_path (path);
 
             if (file != null)
                 file.set_expanded (expanded);

=== modified file 'src/View/PropertiesWindow.vala'
--- src/View/PropertiesWindow.vala	2015-03-06 03:36:39 +0000
+++ src/View/PropertiesWindow.vala	2015-03-26 02:53:22 +0000
@@ -40,7 +40,7 @@
 
     private uint count;
     private GLib.List<GOF.File> files;
-    private GOF.File goffile;
+    private unowned GOF.File goffile;
     private FM.AbstractDirectoryView view;
 
     private Gee.Set<string>? mimes;
@@ -105,7 +105,18 @@
         }
 
         view = _view;
-        files = _files.copy ();
+
+        /* The properties window may outlive the passed-in file object
+           lifetimes. The objects must be referenced as a precaution.
+
+           GLib.List.copy() would not guarantee valid references: because it
+           does a shallow copy (copying the pointer values only) the objects'
+           memory may be freed even while this code is using it. */
+        foreach (unowned GOF.File file in _files)
+            /* prepend(G) is declared "owned G", so ref() will be called once
+               on the unowned foreach value. */
+            files.prepend (file);
+
         count = files.length();
 
         if (count < 1 ) {

=== modified file 'src/marlin-icon-renderer.c'
--- src/marlin-icon-renderer.c	2015-01-18 20:46:03 +0000
+++ src/marlin-icon-renderer.c	2015-03-26 02:53:22 +0000
@@ -661,6 +661,12 @@
             g_object_unref (pix);
         }
     }
+
+    /* The render call should always be preceded by a set_property call from
+       GTK. It should be safe to unreference or free the allocate memory
+       here. */
+    _g_object_unref0 (priv->file);
+    _g_object_unref0 (priv->drop_file);
 }
 
 /*