Skip to content

Work around performance issue with Qt 5.12 and PreviewButtonDelegate.#2003

Merged
daschuer merged 2 commits into
mixxxdj:2.2from
rryan:2.2-previewdelegate
Jan 30, 2019
Merged

Work around performance issue with Qt 5.12 and PreviewButtonDelegate.#2003
daschuer merged 2 commits into
mixxxdj:2.2from
rryan:2.2-previewdelegate

Conversation

@rryan
Copy link
Copy Markdown
Member

@rryan rryan commented Jan 22, 2019

Launchpad Bug #1812763.

After upgrading to Qt 5.12, Mixxx idle CPU usage is high (99%) on
macOS. Profiling shows LibraryPreviewDelegate::paintItem as one of the hottest
functions. It appears to be related to calling QPushButton::setGeometry for each
row of the library. My workaround relies on the fact that we only need the
QPushButton size to be correct, not its position.

On macOS 10.13.6, CPU usage drops from 99% to 50% with this change
(and with #1994, idle usage drops to 15%).

@Be-ing Be-ing changed the base branch from master to 2.2 January 22, 2019 01:55
@rryan
Copy link
Copy Markdown
Member Author

rryan commented Jan 22, 2019

Be-ing changed the base branch from master to 2.2 2 minutes ago

Doh. Thanks.

@rryan
Copy link
Copy Markdown
Member Author

rryan commented Jan 22, 2019

I removed the workaround added for lp:1776555 but I'm not sure if that was necessary to fix the issue. In general I'm wary of us messing around with QWidget internals (which I believe WA_WState_Visible is an internal attribute for state tracking).

Copy link
Copy Markdown
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. I have added only some minor comments.
Regarding the Qt5.2 crash fix, I will test if I can reproduce the issue with this branch on Trusty maybe you have fixed it as well.
Else we need to know which is the first version of Qt that has fixed the root cause.
Hopefully we can remove it anyway because Trusty will EOL soon.

Q_UNUSED(option);
QPushButton* btn = new QPushButton(parent);
btn->setObjectName("LibraryPreviewButton");
QPushButton* btn = new LibraryPreviewButton(parent);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use here make_parented

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I could, but I have to return a raw pointer from this method to Qt since it takes ownership, so the parented_ptr wouldn't be documenting much.

Comment thread src/library/previewbuttondelegate.h Outdated
@daschuer
Copy link
Copy Markdown
Member

With this branch, I am not able to crash Mixxx on Trusty like lp:1776555
Unfortunately the preview button size is jumping around, not following always the column width.
This is a regression from the 2.2 branch.

@daschuer daschuer closed this Jan 23, 2019
@daschuer daschuer reopened this Jan 23, 2019
@daschuer
Copy link
Copy Markdown
Member

The "close and comment" button strikes again, sorry.

Copy link
Copy Markdown
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Does this resize issue also happens on your device?

… Launchpad Bug #1812763

After upgrading to Qt 5.12, Mixxx idle CPU usage is high (99%) on
macOS. Profiling shows LibraryPreviewDelegate::paintItem as one of the hottest
functions. It appears to be related to calling QPushButton::setGeometry for each
row of the library. My workaround relies on the fact that we only need the
QPushButton size to be correct, not its position.

On macOS 10.13.6, CPU usage drops from 99% to 50% with this change
(and with mixxxdj#1994, idle usage drops to 15%).
@rryan
Copy link
Copy Markdown
Member Author

rryan commented Jan 28, 2019

With this branch, I am not able to crash Mixxx on Trusty like lp:1776555

Great, thanks for checking.

Unfortunately the preview button size is jumping around, not following always the column width.
This is a regression from the 2.2 branch.
Does this resize issue also happens on your device?

No :( it works for me -- the delegate is always drawn at the right width when I resize the column.

@rryan rryan force-pushed the 2.2-previewdelegate branch from d1129b8 to 5c32882 Compare January 28, 2019 02:20
@rryan
Copy link
Copy Markdown
Member Author

rryan commented Jan 28, 2019

Sorry for rebasing during review, but I wanted testers of this branch to have #1994 included. I didn't change any commits.

@daschuer
Copy link
Copy Markdown
Member

If I resize the column far more than needed, the size cycling with the coloum width.
I have re-added the
m_pButton->setAttribute(Qt::WA_WState_Visible);
hack, the problem happens less often.
But it is still there at some width.

@daschuer
Copy link
Copy Markdown
Member

I have tried to debug the resizing issue, but it seems to happen inside the image style code.
Than I have tested 2.1.7 and it suffers the same issue. So it is no regression here, we can merge it.
LGTM.

Thank you.

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.

2 participants