changeset 2239:7d1fe1247eda

Merge remote-tracking branches 'merge-requests/6' and 'merge-requests/7' * 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
author Klaus Ethgen <Klaus@Ethgen.de>
date Sun, 20 Jul 2014 13:57:40 +0100
parents de3ebbdacaa8 (current diff) 80779d942b27 (diff)
children d3a300d826d0
files src/filedata.c
diffstat 4 files changed, 201 insertions(+), 38 deletions(-) [+]
line wrap: on
line diff
--- a/src/filedata.c	Thu Dec 13 13:17:47 2012 +0100
+++ b/src/filedata.c	Sun Jul 20 13:57:40 2014 +0100
@@ -26,6 +26,10 @@
 
 #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 @@
 		}
 
 	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 @@
 {
 	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 @@
 	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 @@
 
 	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)
+
+	// Free FileData if it's no longer ref'd
+	file_data_consider_free(fd);
+}
+
+/**
+ * \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);
+
+	g_assert(fd->magick == FD_MAGICK);
+	fd->locked = TRUE;
+
+	DEBUG_2("file_data_ref fd=%p (%d): '%s'", fd, fd->ref, fd->path);
+}
+
+/**
+ * \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_assert(fd->magick == FD_MAGICK);
+	fd->locked = FALSE;
+
+	// 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)
 		{
-		GList *work;
-		FileData *parent = fd->parent ? fd->parent : fd;
-
-		if (parent->ref > 0) return;
-
-		work = parent->sidecar_files;
-		while (work)
-			{
-			FileData *sfd = work->data;
-			if (sfd->ref > 0) return;
-			work = work->next;
-			}
-
-		/* none of parent/children is referenced, we can free everything */
-
-		DEBUG_2("file_data_unref: 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);
+		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);
+		}
+}
 
 /*
  *-----------------------------------------------------------------------------
@@ -1150,8 +1262,11 @@
 	filelist_read_real(dir, &files, NULL, TRUE);
 
 	fd = g_hash_table_lookup(file_data_pool, path_utf8);
-	g_assert(fd);
-	file_data_ref(fd);
+	if (!fd) fd = file_data_new(path_utf8, &st, TRUE);
+	if (fd)
+		{
+		file_data_ref(fd);
+		}
 
 	filelist_free(files);
 	g_free(dir);
--- a/src/filedata.h	Thu Dec 13 13:17:47 2012 +0100
+++ b/src/filedata.h	Sun Jul 20 13:57:40 2014 +0100
@@ -43,6 +43,11 @@
 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);
--- a/src/typedefs.h	Thu Dec 13 13:17:47 2012 +0100
+++ b/src/typedefs.h	Sun Jul 20 13:57:40 2014 +0100
@@ -514,6 +514,7 @@
 
 	HistMap *histmap;
 
+	gboolean locked;
 	gint ref;
 	gint version; /* increased when any field in this structure is changed */
 	gboolean disable_grouping;
--- a/src/view_file_list.c	Thu Dec 13 13:17:47 2012 +0100
+++ b/src/view_file_list.c	Sun Jul 20 13:57:40 2014 +0100
@@ -148,9 +148,25 @@
 	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 @@
 
 	if (!vf->list)
 		{
-		vflist_store_clear(vf);
+		vflist_store_clear(vf, FALSE);
 		vf_send_update(vf);
 		return;
 		}
@@ -1686,6 +1702,20 @@
 		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 @@
 
 	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 @@
 	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 @@
 			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);
 }