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

Fix high memory usage in Reader #21120

Merged
merged 9 commits into from
Jul 26, 2023
Merged

Fix high memory usage in Reader #21120

merged 9 commits into from
Jul 26, 2023

Conversation

kean
Copy link
Contributor

@kean kean commented Jul 17, 2023

Related Issue: #20989

This PR introduces a MemoryCache singleton for storing images cached by all the image-loading subsystems in the app, of which there are a few.

Prior to this change, if you were casually browsing the Reader, you could easily reach 2 GB or higher memory usage, which is likely the reason for the frequent Watchdog terminations.

RCA: There are multiple subsystems that manage image loading in the app, each with its own cache. In some cases, the caches had no count and cost limit. More info: p1689355268133699-slack-C05CANZ2B6F.

image

After the change, the memory usage fluctuates at a quite respectable 400 MB limit, and the cache consistently gets trimmed as you use the app.

Screenshot 2023-07-17 at 5 32 22 PM

GIF

GIF data is now also stored in the same MemoryCache/shared instance.

Some blogs post heavy GIFs (20 MB+), and when I encountered one, the memory usage spiked to around 1 GB, which is expected for GIFs format. But when you leave such a site, the memory usage slowly wanes.

To test:

The scenarios I tested include browsing the posts in the Reader and opening post details.

  • Verify that the images throughout the app are cached in memory.

Regression Notes

  1. Potential unintended areas of impact: image caching through the app
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual testing
  3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean kean added this to the Pending milestone Jul 17, 2023
@kean kean requested review from staskus, alpavanoglu and guarani July 17, 2023 22:03
@kean
Copy link
Contributor Author

kean commented Jul 18, 2023

There is an odd compilation issues due to the change in 1.13.0 release. Investigating.

Screenshot 2023-07-18 at 12 36 57 PM

@alpavanoglu
Copy link
Contributor

@kean Hey,
You just need to remove the imports of WordPressUI.h from the Objective-C files as they all already import it view the Swift header. This should've been a major change in the WordPressUI framework and not a simply minor update to indicate that. But this is the case right now and that should solve it

@kean
Copy link
Contributor Author

kean commented Jul 18, 2023

@alpavanoglu it looks like it worked, thank you! Should I remove the remaining imports?

#import <WordPressUI/UILabel+SuggestSize.h>
// ...

@kean kean changed the title Fix high memory usage in the Reader Fix high memory usage in Reader Jul 18, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 18, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21120-7cffec9
Version22.9
Bundle IDorg.wordpress.alpha
Commit7cffec9
App Center BuildWPiOS - One-Offs #6506
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 18, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21120-7cffec9
Version22.9
Bundle IDcom.jetpack.alpha
Commit7cffec9
App Center Buildjetpack-installable-builds #5539
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thanks for the work! I first reviewed the code and then will move on to testing.

I agree with using a single cache, especially if we want to keep the memory usage under control.

MemoryCache looks straightforward, heavily utilizing NSCache. It looks like AnimatedImageCache was also simplified by reusing MemoryCache and removing extra calculations for an acceptable cost limit.

I lack experience implementing such image caching systems so I cannot give much expert insight. However, the implementation of MemoryCache looks understandable and straightforward. My main concern was about setting MemoryCache as a default image cache for AlamofireImage. Since ImageLoader uses AlamofireImage I wondered if there's anything we might miss by using MemoryCache?

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

I tested the solution

  • MemoryCache caching works well for static images and GIFs
  • MemoryCache also as expected acts as a default cache for AlamofireImage and WordPressUI
  • Memory footprint rose to about 700MB for me before going down and settling around 600MB. With large gifs, it spiked up to 900MB+ a couple of times and then quickly dropped. As I read in NSCache documentation:

This is not a strict limit, and if the cache goes over the limit, an object in the cache could be evicted instantly, at a later point in time, or possibly never, all depending on the implementation details of the cache.

I tested both on the trunk and on this branch but both produced similar results after a similar amount of usage and similar sites. I will do more testing tomorrow. Did you use some particular media-heavy sites yourself?

It will also be good for at least second set of eyes to look at this PR.

@guarani
Copy link
Contributor

guarani commented Jul 19, 2023

It will also be good for at least second set of eyes to look at this PR.

I can look over this tomorrow. @kean, do you think it makes sense to try to target Monday's beta?

@kean
Copy link
Contributor Author

kean commented Jul 19, 2023

Did you use some particular media-heavy sites yourself?

I searched for "photos" and "GIFs" and browsed more or less randomly. The worst-case scenario is when you browse a mix of GIFs, regular photos, and avatars. The GIFs have their own cache in the trunk, which keeps growing almost indefinitely. I quickly hit ~1.8 GB or RAM, and then the memory clears after a memory warning. I tested on iPhone 13.

You can look for application_low_memory_warning in the console. I just re-tested it again and got four memory warnings in a span of around 5 minutes. I don't think Watchdog can kill an app when you have a debugger attached. In production, that would've been a crash. The memory warnings are the main thing to look for.

MemoryCache also as expected acts as a default cache for AlamofireImage and WordPressUI

In the trunk, the app uses the default AlamofireImage memory cache with a 100 MB limit. So I actually increased the limit to up to 256 MB for the categories of photos that are loaded using AlamofireImage. But I reduced the combined limit significantly.

I don't expect this fix to be a panacea, but it should at least reduce the number of these worst-case-scenario memory warnings and crashes.

@kean
Copy link
Contributor Author

kean commented Jul 19, 2023

do you think it makes sense to try to target Monday's beta?

I'd say yes. I kept the number of changes to the minimum to reduce the risk of regressions. In the worst-case scenario, this small change may end up being not enough, but it should be an improvement.

