Merge remote-tracking branches 'merge-requests/6' and 'merge-requests/7'
authorKlaus Ethgen <Klaus@Ethgen.de>
Sun, 20 Jul 2014 12:57:40 +0000 (13:57 +0100)
committerKlaus Ethgen <Klaus@Ethgen.de>
Sun, 20 Jul 2014 12:57:40 +0000 (13:57 +0100)
* merge-requests/6:
  Bug 3594998: make lirc initialization quieter

* merge-requests/7:
  Add filedata counting to watch for filedata leaks
  Use FileData locks to avoid expensive reloads with marks enabled
  Add "lock" functionality to keep FileDatas in memory

src/filedata.c
src/filedata.h
src/lirc.c
src/typedefs.h
src/view_file_list.c

index 9c2ac8c..c99f3a9 100644 (file)
 
 #include <errno.h>
 
+#ifdef DEBUG_FILEDATA
+gint global_file_data_count = 0;
+#endif
+
 static GHashTable *file_data_pool = NULL;
 static GHashTable *file_data_planned_change_hash = NULL;
 
@@ -383,6 +387,10 @@ static FileData *file_data_new(const gchar *path_utf8, struct stat *st, gboolean
                }
 
        fd = g_new0(FileData, 1);
+#ifdef DEBUG_FILEDATA
+       global_file_data_count++;
+       DEBUG_2("file data count++: %d", global_file_data_count);
+#endif
 
        fd->size = st->st_size;
        fd->date = st->st_mtime;
@@ -532,6 +540,12 @@ static void file_data_free(FileData *fd)
 {
        g_assert(fd->magick == FD_MAGICK);
        g_assert(fd->ref == 0);
+       g_assert(!fd->locked);
+
+#ifdef DEBUG_FILEDATA
+       global_file_data_count--;
+       DEBUG_2("file data count--: %d", global_file_data_count);
+#endif
 
        metadata_cache_free(fd);
        g_hash_table_remove(file_data_pool, fd->original_path);
@@ -549,6 +563,57 @@ static void file_data_free(FileData *fd)
        g_free(fd);
 }
 
+/**
+ * \brief Checks if the FileData is referenced
+ *
+ * Checks the refcount and whether the FileData is locked.
+ */
+static gboolean file_data_check_has_ref(FileData *fd)
+{
+       return fd->ref > 0 || fd->locked;
+}
+
+/**
+ * \brief Consider freeing a FileData.
+ *
+ * This function will free a FileData and its children provided that neither its parent nor it has
+ * a positive refcount, and provided that neither is locked.
+ */
+static void file_data_consider_free(FileData *fd)
+{
+       GList *work;
+       FileData *parent = fd->parent ? fd->parent : fd;
+
+       g_assert(fd->magick == FD_MAGICK);
+       if (file_data_check_has_ref(fd)) return;
+       if (file_data_check_has_ref(parent)) return;
+
+       work = parent->sidecar_files;
+       while (work)
+               {
+               FileData *sfd = work->data;
+               if (file_data_check_has_ref(sfd)) return;
+               work = work->next;
+               }
+
+       /* Neither the parent nor the siblings are referenced, so we can free everything */
+       DEBUG_2("file_data_consider_free: deleting '%s', parent '%s'",
+               fd->path, fd->parent ? parent->path : "-");
+
+       work = parent->sidecar_files;
+       while (work)
+               {
+               FileData *sfd = work->data;
+               file_data_free(sfd);
+               work = work->next;
+               }
+
+       g_list_free(parent->sidecar_files);
+       parent->sidecar_files = NULL;
+
+       file_data_free(parent);
+}
+
 #ifdef DEBUG_FILEDATA
 void file_data_unref_debug(const gchar *file, gint line, FileData *fd)
 #else
@@ -566,45 +631,92 @@ void file_data_unref(FileData *fd)
 
        fd->ref--;
 #ifdef DEBUG_FILEDATA
-       DEBUG_2("file_data_unref fd=%p (%d): '%s' @ %s:%d", fd, fd->ref, fd->path, file, line);
+       DEBUG_2("file_data_unref fd=%p (%d:%d): '%s' @ %s:%d", fd, fd->ref, fd->locked, fd->path,
+               file, line);
 #else
-       DEBUG_2("file_data_unref fd=%p (%d): '%s'", fd, fd->ref, fd->path);
+       DEBUG_2("file_data_unref fd=%p (%d:%d): '%s'", fd, fd->ref, fd->locked, fd->path);
 #endif
-       if (fd->ref == 0)
-               {
-               GList *work;
-               FileData *parent = fd->parent ? fd->parent : fd;
 
-               if (parent->ref > 0) return;
+       // Free FileData if it's no longer ref'd
+       file_data_consider_free(fd);
+}
 
