PICARD-936: Improve UI responsiveness whilst busy loading, saving etc. #604

Sophist-UK · 2017-01-27T13:02:17Z

Improves the responsiveness during worker thread operations by triggering main thread to process events after events are sent to it.

Improves file loading performance by moving operations into separate threadpools.

This PR was brought to you as a joint effort between Sophist-UK and @samj1912.

Sophist-UK · 2017-01-27T13:04:00Z

This is definitely better, however there are two threading issues that still need fixing:

Most importantly, loading the file (file.py#load) is queued on a thread, but the new code (in util/init.py#open_file) to guess the file format is not and is therefore blocking. IMO the code in add_files needs refactoring to queue it on a thread.
Less importantly, the non-recursive add_directory may need to be queue it on a thread - but I need to get add_files working properly before I can test to see if this is necessary if we are already busy loading large directories recursively.

Any suggestions welcome.

Sophist-UK · 2017-01-27T13:07:31Z

picard/tagger.py

+                    get_files,
+                    process,
+                    priority=2,
+                    thread_pool=self.load_thread_pool)


We run this on the load threadpool to avoid it blocking, and run it at priority 2 so that it runs ahead of queued tasks to load metadata from files.

But I wonder whether it might make more sense to have another threadpool just for this.

I tried this and it did not seem to make much difference.

Sophist-UK · 2017-01-27T15:51:09Z

Whilst the UI is definitely a bit more responsive with the PR in its current state, the UI is still pretty unresponsive, and I am not sure how to proceed further.

Any help gratefully received.

Sophist-UK · 2017-02-03T07:45:47Z

Ok - I have found the primary cause for UI non-responsiveness.

In picard/util/thread.py#to_main When we send a task to the main UI "thread" as an event, Picard does not process that event until the main thread gets activated, and because of the Python LGL stopping real multi-tasking, that doesn't happen until potentially a lot later.

So I have added a QCoreApplication.processEvents() immediately after sending the event, in order to give control to the UI thread to process the event, and it makes the whole of Picard much more lively.

That's the good news.

The bad news is that it exposes other bug(s), most notably when loading a lot of files I get a slew of errors RuntimeError: maximum recursion depth exceeded.

The UI is also too lively, showing updates to objects too fast for the eye to process - so we probably need to do something to only display them every so many miliseconds - and leaving some objects blank,

So there is more work to do, but I am on the right track.

Sophist-UK · 2017-02-03T07:49:20Z

I suggest that we delay further work on #613 until this PR is completed so that we can evaluate whether this resolves the performance issues with large number of files loaded, thus avoiding the need for #613.

Sophist-UK · 2017-02-03T09:49:02Z

Example recursion errors: picard-recursion-errors.txt

Sophist-UK · 2017-02-03T11:29:41Z

Ok - I worked out that the cause was calling processEvents when we were on the main thread rather than leaving the main thread to get to it in its own time.

So, basic functionality is now working great. I just need to see what tweaks I need to do to which functions run on which threads and this will be ready for wider testing and merge.

Sophist-UK · 2017-02-03T12:03:42Z

At the current commit this PR seems stable - any further tweaks will be to threading to make the GUI even more responsive.

So I would welcome anyone else giving this a good test - especially on non-windows systems - and to give feedback about whether it adversely impacts file load or save performance.

Sophist-UK · 2017-02-03T12:06:34Z

P.S. Github is showing the last two commits in the wrong sequence if anyone is wondering about a missing import on the first one.

Sophist-UK · 2017-02-03T14:03:34Z

Remote loads still leave UI unresponsive - need to agree with @samj1912 about borrowing some of his code to try to address this.

samj1912 · 2017-02-03T17:50:41Z

An alt approach in #616 I am not sure which one will allow the UI to be more responsive. Can you check my PR's performance once too?

We will go with the better alt. :)

samj1912 · 2017-02-03T18:00:39Z

Since its a UI based thing, I just tested with a stopwatch from clicking the load button to when the UI is responsive again. I am running picard on Ubuntu 16.10, on a SSD , ext4.

While loading ~1000 files using your code, it takes picard about ~25 seconds before it is responsive again.
With my PR it is somewhere around ~20 seconds.

I think we can improve on this code further by finding where the UI is being blocked while loading and separating the load code from the main thread as much as possible :)

Sophist-UK · 2017-02-03T19:12:19Z

I am working on the final piece - which I suspect is quite similar to what you have just done - in which case I will borrow your code if I may. I have just reworked the whole file loading code to eliminate duplications and prepare for running open_file in a thread which should make the UI immediately responsive. :-)

samj1912 · 2017-02-03T19:15:21Z

Feel free to borrow my code if it helps :) 👍

Sophist-UK · 2017-02-03T20:32:01Z

