Merge lp://qastaging/~cmm2-deactivatedaccount/pantheon-files/fix-1404588 into lp://qastaging/~elementary-apps/pantheon-files/trunk

I can confirm that this fixes the slow copying. Thanks!

However, this looks like it's going to break undo because you've reversed the order of the items in the list but never updated the undo/redo code to account for that.

> However, this looks like it's going to break undo because you've reversed the
> order of the items in the list but never updated the undo/redo code to account
> for that.

I don't believe this to be the case. The instances where construct_gfile_list is called with 'action->sources' or 'action->destinations' are unaffected due to multiple-of-two calls to g_list_prepend. The remaining instances all involve the trash, where the ordering logic is already buggy, because g_hash_table_get_keys does not guarantee the returned keys will be in the original insertion order.

This can be demonstrated with the following program:

   #include <stdio.h>
   #include <glib.h>

   void print_cb(gpointer data, gpointer) {
      const char* kv = (const char*) data;
      printf("%s\n", kv);
   }

   int main() {
      GHashTable* ht = g_hash_table_new(g_str_hash, g_str_equal);
      g_hash_table_insert(ht, "key1", "value1");
      g_hash_table_insert(ht, "key2", "value2");
      g_hash_table_insert(ht, "key3", "value3");
      GList* kl = g_hash_table_get_keys(ht);
      g_list_foreach(kl, (GFunc) print_cb, NULL);
      return 0;
   }

   $ gcc $(pkg-config glib-2.0 --cflags --libs) -std=c99 -o test test.c
   $ ./test
   key2
   key1
   key3

I tested undo/redo behavior a bit before submitting this patch, and nothing seemed negatively affected. Regardless, the existing code and this patch is clearly very suboptimal, so when I have time I will be attempting a more major rewrite to optimize the code clarity and performance.

Thanks for your work on this - I will regression test it as soon as I can.

In the meantime I draw your attention to bug #1404600, also to do with copying files and now also with a bounty of $100, which you might like to tackle.

Sorry for the delay in testing.

I could not demonstrate any speedup when copying 1350 x 10byte files and 2700 x 5byte files between tmpfs folders set up as per the bug. In each case trunk and this branch took the same length of time (within a few percent) and in each case it took approximately 4 times as long to copy twice the number of files. This was using Ctrl-C to copy and Ctrl-V to paste. For comparison Thunar performed the same copy approximately 20 times faster.

So I am afraid I cannot consider the bug fixed yet.

No progress window appeared during copy (but that's probably a separate bug).

Undoing the copy worked OK though.

The test scenario I used for this branch was copying 250,000 files, which resulted in an exponential delay caused by list traversals. I'm not surprised that this fix does not improve copying only 1,350 files. Lower bounds performance is something that will need to be improved separately.

In order to bring the speed of Pantheon Files in line with efficient file managers such as Thunar, a significant portion of code will need to be thrown out and rewritten from scratch. This includes almost all of the code in libcore, as well as other code using the libcore API.

For an idea of just how bad Pantheon Files currently is:

After copying 1 million files, the virtual memory size was > 1G. (This is with the test_dir_is_parent patch applied, otherwise pantheon-files would go OOM). Later, I rewrote[1] marlin_file_operations_copy_move. Copying 1 million files again, VmSize started and ended with 19M.

This issue ties into bug #1404600. Improving CPU performance will involve optimizing the (mis)use of data structures, which will also reduce the massive amount of heap fragmentation.

I am removing my bounty claim on this bug. In retrospect, it wasn't justified.

[1] http://paste.ubuntu.com/10449351/ - this was just for testing, but in theory, the actual implementation can be just as efficient.

It was probably over-optimistic to expect this to be a simple fix. After consideration I am willing to pay out the bounty for the work you have done since the bug reporter has confirmed that you have fixed the bug as described (i.e. relating to 100,000's of files).

We could discuss creating a new bug relating explicitly to baseline file operation performance with a more suitable bounty. Files does not necessarily need to be the fastest file manager as long as in practice it is fast enough for most users.

Jeremy,

I created a blueprint for this over at https://blueprints.launchpad.net/pantheon-files/+spec/improve-file-processing-performance. Let me know what you think.

Regarding the bounty: Please keep it. My claim was frankly predatory, for such an easy fix; I feel embarrassed.

Thanks very much for the excellent blueprint! How easy something is depends on how talented and/or experienced you are and you clearly have significant skills. You are welcome to contribute to elementaryos any time - with or without a bounty.

cmm2 (Sorry I don't have your name): I set the bug status back to "Confirmed" from "In progress" and unlinked this branch because (as far as I know)you cannot remove a bounty once placed. I widened the scope of the bug and increased the bounty, if you wish to continue to work on it.

Thanks again for your contributions.