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 QFontMetrics::elidedText over manual implementation #4807

Merged
merged 5 commits into from Sep 9, 2023

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Sep 7, 2023

Description

There is a major performance issue when displaying replies to a message with certain Arabic characters (measuring the width for them seems to be a slow path). This PR attempts to resolve parts of that issue.

Reproduction

If you're running a debug build, do the following:

  1. Open !!! as a new split
  2. Run the following command a few times:
    /fakemsg @reply-parent-user-login=medicwho;id=162f1550-4686-40a9-bec1-66aa5cf4fcc0;subscriber=1;reply-parent-msg-id=72733dad-a857-408d-ab16-e96834aadf73;first-msg=0;reply-parent-display-name=MedicWho;reply-thread-parent-msg-id=72733dad-a857-408d-ab16-e96834aadf73;badges=subscriber/24,no_audio/1;reply-thread-parent-user-login=medicwho;room-id=11148817;user-id=117691339;turbo=0;color=#DAA520;reply-parent-user-id=71651832;display-name=Mm2PL;badge-info=subscriber/27;reply-parent-msg-body=ࢿࣃࣄࣁࢾࣂࣄࣁࢿࣃࣄࢿࢾࣂࣃࢿࣄࣃࣁࢿࣆࣀࣃࣄࢿࣃࣆࣃࣀࣂࢾࣅࣀࣃࢿࣆࣄࢿࣂࣂࢾࣆࣁࣂࢾࣅࢿࢾࣁࣄࢿࣀࣁࣆࣁࣁࢿࣂࣆࣆࣄࣃࢿࣀࣁࣁࣃࣀࣅࣃࣃࣀࣂࣂࣀࢿࢾࣅࢾࣃࢿࢾࣅࣀࣂࣀࣁࣃࣃࣃࣀࢾࣄࣃࣆࢾࣃࣆࣄࣃࣅࣃࣂࣄࣁࣆࢾࣄࢿࣃࢾࢿࣅࣆࣃࢾࣄࣄࣂࢾࣂࣂࣁࣅࣅࢿࢾࣀࢾࣄࣅࣅࢿࣆࣄࣁࣄࣂࣁࣄࢾࣁࣅࣆࣂࢿࣄࣅࣆࢿࢿࣄࣀࣄࣅࣃࣄࢿࣀࣄࢿࣅࣆࣆࣆࢾࣀࢾࢿࣄࣂࣁࣆࣁࣅࣀࢿࣃࣀࣅࢿࣆࣃࣅࣂࢿࢿࢾࣀࣆࣆࣁࣂࣃࣄࣁࣀࣁࣆࢾࢾࣁࢾࣆࢿࣁࣆࣂࣃࢾࣄࢿࣂࣂࢿࣂࣃࣃࣄࣃࣂࣆࢿࢿࣄࣂࢾࣄࢾࢿࣃࢿࣁࣅࣆࣅࣁࢾࣀࣆࢾࣂࣂࣅࣃࣀࣂࢾࣁࢿࣅࣃࢿࣂࣄࢾࣆࢿࣂࣄࣁࣁࢿࣂࢿࣄࣆࣃࢾࣆࣀࣀࣀࢾࣃࣃࣀࣁࣃࣄࣀࣀࣆࣅࣁࣁࣂࣁࣅࣅࢾࣀࢿࣂࣀࣀࢿࢿࣃࣆࣆࣃࣄࣆࣆࣁࣁࣆࢾࢾࣃࣄࢿࢿࢾࣆࣆࣄࢿࣄࢾࣅࣂࣆࣀࣀࣅࣂࣁࣄࣅࢾࢾࣆࣀࣆࣆࣅࣀࣁࣅࣅࢿࣆࣀࣆࣂࢾࢿࢿࣃࢿࣃࢿࢾࢾࣄࣁࣁࢾࣁࣀࣅࣅࢾࣆࣄࣂࣄࣅࢿࢾࣄࣅࣄࣁࣅࣆࣄࢾࣀࣃࣄࢿࣂࣀࣁࣆࣆࣃࣁࣃࣆࣄࣆࢿࣁࢾࣂࣃࣃࣂࣆࣆࣂࣃࢿࣁࣆࣃࣆࣀࣅࣅࣃ;mod=0;returning-chatter=0;emotes=;flags=;tmi-sent-ts=1694024399198;user-type= :mm2pl!mm2pl@mm2pl.tmi.twitch.tv PRIVMSG #!!! :@MedicWho _unicode
    

When running a release build:

[!WARNING]
Do this in a private channel! This will impact others!

  1. Send
    ࢿࣃࣄࣁࢾࣂࣄࣁࢿࣃࣄࢿࢾࣂࣃࢿࣄࣃࣁࢿࣆࣀࣃࣄࢿࣃࣆࣃࣀࣂࢾࣅࣀࣃࢿࣆࣄࢿࣂࣂࢾࣆࣁࣂࢾࣅࢿࢾࣁࣄࢿࣀࣁࣆࣁࣁࢿࣂࣆࣆࣄࣃࢿࣀࣁࣁࣃࣀࣅࣃࣃࣀࣂࣂࣀࢿࢾࣅࢾࣃࢿࢾࣅࣀࣂࣀࣁࣃࣃࣃࣀࢾࣄࣃࣆࢾࣃࣆࣄࣃࣅࣃࣂࣄࣁࣆࢾࣄࢿࣃࢾࢿࣅࣆࣃࢾࣄࣄࣂࢾࣂࣂࣁࣅࣅࢿࢾࣀࢾࣄࣅࣅࢿࣆࣄࣁࣄࣂࣁࣄࢾࣁࣅࣆࣂࢿࣄࣅࣆࢿࢿࣄࣀࣄࣅࣃࣄࢿࣀࣄࢿࣅࣆࣆࣆࢾࣀࢾࢿࣄࣂࣁࣆࣁࣅࣀࢿࣃࣀࣅࢿࣆࣃࣅࣂࢿࢿࢾࣀࣆࣆࣁࣂࣃࣄࣁࣀࣁࣆࢾࢾࣁࢾࣆࢿࣁࣆࣂࣃࢾࣄࢿࣂࣂࢿࣂࣃࣃࣄࣃࣂࣆࢿࢿࣄࣂࢾࣄࢾࢿࣃࢿࣁࣅࣆࣅࣁࢾࣀࣆࢾࣂࣂࣅࣃࣀࣂࢾࣁࢿࣅࣃࢿࣂࣄࢾࣆࢿࣂࣄࣁࣁࢿࣂࢿࣄࣆࣃࢾࣆࣀࣀࣀࢾࣃࣃࣀࣁࣃࣄࣀࣀࣆࣅࣁࣁࣂࣁࣅࣅࢾࣀࢿࣂࣀࣀࢿࢿࣃࣆࣆࣃࣄࣆࣆࣁࣁࣆࢾࢾࣃࣄࢿࢿࢾࣆࣆࣄࢿࣄࢾࣅࣂࣆࣀࣀࣅࣂࣁࣄࣅࢾࢾࣆࣀࣆࣆࣅࣀࣁࣅࣅࢿࣆࣀࣆࣂࢾࢿࢿࣃࢿࣃࢿࢾࢾࣄࣁࣁࢾࣁࣀࣅࣅࢾࣆࣄࣂࣄࣅࢿࢾࣄࣅࣄࣁࣅࣆࣄࢾࣀࣃࣄࢿࣂࣀࣁࣆࣆࣃࣁࣃࣆࣄࣆࢿࣁࢾࣂࣃࣃࣂࣆࣆࣂࣃࢿࣁࣆࣃࣆࣀࣅࣅࣃ
    
  2. Reply to the message

The manual text elision is really slow here, because it removes one character of the last overflowing word at a time. Qt has a native text elision algorithm, that seems to be faster (QFontMetrics::elidedText).

This isn't the exact same workload (as I couldn't interact with Chatterino before) but in both cases, the same channel was shown.

Before After
vtune-gui_2023-09-07_13-03-18 vtune-gui_2023-09-07_13-03-01

Further Improvements