Thank you. I did.

The code in #616 was pretty much exactly what I was about to write - so this PR now includes both @samj1912's worker thread code and his Add Directory iteration rewrite - both with some restructuring to handle hidden and excluded files in the right place and also to facilitate a check on number of files to be added easily. See what you think.

This PR is now ready for code review and testing by others on systems other than windows.

In the mean while I will see whether this helps with slow-down when you have loaded large numbers of files.

samj1912 · 2017-02-03T20:57:56Z

picard/tagger.py

-                    new_files.append(file)
-        if new_files:
+                    new_files[filename] = file
+                QtCore.QCoreApplication.processEvents()


Please add a comment explaining this line.

samj1912 · 2017-02-03T21:03:43Z

picard/tagger.py

        for filename in filenames:
            filename = os.path.normpath(os.path.realpath(filename))
-            if ignore_hidden and is_hidden(filename):


Why was this code block removed and moved to add directory code?
What if a user selects a group of files? Ignore regex and hidden file ignores won't work then?

I must have missed that. But it is an easy one to fix.

Apparently NOT fixed. Sorry. Will fix it now.

Sorry again. It was fixed - but it is in _add_files_from_directory lines 476-478 rather than here.

@samj1912 Since I have fixed this review comment can you please mark it as fixed?

Sophist-UK · 2017-02-03T21:05:19Z

The responsiveness of the UI is still not perfect - for example when loading files and processing webservice responses it can still be sluggish - however I doubt that there is much more that we can do with threading - I suspect that the cause is now the LGL preventing pre-emptive multi-tasking and that, If we can identify where to put them, a few QCoreApplication.processevent() calls will help further, Once this PR has been merged and I have the time I will take a look at some of these code paths and see if I can profile them.

To put this in context, the UI unresponsiveness when loading local files is now of the order of a second or two rather than 10's of seconds i.e. an order of magnitude better.

Sophist-UK · 2017-02-03T21:16:02Z

P.S. I have just finished loading 8594 pre-tagged files in 529 releases from a local drive and have not noticed any slowdown in file loading. It is early days for this code, but I am hopeful that the performance problems with large number of files have been significantly reduced.

However, the handles leak still persists - at the end of this load, task manager reports Python as having 58,708 open handles!!!! To put this in context, after Picard has initialised completely, Python shows 755 handles.

Sophist-UK · 2017-02-03T21:17:45Z

P.S. I am going on holiday next week for 2 weeks - so if you want code changes before I go away and before the release of 1.4 on 14 Feb, please let me have them in the next day or two at the latest.

zas · 2017-02-03T21:39:37Z

picard/tagger.py

            del self._cmdline_files
+            if files:
+                self.add_files([files])


Isn't files already a list here ?

add_files now takes a list of lists of filenames. We group files in each directory into a list and then create a list of these - so we can queue these for guess_format and loading on a directory by directory basis so the user does not have to wait until we have guess_format on every file they have selected to load before they start appearing in the UI.

P.S. About to rename tagger.py#add_files to _add_files and create a stub add_files to handle the Add Files dialog which @samj1912 picked up on.

I don't get why you need to pass list of lists of filenames, but i didn't check the code in details yet. I'm worried about such change concerning functions like tagger.add_files() that may be used in plugins (we really need to clarify plugins API in future versions).

This has been fixed. I renamed this to _add_files and created a stub tagger.add_files() which provides the same interface and functionality as before.

zas · 2017-02-03T21:48:09Z

P.S. I am going on holiday next week for 2 weeks - so if you want code changes before I go away and before the release of 1.4 on 14 Feb, please let me have them in the next day or two at the latest.

Such PR will not be merged in 1.4 but rather in 1.4.1

Sophist-UK · 2017-02-03T22:04:44Z

Such PR will not be merged in 1.4 but rather in 1.4.1

No problem.

zas · 2017-02-03T23:31:33Z

picard/util/thread.py

@@ -36,12 +36,15 @@ def run(self):

 class Runnable(QRunnable):

-    def __init__(self, func, next):
+    def __init__(self, func, next, os_priority=0):


What is os_priority ?

I accept that this is extra to the functionality of this PR. The idea is that it adjusts the operating system priority of the thread down (or up) in order to influence the way threads are dispatched by Python. But now I understand more about how the Python GIL works and impacts which thread gets the GIL (which is essentially random), I am not sure that it would be of any use. (And we don't know what else is running on the user's system and whether it would make sense to do it.)

So I will be happy to revert this Commit.

zas · 2017-02-03T23:32:12Z

picard/util/thread.py

        QRunnable.__init__(self)
        self.func = func
        self.next = next
