-
-
Notifications
You must be signed in to change notification settings - Fork 446
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
Periodically free memory from unused images #3915
Periodically free memory from unused images #3915
Conversation
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.
No big feedback yet, but: 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 👍 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me apart from some small things
There was a problem hiding this 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
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
Image *this2 = const_cast<Image *>(this); | |
auto *this2 = const_cast<Image *>(this); |
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
Image *this2 = const_cast<Image *>(this);
^
return instance; | ||
} | ||
|
||
void ImageExpirationPool::addImagePtr(ImagePtr imgPtr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: parameter 'imgPtr' is unused [misc-unused-parameters]
void ImageExpirationPool::addImagePtr(ImagePtr imgPtr) | |
void ImageExpirationPool::addImagePtr(ImagePtr /*imgPtr*/) |
@@ -3,11 +3,14 @@ | |||
#include <QPixmap> | |||
#include <QString> | |||
#include <QThread> | |||
#include <QTimer> | |||
#include <QVector> | |||
#include <atomic> | |||
#include <boost/noncopyable.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'boost/noncopyable.hpp' file not found [clang-diagnostic-error]
#include <boost/noncopyable.hpp>
^
*/ | ||
void freeOld(); | ||
|
||
private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
private: |
src/messages/Image.hpp:105: previously declared here
private:
^
Ignore clang-tidy for now 🗡️ There seems to be a "leak" in the |
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). I added caching to creating |
There was a problem hiding this 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
CHANGELOG.md
was updated, if applicableDescription
Attempts to reduce memory usage due to images.
Images store the last time they were accessed via
Image::pixmapOrLoad
. Every 1 minute, Chatterino will check everyImage
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:
Utilized memory goes from 154.74 MB before cleanup to 88.57 MB after.