-               work = parent->sidecar_files;
-               while (work)
-                       {
-                       FileData *sfd = work->data;
-                       if (sfd->ref > 0) return;
-                       work = work->next;
-                       }
+/**
+ * \brief Lock the FileData in memory.
+ *
+ * This allows the caller to prevent a FileData from being freed, even after its refcount is zero.
+ * This is intended to be used in cases where a FileData _should_ stay in memory as an optimization,
+ * even if the code would continue to function properly even if the FileData were freed.  Code that
+ * _requires_ the FileData to remain in memory should continue to use file_data_(un)ref.
+ * <p />
+ * Note: This differs from file_data_ref in that the behavior is reentrant -- after N calls to
+ * file_data_lock, a single call to file_data_unlock will unlock the FileData.
+ */
+void file_data_lock(FileData *fd)
+{
+       if (fd == NULL) return;
+       if (fd->magick != FD_MAGICK) DEBUG_0("fd magick mismatch fd=%p", fd);
 
-               /* none of parent/children is referenced, we can free everything */
+       g_assert(fd->magick == FD_MAGICK);
+       fd->locked = TRUE;
 
-               DEBUG_2("file_data_unref: deleting '%s', parent '%s'", fd->path, fd->parent ? parent->path : "-");
+       DEBUG_2("file_data_ref fd=%p (%d): '%s'", fd, fd->ref, fd->path);
+}
 
-               work = parent->sidecar_files;
-               while (work)
-                       {
-                       FileData *sfd = work->data;
-                       file_data_free(sfd);
-                       work = work->next;
-                       }
+/**
+ * \brief Reset the maintain-FileData-in-memory lock
+ *
+ * This again allows the FileData to be freed when its refcount drops to zero.  Automatically frees
+ * the FileData if its refcount is already zero (which will happen if the lock is the only thing
+ * keeping it from being freed.
+ */
+void file_data_unlock(FileData *fd)
+{
+       if (fd == NULL) return;
+       if (fd->magick != FD_MAGICK) DEBUG_0("fd magick mismatch fd=%p", fd);
 
-               g_list_free(parent->sidecar_files);
-               parent->sidecar_files = NULL;
+       g_assert(fd->magick == FD_MAGICK);
+       fd->locked = FALSE;
 
-               file_data_free(parent);
+       // Free FileData if it's no longer ref'd
+       file_data_consider_free(fd);
+}
+
+/**
+ * \brief Lock all of the FileDatas in the provided list
+ *
+ * \see file_data_lock(FileData)
+ */
+void file_data_lock_list(GList *list)
+{
+       GList *work;
+
+       work = list;
+       while (work)
+               {
+               FileData *fd = work->data;
+               work = work->next;
+               file_data_lock(fd);
                }
 }
 
+/**
+ * \brief Unlock all of the FileDatas in the provided list
+ *
+ * \see file_data_unlock(FileData)
+ */
+void file_data_unlock_list(GList *list)
+{
+       GList *work;
 
+       work = list;
+       while (work)
+               {
+               FileData *fd = work->data;
+               work = work->next;
+               file_data_unlock(fd);
+               }
+}
 
 /*
  *-----------------------------------------------------------------------------
index 558b592..43edc42 100644 (file)
@@ -43,6 +43,11 @@ FileData *file_data_ref(FileData *fd);
 void file_data_unref(FileData *fd);
 #endif
 
+void file_data_lock(FileData *fd);
+void file_data_unlock(FileData *fd);
+void file_data_lock_list(GList *list);
+void file_data_unlock_list(GList *list);
+
 gboolean file_data_check_changed_files(FileData *fd);
 
 void file_data_increment_version(FileData *fd);
index cbd09d4..31c1b0b 100644 (file)
@@ -204,30 +204,37 @@ static gboolean lirc_input_callback(GIOChannel *source, GIOCondition condition,
 void layout_image_lirc_init(LayoutWindow *lw)
 {
        gint flags;
+       gboolean lirc_verbose = (get_debug_level() >= 2);
 
-       DEBUG_1("Initializing LIRC...");
-       lirc_fd = lirc_init(GQ_APPNAME_LC, get_debug_level() > 0);
+       lirc_fd = lirc_init(GQ_APPNAME_LC, lirc_verbose);
        if (lirc_fd == -1)
                {
-               g_fprintf(stderr, _("Could not init LIRC support\n"));
+               DEBUG_1("Initializing LIRC... failed");
                return;
                }
+       
+       DEBUG_1("Initializing LIRC... OK");
        if (lirc_readconfig(NULL, &config, NULL) == -1)
                {
                lirc_deinit();
+               
                g_fprintf(stderr,
                        _("could not read LIRC config file\n"
                        "please read the documentation of LIRC to \n"
                        "know how to create a proper config file\n"));
+               fflush(stderr);
+
+               DEBUG_1("Failed to read LIRC config file");
                return;
                }
+       if (lirc_verbose) fflush(stderr);
+
        gio_chan = g_io_channel_unix_new(lirc_fd);
        input_tag = g_io_add_watch(gio_chan, G_IO_IN,
                                   lirc_input_callback, lw);
        fcntl(lirc_fd, F_SETOWN, getpid());
        flags = fcntl(lirc_fd, F_GETFL, 0);
        if (flags != -1) fcntl(lirc_fd, F_SETFL, flags|O_NONBLOCK);
-       fflush(stderr);
 }
 
 #endif /* HAVE_LIRC */