Even with this implementation, Chatterino will lag if many channels are open and one or more channels contain a reply to a bad message. This is because there are too many forced layouts. When many channels are open and an image gets loaded, all ChannelViews are forcefully laid out. Images are loaded when laying out messages (see #3929 and #3929).

A ChannelView that's not visible shouldn't need to be laid out. I attempted a fix in perf/layout-visible. This does have some consequences (e.g. switching channels isn't as smooth but not really noticeable - could be behind a setting [lazy vs. eager layout]). On this upside, this has the same effect as #3929 in that images won't be loaded if they're not needed, thus reducing memory usage and reducing lag when loading recent messages at the start for all open channels.

Another optimization one can make is to swap LimitedQueue for message layouts inside a ChannelView with a raw boost::circular_buffer, since it's only ever accessed by the GUI thread. I did that in perf/no-limited-queue. This will reduce a lot of unnecessary copies (for all the snapshots) resulting in less memory use and lower rendering times (copying the shared pointers requires incrementing the reference count).

Yet another optimization could be to remember in a MessageLayout if it contained any unloaded images. MessageLayout::layout could then ignore layout requests if an image was loaded, but the current element doesn't contain any unloaded image.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Mm2PL
Copy link
Collaborator

Mm2PL commented Sep 7, 2023

Alternate reproduction steps that don't impact others:

  1. Open !!! as a new split
  2. Run this command a bunch:
/fakemsg @reply-parent-user-login=medicwho;id=162f1550-4686-40a9-bec1-66aa5cf4fcc0;subscriber=1;reply-parent-msg-id=72733dad-a857-408d-ab16-e96834aadf73;first-msg=0;reply-parent-display-name=MedicWho;reply-thread-parent-msg-id=72733dad-a857-408d-ab16-e96834aadf73;badges=subscriber/24,no_audio/1;reply-thread-parent-user-login=medicwho;room-id=11148817;user-id=117691339;turbo=0;color=#DAA520;reply-parent-user-id=71651832;display-name=Mm2PL;badge-info=subscriber/27;reply-parent-msg-body=ࢿࣃࣄࣁࢾࣂࣄࣁࢿࣃࣄࢿࢾࣂࣃࢿࣄࣃࣁࢿࣆࣀࣃࣄࢿࣃࣆࣃࣀࣂࢾࣅࣀࣃࢿࣆࣄࢿࣂࣂࢾࣆࣁࣂࢾࣅࢿࢾࣁࣄࢿࣀࣁࣆࣁࣁࢿࣂࣆࣆࣄࣃࢿࣀࣁࣁࣃࣀࣅࣃࣃࣀࣂࣂࣀࢿࢾࣅࢾࣃࢿࢾࣅࣀࣂࣀࣁࣃࣃࣃࣀࢾࣄࣃࣆࢾࣃࣆࣄࣃࣅࣃࣂࣄࣁࣆࢾࣄࢿࣃࢾࢿࣅࣆࣃࢾࣄࣄࣂࢾࣂࣂࣁࣅࣅࢿࢾࣀࢾࣄࣅࣅࢿࣆࣄࣁࣄࣂࣁࣄࢾࣁࣅࣆࣂࢿࣄࣅࣆࢿࢿࣄࣀࣄࣅࣃࣄࢿࣀࣄࢿࣅࣆࣆࣆࢾࣀࢾࢿࣄࣂࣁࣆࣁࣅࣀࢿࣃࣀࣅࢿࣆࣃࣅࣂࢿࢿࢾࣀࣆࣆࣁࣂࣃࣄࣁࣀࣁࣆࢾࢾࣁࢾࣆࢿࣁࣆࣂࣃࢾࣄࢿࣂࣂࢿࣂࣃࣃࣄࣃࣂࣆࢿࢿࣄࣂࢾࣄࢾࢿࣃࢿࣁࣅࣆࣅࣁࢾࣀࣆࢾࣂࣂࣅࣃࣀࣂࢾࣁࢿࣅࣃࢿࣂࣄࢾࣆࢿࣂࣄࣁࣁࢿࣂࢿࣄࣆࣃࢾࣆࣀࣀࣀࢾࣃࣃࣀࣁࣃࣄࣀࣀࣆࣅࣁࣁࣂࣁࣅࣅࢾࣀࢿࣂࣀࣀࢿࢿࣃࣆࣆࣃࣄࣆࣆࣁࣁࣆࢾࢾࣃࣄࢿࢿࢾࣆࣆࣄࢿࣄࢾࣅࣂࣆࣀࣀࣅࣂࣁࣄࣅࢾࢾࣆࣀࣆࣆࣅࣀࣁࣅࣅࢿࣆࣀࣆࣂࢾࢿࢿࣃࢿࣃࢿࢾࢾࣄࣁࣁࢾࣁࣀࣅࣅࢾࣆࣄࣂࣄࣅࢿࢾࣄࣅࣄࣁࣅࣆࣄࢾࣀࣃࣄࢿࣂࣀࣁࣆࣆࣃࣁࣃࣆࣄࣆࢿࣁࢾࣂࣃࣃࣂࣆࣆࣂࣃࢿࣁࣆࣃࣆࣀࣅࣅࣃ;mod=0;returning-chatter=0;emotes=;flags=;tmi-sent-ts=1694024399198;user-type= :mm2pl!mm2pl@mm2pl.tmi.twitch.tv PRIVMSG #!!! :@MedicWho _unicode

Requires debug build

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Sep 7, 2023

Alternate reproduction steps that don't impact others:

Thank you for mentioning that! I added it to the reproduction.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Sep 8, 2023

Just realized that SingleLineTextElement doesn't need to split the message into words. Emoji::parse can handle spaces. This would also reduce the amount of measurements when the text contains spaces. Furthermore, once Qt 5.12 support is dropped, Emoji::parse can return QStringViews into the original string.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageElement.cpp Show resolved Hide resolved
@pajlada pajlada enabled auto-merge (squash) September 9, 2023 11:31
@pajlada pajlada merged commit 515c40f into Chatterino:master Sep 9, 2023
16 checks passed
@Nerixyz Nerixyz deleted the perf/elided-text branch October 6, 2023 14:35
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.

None yet

3 participants