Periodically free memory from unused images #3915

dnsge · 2022-08-10T04:04:19Z

CHANGELOG.md was updated, if applicable

Description

Attempts to reduce memory usage due to images.

Reduces copying of frame data (though this doesn't actually increase memory usage in the long term)
Periodically discards frame data of Images that haven't recently been painted

Images store the last time they were accessed via Image::pixmapOrLoad. Every 1 minute, Chatterino will check every Image instance and check if the last accessed time was more than 10 minutes ago. If so, it deletes the frame data and marks the image as needing to be loaded again.

The intention is that Chatterino instances with many open splits but with only a few being used at any moment will no longer hold Image data for emotes that aren't used in the active splits. This has no effect on emotes shared between splits.

These duration values are specified here: https://github.com/dnsge/chatterino2/blob/b35071aee8511aa87155e70184254aec51b14013/src/messages/Image.cpp#L28-L31

Addresses #3062

Memory Usage Graph

The following graph's lines indicate:

Opening the emote popup for a single channel
Closing the emote popup
The automatic expired image frame data cleanup occurring (at shorter expiration for testing)

Utilized memory goes from 154.74 MB before cleanup to 88.57 MB after.

Periodically, we check for images with frame data that has not been recently painted. Upon finding an "expired" image that can be reloaded later (e.g. from a URL), we delete the frame data and mark it as unloaded.

pajlada · 2022-08-13T10:43:20Z

No big feedback yet, but:
The allImages field contains every single emote and emoji, which means the iteration we do happens for all ~6k images. Could they be added only when they are actually loaded (i.e. from the Image::load function)? Could they also then be removed from the list again since they would be re-added whenever Image::load is called next?

Images also seem to load as soon as they are created, even if the split isn't visible. Probably not relevant for this PR, but this PR has made that fact very clear.

Overall, this approach seems like a sane approach to take 👍

src/messages/Image.cpp

pajlada · 2022-08-17T08:38:00Z

I want to take another look at the code before I merge this in, but I've been running this branch for 2 days without any crashes/freezes

fourtf

Looks good to me apart from some small things

src/messages/Image.cpp

src/messages/Image.hpp

src/messages/Image.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2022-09-03T12:27:13Z

src/messages/Image.cpp

@@ -346,8 +373,10 @@ void Image::load() const

    if (this->shouldLoad_)
    {
-        const_cast<Image *>(this)->shouldLoad_ = false;
-        const_cast<Image *>(this)->actuallyLoad();
+        Image *this2 = const_cast<Image *>(this);


warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]

Suggested change

Image *this2 = const_cast<Image *>(this);

auto *this2 = const_cast<Image *>(this);

github-actions · 2022-09-03T12:27:13Z

src/messages/Image.cpp

@@ -346,8 +373,10 @@ void Image::load() const

    if (this->shouldLoad_)
    {
-        const_cast<Image *>(this)->shouldLoad_ = false;
-        const_cast<Image *>(this)->actuallyLoad();
+        Image *this2 = const_cast<Image *>(this);


warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

Image *this2 = const_cast<Image *>(this); ^

github-actions · 2022-09-03T12:27:13Z

src/messages/Image.cpp

+    return instance;
+}
+
+void ImageExpirationPool::addImagePtr(ImagePtr imgPtr)


warning: parameter 'imgPtr' is unused [misc-unused-parameters]

Suggested change

void ImageExpirationPool::addImagePtr(ImagePtr imgPtr)

void ImageExpirationPool::addImagePtr(ImagePtr /*imgPtr*/)

github-actions · 2022-09-03T12:27:13Z

src/messages/Image.hpp

@@ -3,11 +3,14 @@
 #include <QPixmap>
 #include <QString>
 #include <QThread>
+#include <QTimer>
 #include <QVector>
 #include <atomic>
 #include <boost/noncopyable.hpp>


warning: 'boost/noncopyable.hpp' file not found [clang-diagnostic-error]

#include <boost/noncopyable.hpp> ^

github-actions · 2022-09-03T12:27:13Z

src/messages/Image.hpp

+     */
+    void freeOld();
+
+private:


warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change

private:

src/messages/Image.hpp:105: previously declared here

private: ^

pajlada · 2022-09-03T12:31:07Z

Ignore clang-tidy for now 🗡️

There seems to be a "leak" in the images debugcount - you can reproduce this by opening Chatterino, press F10 to open the Debug popup, then type an emote in a split. It will increase the images counter by 1.
Type the same emote again, it will increase the images counter by 1 again. Every time the emote is written, the count is increased.

dnsge · 2022-09-03T16:53:58Z

There seems to be a "leak" in the images debugcount

I found the issue, it wasn't actually due to incorrectly keeping track of the number of images. Instead, every message would have the reply button image pixmap created and appended, regardless of whether the setting is enabled (this is correct behavior).

https://github.com/dnsge/chatterino2/blob/20c974fdab215c54297cc5a47ea7d1e03c6065be/src/providers/twitch/TwitchMessageBuilder.cpp#L346-L350

I added caching to creating Image instances from resource pixmaps so that we reuse the same Image for the same pixmap and scale.

dnsge · 2022-09-03T17:02:30Z

By the way, commit 2dd96d2 adds the usage of <boost/functional/hash.hpp>. I have no clue about vcpkg, but I remember that bringing in circular_buffer required adding some vcpkg entry (#3941). It would be good if someone could check if this new include requires an entry too.

github-actions

clang-tidy made some suggestions

src/messages/Image.cpp

Suggestions taken care of

dnsge added 4 commits August 7, 2022 17:52

Reduce frame copying and preallocate vectors

df89a48

Automatically free image frame data after not being used

1f88199

Periodically, we check for images with frame data that has not been recently painted. Upon finding an "expired" image that can be reloaded later (e.g. from a URL), we delete the frame data and mark it as unloaded.

Change how QTimer is used, add ifndef guards

e96eb44

Update CHANGELOG.md

b35071a

dnsge changed the title ~~Reduce frame copying and preallocate vectors~~ Attempt to reduce memory usage from Images Aug 10, 2022

dnsge added 5 commits August 10, 2022 13:18

Use map of raw and weak ptrs for safety

bd99f22

Clear frame data without allocating new Frames instance

eadff89

Execute freeOld in GUI thread

28eeccb

Reorder Image.hpp/Image.cpp

60c8e2a

Reword changelog entry

988423b

pajlada reviewed Aug 13, 2022

View reviewed changes

src/messages/Image.cpp Show resolved Hide resolved

dnsge added 2 commits August 13, 2022 12:27

Only add images to ImageExpirationPool if they have frame data

b74b826

Merge branch 'master' into perf/image-memory-optimization

56dd41c

dnsge marked this pull request as ready for review August 14, 2022 00:10

pajlada changed the title ~~Attempt to reduce memory usage from Images~~ Periodically free memory from unused images Aug 17, 2022

dnsge mentioned this pull request Aug 17, 2022

Images for emotes, badges, etc. are not lazily loaded as intended #3929

Open

4 tasks

fourtf previously requested changes Aug 27, 2022

View reviewed changes

src/messages/Image.cpp Outdated Show resolved Hide resolved

src/messages/Image.hpp Outdated Show resolved Hide resolved

src/messages/Image.cpp Show resolved Hide resolved

src/messages/Image.cpp Outdated Show resolved Hide resolved

src/messages/Image.cpp Outdated Show resolved Hide resolved

dnsge and others added 3 commits August 27, 2022 11:44

Minor fixes

4f212a3

Merge branch 'master' into perf/image-memory-optimization

a7a0fd2

Merge branch 'master' into perf/image-memory-optimization

1cbd8b0

github-actions bot reviewed Sep 3, 2022

View reviewed changes

dnsge added 2 commits September 3, 2022 12:31

Cache resource Image instances

2dd96d2

Keep track of # of loaded images in debug popup

de1ff9e

Merge branch 'master' into perf/image-memory-optimization

edcd22f

github-actions bot reviewed Sep 3, 2022

View reviewed changes

src/messages/Image.cpp Show resolved Hide resolved

Merge branch 'master' into perf/image-memory-optimization

e77b6cf

pajlada approved these changes Sep 4, 2022

View reviewed changes

pajlada enabled auto-merge (squash) September 4, 2022 10:06

pajlada merged commit 8ec032f into Chatterino:master Sep 4, 2022

dnsge deleted the perf/image-memory-optimization branch September 5, 2022 00:26

dnsge mentioned this pull request Sep 7, 2022

Lazily load images on paint, not layout #3950

Open

2 tasks

TETYYS mentioned this pull request Sep 14, 2022

Chatterino has a memory usage problem #3062

Closed

Felanbird mentioned this pull request Nov 29, 2022

Animated emotes disappear after staying in an inactive window #4202

Open

4 tasks

Felanbird mentioned this pull request Dec 6, 2022

Unnecessary resource usage in channels that I'm not particularly active in #3540

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Periodically free memory from unused images #3915

Periodically free memory from unused images #3915

dnsge commented Aug 10, 2022 •

edited

pajlada commented Aug 13, 2022 •

edited

pajlada commented Aug 17, 2022

fourtf left a comment

github-actions bot left a comment

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

github-actions bot Sep 3, 2022

pajlada commented Sep 3, 2022

dnsge commented Sep 3, 2022

dnsge commented Sep 3, 2022

github-actions bot left a comment

	Image this2 = const_cast<Image >(this);
	auto this2 = const_cast<Image >(this);

	void ImageExpirationPool::addImagePtr(ImagePtr imgPtr)
	void ImageExpirationPool::addImagePtr(ImagePtr /imgPtr/)

Periodically free memory from unused images #3915

Periodically free memory from unused images #3915

Conversation

dnsge commented Aug 10, 2022 • edited

Description

Memory Usage Graph

pajlada commented Aug 13, 2022 • edited

pajlada commented Aug 17, 2022

fourtf left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

github-actions bot Sep 3, 2022

Choose a reason for hiding this comment

pajlada commented Sep 3, 2022

dnsge commented Sep 3, 2022

dnsge commented Sep 3, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

dnsge commented Aug 10, 2022 •

edited

pajlada commented Aug 13, 2022 •

edited