Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use QThreadPool for threading #146

Merged
merged 3 commits into from Jul 24, 2013
Merged

Conversation

@mwiencek
Copy link
Member

mwiencek commented Jul 5, 2013

This replaces our custom threading code with QThreadPool/QRunnable. A nice thing about QThreadPool is that it uses an ideal thread count for the system, and takes care of queuing etc. I'm not sure if there's a reason these weren't used before, so maybe this patch has problems that I'm unaware of. I haven't noticed any dip in performance. More testing is needed.

@zas

This comment has been minimized.

Copy link

zas commented on e40e1f3 Jul 5, 2013

I did some tests, it is working for me, no visible issue for now.

@lalinsky

This comment has been minimized.

Copy link
Member

lalinsky commented Jul 5, 2013

I think that this will lead to weird race conditions when saving files. If I remember correctly, the previous code always saved one file at a time. If you allow multiple saves at the same time, the _save_and_rename function in file.py must be able to handle things like if the target directory does not exist when calling isdir, but when it wants to call makedirs, it's already there and it will throw an exception.

@mwiencek

This comment has been minimized.

Copy link
Member Author

mwiencek commented Jul 5, 2013

@lalinsky ah, thanks for the insight. That does make sense. I wonder if it's worth making those functions thread-safe. It's a shame, because I'd love to not have to maintain our own threadpool code if we don't have to.

@lalinsky

This comment has been minimized.

Copy link
Member

lalinsky commented Jul 6, 2013

One simple solution would be to add a lock around the save process, or we could use a dedicated QThreadPool with maxThreadCount=1 for saving,

@mwiencek

This comment has been minimized.

Copy link
Member Author

mwiencek commented Jul 8, 2013

Hmm, I like the sound of the second solution. I'm afraid that a lock might quickly max out the active thread count and clog the thread queues with file saves. But I'll give both ideas a shot and see how they perform. :)

@zas

This comment has been minimized.

Copy link
Collaborator

zas commented Jul 20, 2013

Any progress on this patch ?

@mwiencek

This comment has been minimized.

Copy link
Member Author

mwiencek commented Jul 20, 2013

Unfortunately not, I haven't had enough time lately to finish this. I'll try to get it done by the end of next week.

mwiencek added 2 commits Jul 23, 2013
With a thread count of 1, to avoid race conditions in File._save_and_rename.
Conflicts:
	picard/ui/mainwindow.py
@mwiencek

This comment has been minimized.

Copy link
Member Author

mwiencek commented Jul 23, 2013

Went with the separate thread pool idea in 135648d. I tried a mutex, and at most saw four threads waiting simultaneously. But this might've been because I'm using an SSD, so I don't know what the average performance would be like.

@lalinsky

This comment has been minimized.

Copy link
Member

lalinsky commented Jul 23, 2013

+1 from me

@mineo

This comment has been minimized.

Copy link

mineo commented on picard/log.py in e40e1f3 Jul 23, 2013

I think those functions are the only ones using proxy_to_main which already has a #REMOVEME comment on it - unless some plugins use it, now seems like a good time to remove it :)

/edit: Sorry, I see you actually removed that - I'll blame the unbearable heat for not noticing that!

This comment has been minimized.

Copy link
Owner Author

mwiencek replied Jul 23, 2013

I did remove it ;)

Okay, I actually replaced it with thread.to_main, which is used in the _message function, plus set_statusbar_message in mainwindow.py and Runnable.run in thread.py.

I wish I knew what the context of the #REMOVEME comment was. It's still necessary to run certain things in the main GUI thread only (basically any Qt function that update the GUI can't be run in a separate thread).

priority = kwargs.pop('priority', QtCore.Qt.LowEventPriority)
event = ProxyToMainEvent(handler, args, kwargs)
QtCore.QCoreApplication.postEvent(self, event, priority)
def run_task(func, next, priority=0, thread_pool=None):

This comment has been minimized.

Copy link
@mineo

mineo Jul 23, 2013

Member

Hm, I'm not sure I understand the change from Qt EventPriority enum to numerical values - was it done because QThreadpool.start uses priority=0 by default and we need 2 priorities higher than the default (whereas EventPriority only provides 1)?

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 23, 2013

Author Member

Yeah, basically. QThreadPool just uses a simple int for priorities, starting at 0. Not sure if it was a design decision by the Qt people to not use an enum, or what.

IIRC, LowEventPriority corresponds to -1, which causes the thread to never start because it's less than 0 (that might be a Qt bug). So the Qt::EventPriority enum just wasn't meant to be used with QThreadPool, it seems.

@zas

This comment has been minimized.

Copy link
Collaborator

zas commented Jul 23, 2013

Looks ok to me, but i'm a noob regarding python/Qt/threads, i quickly tested in various cases, i didn't notice any issue.
Not sure it is the best performance wise, but code is much simpler, so +1 from me.

@mineo

This comment has been minimized.

Copy link
Member

mineo commented Jul 24, 2013

+1 from me as well.

mwiencek added a commit that referenced this pull request Jul 24, 2013
@mwiencek mwiencek merged commit ca12a56 into metabrainz:master Jul 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.