From: Klaus Ethgen Date: Sun, 20 Jul 2014 12:57:40 +0000 (+0100) Subject: Merge remote-tracking branches 'merge-requests/6' and 'merge-requests/7' X-Git-Tag: v1.2~1 X-Git-Url: http://geeqie.org/cgi-bin/gitweb.cgi?p=geeqie.git;a=commitdiff_plain;h=d6c33614c45e38845a0bcfd702e38e18d5dcec0b;hp=e79d3547fb650e5f45cb9c30707724c221fdfc20 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 --- diff --git a/src/filedata.c b/src/filedata.c index 9c2ac8c2..c99f3a91 100644 --- a/src/filedata.c +++ b/src/filedata.c @@ -26,6 +26,10 @@ #include +#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. + *

+ * 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); + } +} /* *----------------------------------------------------------------------------- diff --git a/src/filedata.h b/src/filedata.h index 558b5926..43edc42f 100644 --- a/src/filedata.h +++ b/src/filedata.h @@ -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); diff --git a/src/lirc.c b/src/lirc.c index cbd09d4e..31c1b0b4 100644 --- a/src/lirc.c +++ b/src/lirc.c @@ -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 */ diff --git a/src/typedefs.h b/src/typedefs.h index 482c05a4..52068c9b 100644 --- a/src/typedefs.h +++ b/src/typedefs.h @@ -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; diff --git a/src/view_file_list.c b/src/view_file_list.c index c8a1cd6d..8d2d80c3 100644 --- a/src/view_file_list.c +++ b/src/view_file_list.c @@ -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); }