+        self.thread_priority = min(QThread.IdlePriority,max(QThread.TimeCriticalPriority,


Explain in a comment please.

I will if we don't decide to revert this functionality.

zas · 2017-02-03T23:33:02Z

picard/util/thread.py



 def to_main(func, *args, **kwargs):
    QCoreApplication.postEvent(QCoreApplication.instance(),
                               ProxyToMainEvent(func, *args, **kwargs))
+    if QCoreApplication.instance().thread() != QThread.currentThread():


Explain in a comment please.

zas · 2017-02-03T23:33:37Z

picard/tagger.py

+        list_to_add = []
+        # walk topdown to allow modifying the sub-dirs to remove hidden dirs
+        for root, dirs, files in os.walk(unicode(path), topdown=True):
+            QtCore.QCoreApplication.processEvents()


This line deserves a comment on its role.

samj1912 · 2017-02-04T06:56:14Z

I think it will be better to remove the OS priority commits in this pr and make a separate pr for them.

Also since you are rebasing would it be possible to cherry pick my commits to this PR so that the commits are kept.

It will be nice if you could add me to collaborators for this pr, I might have a few changes I would like to make.

To keep UI characteristics the same regardless of whether debug logging is off or on, if off we call processEvents.

... so that we can make UI more responsive by calling processEvents from a worker thread and avoid Recursion Depth Exceeded messages.

This enables file opening and loading to happen in parallel which gives a better UI experience.

Sophist-UK · 2017-03-20T11:06:27Z

There is still a problem with UI responsiveness loading large numbers of files. The symptoms are that one of the worker threads (doing the new file evaluation) monopolises execution despite issuing a processEvents. Debug messages from worker threads (which also run to_main) seem to appear promptly, but the call-back function doesn't run promptly. Other worker threads like loading files are not getting a look in and I suspect that neither is the main thread. I am guessing that when it issues a local disk I/O for the 128 bytes the read finishes so quickly that the thread is ready to execute when Python issues a wakeup to all threads to decide what to execute next - and when the thread runs to_main and processEvents, again the thread is ready to run.

This may be because processEvents only runs events posted from the worker thread (as suggested by the documentation). Or it may be simply because Python doesn't handle threading well and the same thread gets the execution back again by accident (putting work into worker threads typically slows things down - see presentation on how the GIL works). So moving some code back to main thread might actually improve things.

Or perhaps there is some way to force the open_threadpool to give up CPU and allow other threads to execute (we could use time.sleep but that introduces delays when there is no other work).

This is likely to be a bit trial and error (unless anyone else has a bright idea that could help), so please bear with me.

Sophist-UK · 2017-03-21T08:30:03Z

Ok - I am getting there. However, I could do with a bit of advice.

According to http://www.qtcentre.org/printthread.php?t=7167&pp=100 we should be using signals and slots to do calling between worker threads and main thread. I haven't tried switching yet, but it looks like this switches thread and executes the slot immediately - and if so that would seem to be a better way of doing to_main. Whilst this would separate to_main execution from ensuring that the main thread gets sufficient CPU to keep the UI responsive, I think I have a different solution to that.

I am wondering whether our current approach in threads.to_main is just a legacy or whether there is a specific reason that we are not using signals and slots?

processEvents processes events in the running THREAD'S event queue not in the main thread. By using msleep(10) we give up CPU to allow main thread to grab it.

Sophist-UK · 2017-03-28T18:38:23Z

Ok - This is ready for further testing.

Please let me know if I have not resolved any previous review comments.

samj1912 · 2017-03-28T19:23:40Z

@Sophist-UK build is failing. Lots of attribute errors.

Sophist-UK · 2017-03-28T20:50:50Z

@samj1912 Damn. It was running perfectly on my PC. But I didn't run unit tests as I should have done - rusty at this. And when I do, I get the same errors. I will look into it.

Sophist-UK · 2017-03-28T20:58:10Z

I suspect that this is more about how unit testing is setup - without Qt initialisation to_main won't work.

So I either need to modify setup.py so that it initialises Qt (prefered) or have additional checks in thread.py to handle unit test situations (which I don't like the idea of).

This is needed for unit tests where it is not initialised

…://github.com/Sophist-UK/Sophist_picard into PICARD-936_separate_threadpool_for_file_loads

Sophist-UK · 2017-03-28T21:38:25Z

Hopefully that will fix it - will check that Travis ran clean in the morning.

Sophist-UK · 2017-03-31T09:09:21Z

Still more work needed.

A bug where metadata panel stops updating.
When processing lots of webservice responses, UI becomes unresponsive - so need to move some of the heavier webservice handling into worker-threads.

samj1912 · 2017-12-12T17:20:09Z

It's been almost 8 months since this was updated. Closing this with a 'Needs more work' tag. You can reopen this if you want to continue working on it. Also we have updated to Qt5 and py3 so be mindful of that while reworking on this PR.

Sophist-UK · 2017-12-12T18:23:35Z

I agree that this should be closed for three reasons:

a. It was written for P1.4 and needs to be retrofitted to P2.
b. It has multi-threading & intermittent bugs which are hellishly difficult to resolve.
c. It was way too big and needs to be broken into smaller PRs.

When I get time I will rework it for P2 in smaller chunks.

Sophist-UK commented Jan 27, 2017

View reviewed changes

Sophist-UK force-pushed the PICARD-936_separate_threadpool_for_file_loads branch from 16f8313 to 174a576 Compare February 3, 2017 07:35

Sophist-UK mentioned this pull request Feb 3, 2017

PICARD-840: Warn user when loading a large number of files #613

Closed

Sophist-UK changed the title ~~PICARD-936: Separate threadpool for file loads~~ PICARD-936: Improve UI responsiveness whilst busy loading, saving etc. Feb 3, 2017

samj1912 requested changes Feb 3, 2017

View reviewed changes

zas reviewed Feb 3, 2017

View reviewed changes

Sophist-UK added 6 commits March 19, 2017 12:07

Address @zas comments

decbf10

Improve responsiveness adding large numbers of files

98a97b9

Allow worker threads with no callback

15a565d

log.debug runs processEvents when debug off

0bc5df2

To keep UI characteristics the same regardless of whether debug logging is off or on, if off we call processEvents.

Move more code to worker thread...

0de53e8

... so that we can make UI more responsive by calling processEvents from a worker thread and avoid Recursion Depth Exceeded messages.

Add separate thread pool for file opening

29c2554

This enables file opening and loading to happen in parallel which gives a better UI experience.

Sophist-UK force-pushed the PICARD-936_separate_threadpool_for_file_loads branch from 1874c38 to 29c2554 Compare March 19, 2017 12:09

Remove debugging code left in by accident.

04ee3b3

Sophist-UK added 5 commits March 21, 2017 18:44

Create thread.processEvents and sleep thread

481d85a

processEvents processes events in the running THREAD'S event queue not in the main thread. By using msleep(10) we give up CPU to allow main thread to grab it.

Tagger object is QApplication & QCoreApplication

fcc6d61

Fix typo in debug message

ef3ec53

Tweak drag & drop debug statements

ad929e1

Merge branch 'master' into PICARD-936_separate_threadpool_for_file_loads

bbae8ca

Sophist-UK added 2 commits March 28, 2017 22:35

Check that QCoreApplication exists

5ea60ce

This is needed for unit tests where it is not initialised

Merge branch 'PICARD-936_separate_threadpool_for_file_loads' of https…

110551a

…://github.com/Sophist-UK/Sophist_picard into PICARD-936_separate_threadpool_for_file_loads

Sophist-UK added 2 commits March 30, 2017 21:46

Add threading notes

8b5a6ae

Refactor thread code to throttle sleep & processEvents

6ffdb59

samj1912 closed this Dec 12, 2017

samj1912 added the Needs more work label Dec 12, 2017

Sophist-UK mentioned this pull request May 4, 2020

Mitigate thread competition by scanning directories before loading files #1531

Closed

5 tasks

PICARD-936: Improve UI responsiveness whilst busy loading, saving etc. #604

PICARD-936: Improve UI responsiveness whilst busy loading, saving etc. #604

Conversation

Sophist-UK commented Jan 27, 2017 • edited Loading

Sophist-UK commented Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sophist-UK commented Jan 27, 2017

Sophist-UK commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017 • edited Loading

Sophist-UK commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

samj1912 commented Feb 3, 2017

samj1912 commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

samj1912 commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sophist-UK Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sophist-UK commented Feb 3, 2017 • edited Loading

Sophist-UK commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zas commented Feb 3, 2017

Sophist-UK commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 commented Feb 4, 2017

Sophist-UK commented Mar 20, 2017 • edited Loading

Sophist-UK commented Mar 21, 2017

Sophist-UK commented Mar 28, 2017

samj1912 commented Mar 28, 2017

Sophist-UK commented Mar 28, 2017

Sophist-UK commented Mar 28, 2017

Sophist-UK commented Mar 28, 2017

Sophist-UK commented Mar 31, 2017

samj1912 commented Dec 12, 2017

Sophist-UK commented Dec 12, 2017

Sophist-UK commented Jan 27, 2017 •

edited

Loading

Sophist-UK commented Jan 27, 2017 •

edited

Loading

Sophist-UK commented Feb 3, 2017 •

edited

Loading

Sophist-UK Mar 10, 2017 •

edited

Loading

Sophist-UK commented Feb 3, 2017 •

edited

Loading

Sophist-UK commented Mar 20, 2017 •

edited

Loading