Extremely laggy file move. [patch attached]

[installgentoo]

Setup (please complete the following information):

• Distribution: gentoo
• Distribution release: compiled last week
• Geeqie version [geeqie --version]: Geeqie 2.1 GTK3

Describe the bug Super laggy file move.

To reproduce Move any image into a folder with lots of images(tens of thousands). Your entire UI now lags extremely badly. Notice that this can be fixed by removing

    g_signal_connect(G_OBJECT(fdlg->entry), "changed",
             G_CALLBACK(file_util_dest_folder_entry_cb), ud);

at geeqie-2.1/src/utilops.cc:1619

I've no clue what that callback actually does. My hunch is it moves thumbnails around. In any case, i suggest just spawning a new thread there, and placing callback into that thread.

Expected behavior I expect it not to lag. Like, c'mon. "Laggiest image viewer"

Screenshots

Image sources Just get 50k images off safebooru to test on

Error logs or seg. fault files

Additional context

[installgentoo]

Okay i tested this, doesn't seem to crash anything, shouldn't have races. Here's the patch

--- a/src/utilops.cc
+++ b/src/utilops.cc
@@ -35,6 +35,7 @@
 #include "metadata.h"
 #include "exif.h"

+#include <thread>
 #define DIALOG_WIDTH 750

 static GdkPixbuf *file_util_get_error_icon(FileData *fd, GList *list, GtkWidget *widget);
@@ -1202,8 +1203,10 @@

 static void file_util_dest_folder_entry_cb(GtkWidget *UNUSED(entry), gpointer data)
 {
+   std::thread([=] {
    auto ud = static_cast<UtilityData *>(data);
    file_util_dest_folder_update_path(ud);
+   }).detach();
 }

 /* format: * = filename without extension, ## = number position, extension is kept */

[WojciechMula]

A detached thread will be unconditionally terminated when Geeqie process is closed before moving is done.

A perfect solution would be a thread that gets commands from other components and perform some possibly long-running operation and that can be queried by the main thread about progress/pending ops.

[installgentoo]

Another offender is /tmp/tmp/1/geeqie-2.4/src/filedata.cc:1543 coming from /tmp/tmp/1/geeqie-2.4/src/thumb-standard.cc:1126

I'm not sure why geeqie needs to reread the moved files in order to move thumbnails, since code is impenetrable, but the point is, it lags as fuck.

[installgentoo]

@caclark Okay, this patch fixes a very major source of lags on filemove. I've tried to use tl to figure out appropriate folder, but the code is impenetrable. tl seems to require to be used to load file or it callbacks success and segfaults the whole thing. I suggest throwing out all the validate logic, or rather, any callbacks. If something is using a callback/glib it's not worth it. Case in point, bugs and lags in every release, there's no point to validate bugs. Moreover, i can't even understand what that ``validation" is really for, in the code that just should just rename the thumbnail with file.

Simplify this codebase. Please.

--- a/src/thumb-standard.cc
+++ b/src/thumb-standard.cc
@@ -1099,87 +1097,42 @@
 static gboolean thumb_std_maint_move_idle(gpointer data);

