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

Reintroduce caching with a limit #745

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Sep 15, 2023

Pull Request Description

PR #463 introduced caching for ExtendedImage. PR #576 reverted that change. While we have done several things to reduce the jankiness of reloading images (e.g., removing Hero, removing spinners, calculating height), there are still some times when the image reloads unexpectedly and it can be a bit jarring/annoying. One such example is when marking a post as unread, of all things. There's also still the "issue" where if you scroll out of frame and scroll back, all the images have to reload again (discussed here). Finally, there is some discussion in #487 that caching may help to prevent that issue as well.

In this PR, I have first of all set clearMemoryCacheWhenDispose back to false. But more importantly, I have also set cacheMaxAge to 1 minute. This should give us enough time that common operations (scrolling up/down, marking unread) will show the cached image, but the cache will also expire relatively quickly so that prolonged usages of the app will not maintain more than a minute's worth of image cache. (And of course, we can discuss this value.)

Let me know your thoughts on this approach! And maybe we can do some profiling too.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Before

qemu-system-x86_64_G4DuwK9joB.mp4

After

qemu-system-x86_64_XuEKvBe4be.mp4

Checklist

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

@hjiangsu
Copy link
Member

It looks good! I'm still a bit wary about re-introducing the clearMemoryCacheWhenDispose change because it had issues in the past where memory would not be cleared and it would result in a memory leak.

However, we can go ahead and profile this again and check how it performs. I'll keep this on hold until the general release so that we don't introduce this right away if thats good with you!

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

I'll go ahead and merge this sooner so that we can get feedback from the nightly builds!

@hjiangsu hjiangsu merged commit 6ee4212 into thunder-app:develop Sep 22, 2023
1 check passed
@micahmo micahmo deleted the feature/image-cache branch September 23, 2023 02:29
@micahmo
Copy link
Member Author

micahmo commented Sep 25, 2023

Reporting back after daily driving this for a while... Unfortunately setting cacheMaxAge seems to have an unintended side effect which is that the cache is invalidated after the given time period even if the widget is not disposed! That means, for example, if you're reading comments for a while and then navigate back to the main page, all the images reload. 😞

I would like to investigate this some more, because I would like the cache to be maintained as long as the widgets are not disposed, but also keep the cache around for a little if they are.

That said, if there isn't a nice solution by the time the next release comes around, I'd rather roll this back than have the current behavior lol.

@micahmo micahmo mentioned this pull request Jan 31, 2024
3 tasks
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