index 482c05a..52068c9 100644 (file)
@@ -514,6 +514,7 @@ struct _FileData {
 
        HistMap *histmap;
 
+       gboolean locked;
        gint ref;
        gint version; /* increased when any field in this structure is changed */
        gboolean disable_grouping;
index c8a1cd6..8d2d80c 100644 (file)
@@ -148,9 +148,25 @@ static gboolean vflist_store_clear_cb(GtkTreeModel *model, GtkTreePath *path, Gt
        return FALSE;
 }
 
-static void vflist_store_clear(ViewFile *vf)
+static void vflist_store_clear(ViewFile *vf, gboolean unlock_files)
 {
        GtkTreeModel *store;
+       GList *files = NULL;
+
+       if (unlock_files && vf->marks_enabled)
+               {
+               // unlock locked files in this directory
+               filelist_read(vf->dir_fd, &files, NULL);
+               while (files)
+                       {
+                       FileData *fd = files->data;
+                       files = files->next;
+                       file_data_unlock(fd);
+                       file_data_unref(fd);  // undo the ref that got added in filelist_read
+                       }
+               }
+
+       g_list_free(files);
        store = gtk_tree_view_get_model(GTK_TREE_VIEW(vf->listview));
        gtk_tree_model_foreach(store, vflist_store_clear_cb, NULL);
        gtk_tree_store_clear(GTK_TREE_STORE(store));
@@ -1649,7 +1665,7 @@ static void vflist_populate_view(ViewFile *vf, gboolean force)
 
        if (!vf->list)
                {
-               vflist_store_clear(vf);
+               vflist_store_clear(vf, FALSE);
                vf_send_update(vf);
                return;
                }
@@ -1686,6 +1702,20 @@ gboolean vflist_refresh(ViewFile *vf)
                file_data_unregister_notify_func(vf_notify_cb, vf); /* we don't need the notification of changes detected by filelist_read */
 
                ret = filelist_read(vf->dir_fd, &vf->list, NULL);
+
+               if (vf->marks_enabled)
+                       {
+                       // When marks are enabled, lock FileDatas so that we don't end up re-parsing XML
+                       // each time a mark is changed.
+                       file_data_lock_list(vf->list);
+                       }
+               else
+                       {
+                       // FIXME: only do this when needed (aka when we just switched from
+                       // FIXME: marks-enabled to marks-disabled)
+                       file_data_unlock_list(vf->list);
+                       }
+
                vf->list = file_data_filter_marks_list(vf->list, vf_marks_get_filter(vf));
                file_data_register_notify_func(vf_notify_cb, vf, NOTIFY_PRIORITY_MEDIUM);
 
@@ -1697,6 +1727,8 @@ gboolean vflist_refresh(ViewFile *vf)
 
        vflist_populate_view(vf, FALSE);
 
+       DEBUG_1("%s vflist_refresh: free filelist", get_exec_time());
+
        filelist_free(old_list);
        DEBUG_1("%s vflist_refresh: done", get_exec_time());
 
@@ -1868,7 +1900,7 @@ gboolean vflist_set_fd(ViewFile *vf, FileData *dir_fd)
        vf->dir_fd = file_data_ref(dir_fd);
 
        /* force complete reload */
-       vflist_store_clear(vf);
+       vflist_store_clear(vf, TRUE);
 
        filelist_free(vf->list);
        vf->list = NULL;
@@ -1995,6 +2027,16 @@ void vflist_marks_set(ViewFile *vf, gboolean enable)
                        gtk_tree_view_column_set_visible(column, enable);
                }
 
+       if (enable)
+               {
+               // Previously disabled, which means that vf->list is complete
+               file_data_lock_list(vf->list);
+               }
+       else
+               {
+               // Previously enabled, which means that vf->list is incomplete
+               }
+
        g_list_free(columns);
 }