@kean
Copy link
Contributor Author

kean commented Jul 19, 2023

To summarize, before, the app had the following caches:

  • Alamofire's cache with a 100 MB limit
  • UIImageView+Networking with no cache limit (used mainly for avatars)
  • AnimatedImageCache that uses 20% of RAM (~400 MB on 2 GB) – some of this code was copied from Nuke btw 😃

Now these caches were replaced by a single cache with a 256 MB limit. The expected result of this change is that it will reduce the number of memory warnings, especially when browsing Reader with a mix of regular images, user avatars, and GIFs.

P.S. The limits are imprecise because it's impossible to actually calculate how much memory an image takes in memory, how much memory is needed to render it on screen, and how often the system clears the textures used for rendering. So think of these numbers as more relative than absolute.

@kean
Copy link
Contributor Author

kean commented Jul 20, 2023

Update: I found three more caches: two in WPAvatarSource and one in WPTableImageSource. I'm unsure if they are as consequential as the ones in the PR. I'll investigate what they are for tomorrow and open another PR if it's worth updating them.

@staskus
Copy link
Contributor

staskus commented Jul 20, 2023

The expected result of this change is that it will reduce the number of memory warnings, especially when browsing Reader with a mix of regular images, user avatars, and GIFs.

Yes, I think this logic makes sense, given lowered limits and a single cache for both images and gifs. I ran more extensive tests today. If Reader encounters heavy GIFs, both solutions will react in the same way and memory quickly jumps into "memory_warning" territory. The difference I noticed is what happens afterward. The new solution releases memory quickly and almost fully after huge spikes, while an older solution keeps the memory footprint high, presumably because of the higher limits and multiple caches. Even without memory warnings, the newer solution does release more memory after any memory spike. You can observe it in memory graphs.

Before memory warning After memory warning
Before refactoring Test Before Refactoring 1 Test Before Refactoring 2
After refactoring Test After Refactoring 1 Test After Refactoring 2

@guarani
Copy link
Contributor

guarani commented Jul 20, 2023

Prior to this change, if you were casually browsing the Reader, you could easily reach 2 GB or higher memory usage, which is likely the reason for the frequent Watchdog terminations.

I was curious how much browsing would lead to an actual crash on my device. So I browsed images and gifs today for about 15 minutes but couldn't make it crash. I'm on an iPhone 11, so it has 4GB of RAM. By some estimates, apps can use 60% of device RAM which would be 2.4GB in my case. Since you mention casual browsing could reach 2GB, and I did some intense browsing and didn't see a crash, it's possible that:

  • Had I done more browsing, the memory would increase and the app would have been terminated
  • Memory is getting managed somehow so it never reaches high enough levels for the app to be terminated

I'm not sure if the app could go beyond the RAM limit by using paged memory, so perhaps 4GB is not the upper limit on my device.

It's clear this PR will help reduce crashes, I wanted to explore how much so, hence the above observations.

@guarani
Copy link
Contributor

guarani commented Jul 21, 2023

I'll prioritize this tomorrow to review since today I only tested the app store version to try to reproduce an actual crash in the Reader.

@kean
Copy link
Contributor Author

kean commented Jul 21, 2023

The following PR addresses the high memory usage when displaying GIFs.

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I went through the code and it looks good, although I haven't tried to compare the tradeoffs of the new NSCache with the older caching mechanisms. I did see this comment about this where it's stated that it should be an improvement.

I'll do some more testing today and leave another review.

@kean kean force-pushed the task/reduce-memory-usage branch from 3a47ef0 to 5ffdcbc Compare July 21, 2023 21:52
@guarani
Copy link
Contributor

guarani commented Jul 21, 2023

Hey @kean, I don't think we'll get this merged today. It's not a big code change, but as you mention above, it affects images throughout the app, not just Reader (on that note, the PR title seems out of date now).

This is how I see the app's caching right now (based on this Slack thread:

Does that sound accurate to you?

If we're changing the caching of pretty much all the images in the app, can I suggest listing out where in the app (including outside Reader) that you've tested for regressions? It would also be useful to list which parts of the app are not affected by this change. For example, the editor and the avatars shown when @-mentioning someone.

@kean
Copy link
Contributor Author

kean commented Jul 25, 2023

A more detailed breakdown is linked to the recent post on memory caches in Jetpack Mobile.

The list of the affected components is going to be in dozens. My approach was to provide a replacement with the exact same API and behavior (except for the size limits).

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

I retested and re-reviewed the changes once again.

I also smoke-tested other parts of the app and I haven't identified places where image loading was an issue. Address ongoing discussions and concerns before merging 👍

@guarani guarani self-requested a review July 25, 2023 19:21
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I smoke-tested the JP app and didn't run into any issues with this PR. Code-wise, as you mention above, this change unifies caches (and adjusts their sizes) without fundamentally changing the app.

It looks safe and I can't see any issues when testing. I didn't measure the performance improvement and instead relied on @staskus review of that, #21120 (comment).

@alpavanoglu
Copy link
Contributor

I've come back from my vacation. It looks like there has been good progress here. Please let me know if you want me to review and/or test as well.

@kean kean force-pushed the task/reduce-memory-usage branch from 5ffdcbc to 7cffec9 Compare July 26, 2023 14:45
@kean kean enabled auto-merge July 26, 2023 14:51
@kean kean merged commit 48476ba into trunk Jul 26, 2023
@kean kean deleted the task/reduce-memory-usage branch July 26, 2023 15:42
@kean kean modified the milestones: Pending, 23.0 Aug 1, 2023
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.

5 participants