Repository navigation
Qt5: Eliminate slow path when showing messages - #4894
mfitzp merged 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Do you know what this comment is referring to?
Sorry, something went wrong.
There was a problem hiding this comment.
Looking at the history it seems that connecting the showMessage slot directly resulted in a segfault in PySide. The _show_message wrapper appears to be a workaround.
Sorry, something went wrong.
There was a problem hiding this comment.
Can we drop this helper function and just hook up self.statusbar_label.setText to the proper signal?
Sorry, something went wrong.
|
Seems reasonable to me. Still not sure I understand the underlying cause of the regression. attn @mfitzp |
Sorry, something went wrong.
|
Looks good to me. Using the QLabel in the statusbar should also work around the PySide bug that the comment # Fixes a PySide segfault. is referring to. @tacaswell the QStatusBar.showMessage() is apparently for short temporary messages and (apparently) forces an automatic redraw of the UI as well as emitting some other Qt signals. That's a lot to be doing on every mouse movement. By switching to a QLabel the setting of the label and the UI update are decoupled and control can be handed straight back to matplotlib. I think the regression is a combination of the increased mouse events in Qt5, possibly some slowness in matplotlib and using a slower (blocking) method to update the statusbar. Any of those individually probably wasn't an issue, but the combination leads to noticeable lag. Relevant (unanswered) post about slowdown caused by QStatusBar.showMessage() |
Sorry, something went wrong.
|
The cause of the regression is the increase in mouse events from Qt5. They are emitted at rates at which you cannot synchronously update the GUI any more - which one isn't supposed to do in the first place. Again, I think this problem was present in the code all along, it's just that you won't notice it if the event rates are low enough (i.e. not like a typical regression caused by a specific code change). In the debugger you can see that when the showMessage() call is executed, the GUI is updated immediately. The correct behaviour would be to trigger a redraw of the status bar without forcing a redraw in the event handling thread. You could argue that this is a usability/documentation bug in showMessage() since it isn't suited for rapid message updates for that reason. |
Sorry, something went wrong.
|
@mfitzp Please do the honors of merging if/when you are happy with this? [edit typing, reading mid-sentence, and then typing again is hard] |
Sorry, something went wrong.
|
Tested with PyQt4, 5 and PySide without problems. |
Sorry, something went wrong.
Qt5: Eliminate slow path when showing messages
|
It would be interesting to see how a QtQuick based approach performs, since event handling and scene drawing are decoupled more rigorous. I'd expect selectionbox/rubberband and the xy-coordinate text to perform better with this framework. |
Sorry, something went wrong.
|
Thanks both of you. @mfitzp I think this is your first merge to master, congratulations! |
Sorry, something went wrong.
|
@tacaswell It is, thanks! waits for things to explode |
Sorry, something went wrong.
|
FYI Qt5 has a change (vs Qt4) in the way they report mouse events which considered a bug. The patch in the bug report does restore the Qt4 behavior which is much better. See: https://bugreports.qt.io/browse/QTBUG-40889 |
Sorry, something went wrong.
|
Let's quickly fix all slow-paths then while they are so extremely noticeable ;) |
Sorry, something went wrong.
For every single mouse-move event the Qt5 backend displays a message (x, y coordinates) via QStatusBar.showMessage(). This function forces an immediate update of the GUI and causes the application to lag behind mouse movements in case the update process cannot keep up with the event-rate.
As an alternative approach I added a QLabel to the QStatusBar and changed its text property for displaying messages. The redraws for message updates are now handled asynchronously and thus result in much better GUI performance, at least on the systems I tested.