-static void thumb_std_maint_move_validate_cb(const gchar *, gboolean, gpointer data)
-{
-   auto tm = static_cast<TMaintMove *>(data);
-   GdkPixbuf *pixbuf;
-
-   /* get the original thumbnail pixbuf (unrotated, with original options)
-      this is called from image_loader done callback, so tm->tl->il must exist*/
-   pixbuf = image_loader_get_pixbuf(tm->tl->il);
-   if (pixbuf)
-       {
-       const gchar *uri;
-       const gchar *mtime_str;
-
-       uri = gdk_pixbuf_get_option(pixbuf, THUMB_MARKER_URI);
-       mtime_str = gdk_pixbuf_get_option(pixbuf, THUMB_MARKER_MTIME);
-
-       if (uri && mtime_str && strcmp(uri, tm->source_uri) == 0)
-           {
-           gchar *pathl;
-
-           /* The validation utility abuses ThumbLoader, and we
-            * abuse the utility just to load the thumbnail,
-            * but the loader needs to look sane for the save to complete.
-            */
-
-           tm->tl->cache_enable = TRUE;
-           tm->tl->cache_hit = FALSE;
-           tm->tl->cache_local = FALSE;
-           file_data_unref(tm->tl->fd);
-           tm->tl->fd = file_data_new_group(tm->dest);
-           tm->tl->source_mtime = strtol(mtime_str, nullptr, 10);
-
-           pathl = path_from_utf8(tm->tl->fd->path);
-           g_free(tm->tl->thumb_uri);
-           tm->tl->thumb_uri = g_filename_to_uri(pathl, nullptr, nullptr);
-           tm->tl->local_uri = filename_from_path(tm->tl->thumb_uri);
-           g_free(pathl);
-
-           g_free(tm->tl->thumb_path);
-           tm->tl->thumb_path = nullptr;
-           tm->tl->thumb_path_local = FALSE;
-
-           DEBUG_1("thumb move attempting save:");
-
-           thumb_loader_std_save(tm->tl, pixbuf);
-           }
-
-       DEBUG_1("thumb move unlink: %s", tm->thumb_path);
-       unlink_file(tm->thumb_path);
-       }
-
-   thumb_std_maint_move_step(tm);
-}
-
 static void thumb_std_maint_move_step(TMaintMove *tm)
 {
-   const gchar *folder;
-
-   tm->pass++;
-   if (tm->pass > 2)
-       {
+   if (tm->dest && tm->source)
+   {
+       DEBUG_1("thumb move attempting rename:");
+
+       auto* uri = g_filename_to_uri(tm->source, nullptr, nullptr);
+       auto* new_uri = g_filename_to_uri(tm->dest, nullptr, nullptr);
+       auto* thumb_path = thumb_std_cache_path(tm->source, uri, FALSE, THUMB_FOLDER_NORMAL);
+       auto* new_thumb_path = thumb_std_cache_path(tm->dest, new_uri, FALSE, THUMB_FOLDER_NORMAL);
+
+       gboolean success = rename_file(thumb_path, new_thumb_path);
+
+       if (!success)
+           {
+           DEBUG_1("thumb move failed: %s", tm->dest);
+           DEBUG_1("            thumb: %s", new_thumb_path);
+           }
+
+       g_free(uri);
+       g_free(new_uri);
+       g_free(thumb_path);
+       g_free(new_thumb_path);
+
        g_free(tm->source);
        g_free(tm->dest);
        g_free(tm->source_uri);
        g_free(tm->thumb_path);
        g_free(tm);

-       if (thumb_std_maint_move_list)
-           {
-           g_idle_add_full(G_PRIORITY_LOW, thumb_std_maint_move_idle, nullptr, nullptr);
-           }
-
-       return;
-       }
-
-   folder = (tm->pass == 1) ? THUMB_FOLDER_NORMAL : THUMB_FOLDER_LARGE;
-
-   g_free(tm->thumb_path);
-   tm->thumb_path = thumb_std_cache_path(tm->source, tm->source_uri, FALSE, folder);
-   tm->tl = thumb_loader_std_thumb_file_validate(tm->thumb_path, 0,
-                             thumb_std_maint_move_validate_cb, tm);
+   }
+
+   if (thumb_std_maint_move_list)
+   {
+   g_idle_add_full(G_PRIORITY_LOW, thumb_std_maint_move_idle, nullptr, nullptr);
+   }
 }

 static gboolean thumb_std_maint_move_idle(gpointer)

If you know how to initialise tl and not crash the code, it's very easy to use rename_file instead of thumb_loader_std_save and you can do

@@ -177,41 +177,44 @@
    return result;
 }

-static gchar *thumb_loader_std_cache_path(ThumbLoaderStd *tl, gboolean local, GdkPixbuf *pixbuf, gboolean fail)
-{
-   const gchar *folder;
+static gchar const* thumb_determine_folder(ThumbLoaderStd const *tl, GdkPixbuf *pixbuf, gboolean fail)
+{
    gint w;
    gint h;

-   if (!tl->fd || !tl->thumb_uri) return nullptr;
-
    if (pixbuf)
-       {
+   {
        w = gdk_pixbuf_get_width(pixbuf);
        h = gdk_pixbuf_get_height(pixbuf);
-       }
+   }
    else
-       {
+   {
        w = tl->requested_width;
        h = tl->requested_height;
-       }
+   }

    if (fail)
-       {
-       folder = THUMB_FOLDER_FAIL;
-       }
-   else if (w > THUMB_SIZE_NORMAL || h > THUMB_SIZE_NORMAL)
-       {
-       folder = THUMB_FOLDER_LARGE;
-       }
-   else
-       {
-       folder = THUMB_FOLDER_NORMAL;
-       }
+   {
+       return THUMB_FOLDER_FAIL;
+   }
+
+   if (w > THUMB_SIZE_NORMAL || h > THUMB_SIZE_NORMAL)
+   {
+       return THUMB_FOLDER_LARGE;
+   }
+
+   return THUMB_FOLDER_NORMAL;
+}
+
+static gchar *thumb_loader_std_cache_path(ThumbLoaderStd *tl, gboolean local, GdkPixbuf *pixbuf, gboolean fail)
+{
+   if (!tl->fd || !tl->thumb_uri) return nullptr;
+
+   const gchar *folder = thumb_determine_folder(tl, pixbuf, fail);

    return thumb_std_cache_path(tl->fd->path,
-                   (local) ?  tl->local_uri : tl->thumb_uri,
-                   local, folder);
+                   (local) ?  tl->local_uri : tl->thumb_uri,
+                   local, folder);
 }