Skip to content

Qt5: Eliminate slow path when showing messages #4894

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

Merged
merged 1 commit into from
Aug 10, 2015
Merged

Qt5: Eliminate slow path when showing messages #4894

merged 1 commit into from
Aug 10, 2015

Conversation

pwuertz
Copy link
Contributor

@pwuertz pwuertz commented Aug 10, 2015

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.

@tacaswell tacaswell added this to the next point release milestone Aug 10, 2015
@@ -493,8 +497,7 @@ def notify_axes_change(fig):

@QtCore.Slot()
def _show_message(self, s):
# Fixes a PySide segfault.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what this comment is referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop this helper function and just hook up self.statusbar_label.setText to the proper signal?

@pwuertz pwuertz mentioned this pull request Aug 10, 2015
@tacaswell
Copy link
Member

Seems reasonable to me. Still not sure I understand the underlying cause of the regression.

attn @mfitzp

@mfitzp
Copy link
Member

mfitzp commented Aug 10, 2015

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()

@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 10, 2015

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.

@tacaswell
Copy link
Member

@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]

@mfitzp
Copy link
Member

mfitzp commented Aug 10, 2015

Tested with PyQt4, 5 and PySide without problems.

mfitzp added a commit that referenced this pull request Aug 10, 2015
Qt5: Eliminate slow path when showing messages
@mfitzp mfitzp merged commit 8a2aab2 into matplotlib:master Aug 10, 2015
@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 10, 2015

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.

@tacaswell
Copy link
Member

Thanks both of you.

@mfitzp I think this is your first merge to master, congratulations!

@mfitzp
Copy link
Member

mfitzp commented Aug 10, 2015

@tacaswell It is, thanks! waits for things to explode

@pwuertz pwuertz deleted the qt5_slow_message_fix branch August 10, 2015 14:49
@tacaswell tacaswell mentioned this pull request Aug 11, 2015
17 tasks
@TD22057
Copy link
Contributor

TD22057 commented Aug 11, 2015

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

@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 11, 2015

Let's quickly fix all slow-paths then while they are so extremely noticeable ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants