Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core,ios] Release more memory on entering background #11197

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

ChrisLoer
Copy link
Contributor

The iOS SDK (#9203) and the Nav SDK (mapbox/mapbox-navigation-ios#963) both have issues tied to mbgl holding onto too much memory while in the background, causing iOS to kill the app. This PR tries to make some simple changes to ameliorate the situation:

  • b4fb4f6 fixes a simple bug in our onLowMemory implementation that would prevent us from releasing abandoned buffers after clearing the in memory tile cache.
  • 52957fd modifies the iOS SDK to always clear the tile cache on entering the background state. We need to clear the cache when we enter the background because once we're actually in the background, we can't make any GL calls, and the bulk of the data we need to release is tied up in GL buffers.

Together these changes make it so that the memory used by the tile cache is effectively released when the app goes into the background. Tiles that are currently being rendered are still kept in memory, allowing a fast restart. If a low memory warning is received while in the background, we will try to clear the cache again, but it will effectively be a no-op.

How much memory will be released depends on the total size of tiles in the cache, but from playing around with the iosapp, it seems like navigating through city streets will tend to load up at least 50MB in the cache.

Before:
screenshot 2018-02-13 14 27 31

After:
(sharp drops represent switching to another app)
screenshot 2018-02-13 14 23 01

The cost of this change is that, after reloading, if you pan to a tile that would have been cached in memory, it will now require reloading/reparsing from disk. Just playing around on my phone I can't really tell the difference.

Further improvements?

Evacuate the GPU

A significant chunk of memory is still used by the tiles that are still being rendered at the time we enter the background. We don't want to throw away their data if we can avoid it because we want to be able to start rendering immediately on re-entering the foreground. It's possible we could implement a solution where:

  • On entering the background, we'd "slumber" buffers by using something like glGetBufferSubData to copy the data out into memory we manage.
  • If we received a low memory warning while in the background, we'd release our locally stored buffers -- making re-start time a little slower, but still better than having the app killed (?).
  • On re-entering the foreground, as long as the buffers were still intact, we'd re-upload them to the GPU

I don't know how significant the overhead of that copying would be, but I think it would add a fair amount of complexity.

Release other GL resources

I spent a little bit of time poking into releasing textures and framebuffers before we go into the background, since in most cases we can re-create them pretty quickly. I feel like I'm missing something, but even aggressively releasing all textures (without corresponding code to re-create/re-upload them) didn't seem to make a noticeable dent in memory usage. We could also try abandoning/re-creating programs and shaders -- I haven't looked into that at all.

/cc @bsudekum @friedbunny @kkaefer @jfirebaugh @lbud

Context cleanup must be called _after_ render sources release tiles.
@ChrisLoer ChrisLoer added the performance Speed, stability, CPU usage, memory usage, or power usage label Feb 14, 2018
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Seems like a decent strategy to me, but can we rename onLowMemory to minimizeMemoryUse or something similar, since it's no longer being called strictly in low-memory situations?

@ChrisLoer
Copy link
Contributor Author

👍 Renamed to reduceMemoryUse ("minimize" didn't feel quite right because we could choose to be more aggressive). The Android SDK keeps the name onLowMemory since it's still used on actual low memory events.

@tobrun just as a heads up, this PR assumes that Android can release GL memory while in the background -- if that's true, Android will get a win from this PR because of b4fb4f6. If Android has a need to release memory more aggressively in the background, it would also probably be pretty straightforward to add an Android-targeted implementation that also discards the currently rendered tiles during a low memory event.

Android still calls "reduceMemoryUse" only while handling a low memory event.
iOS, on the other hand, calls "reduceMemoryUse" every time it enters the background.
Retain current render tiles for fast restart.
Waiting for a memory warning doesn't work because we can't make GL release calls once we're in the background.
@ChrisLoer ChrisLoer merged commit d4df044 into master Feb 14, 2018
@ChrisLoer ChrisLoer deleted the release-more-memory branch February 14, 2018 18:55
@friedbunny
Copy link
Contributor

friedbunny commented Feb 14, 2018

It’s hard to argue with those numbers. I’m slightly worried about the performance effect this might have on older devices, but obviously keeping ahold of memory doesn’t do any good if the app gets killed in the meantime.

I’ll give this branch change a spin on some of my older devices...

@friedbunny
Copy link
Contributor

This also needs to be added to the iOS changelog (and hopefully included in #11152).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants