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

Clear image memory cache when images are disposed #576

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Aug 7, 2023

Pull Request Description

Sets the image cache to be cleared from memory when the image is disposed. This will help reduce memory consumption a little bit by allowing the RAM to clear out images when not needed. I have increased the cacheExtent previously to help accommodate for the issue where we see a lot of images being reloaded

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu hjiangsu changed the title set memory cache to be cleared when image is disposed to improve memo… Clear image memory cache when images are disposed Aug 7, 2023
@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 7, 2023

@micahmo Can you do some tests to see if this is behaving okay (e.g., images are not reloading when scrolling back up)

@micahmo
Copy link
Member

micahmo commented Aug 8, 2023

The images are flushed from cache pretty quickly now. I don't have to scroll far for them to need reloading. 😞

qemu-system-x86_64_XhKsPiADnS.mp4

@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 8, 2023

Hmm, do you think it's okay? At least when scrolling through the feed normally, it the loading indicators should be less noticeable

It's still using cache, so the image fetching should be quicker than without it. If anything, we can continue to increase the cacheExtent properly until we find a good value

@micahmo
Copy link
Member

micahmo commented Aug 8, 2023

I just tried a bunch of the other apps, and none of them seem to evict their cache like this. Once a post in the feed is loaded, I can scroll way down and come back and its image is already there. But if we're trying to squeeze out better memory performance, this is fine. I actually think removing the loading indicator (and having a dissolve) would go a long way to making it look less like things are constantly loading.

@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 8, 2023

I think this is a temporary fix until we figure out how to limit the cache size and also keep memory usage within check. I do agree that maybe a dissolve would make it less like its loading

@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 8, 2023

One more thing @micahmo - could you try running it on your actual device or on a profile/release mode? I'm doing that right now and I don't get a lot of loading indicators when scrolling through the feed

@micahmo
Copy link
Member

micahmo commented Aug 8, 2023

Tried it on my physical device in profile mode. I would say it's about the same, although it doesn't "feel" as bad because the loading is much faster.

scrcpy_zXD170fLh8.mp4

@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 8, 2023

I guess it could be phone dependent. We can continue to tweak it after! I'll merge this in otherwise if you're okay with it :D

I'll try to upload a recording from my phone to show you how it looks like

@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 8, 2023

trim.D5B8738E-216C-401E-95C4-DC8AEFC1B32F.MOV

@micahmo
Copy link
Member

micahmo commented Aug 8, 2023

I'll merge this in otherwise if you're okay with it :D

Yep, go for it!

@hjiangsu hjiangsu merged commit 2d7a1bb into develop Aug 8, 2023
1 check passed
@micahmo
Copy link
Member

micahmo commented Aug 8, 2023

trim.D5B8738E-216C-401E-95C4-DC8AEFC1B32F.MOV

Wow that looks great and really smooth too. I think we're missing out. 😆

Also I noticed some of the preview progress indicators use the native iOS looking one and some use the more Android looking one. My guess is the difference is between link previews and image previews. Again, I think it would be nice to abandon these spinners altogether (in this case).

@micahmo micahmo deleted the fix/clear-image-memory-cache-on-dispose branch August 8, 2023 14:58
@hjiangsu
Copy link
Member Author

hjiangsu commented Aug 8, 2023

Wow that looks great and really smooth too. I think we're missing out. 😆

I think part of it is the high refresh rate thats enabled by default for iOS (but not Android for some reason)

Also I noticed some of the preview progress indicators use the native iOS looking one and some use the more Android looking one.

These are just differences in which image preview is loading (LinkPreviewGenerator vs the image fetching logic that we have before)!

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