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

App crashes due to Out of Memory Exception with PINRemote #243

Closed
rmato opened this issue Sep 8, 2016 · 15 comments
Closed

App crashes due to Out of Memory Exception with PINRemote #243

rmato opened this issue Sep 8, 2016 · 15 comments
Assignees

Comments

@rmato
Copy link

rmato commented Sep 8, 2016

I am using PINRemoteImage to download images and cached them in my app. My app uses lots of images, just like instagram so the images memory increases all the time. And I have lots of crashes due to out of memory.

Is there a way to set a limit of MB to store in cache? Is there a way to free memory once it's full before the app crashes?

Thanks!

@rmato rmato changed the title App crashes for Out of Memory Exception with PINRemote App crashes due to Out of Memory Exception with PINRemote Sep 8, 2016
@appleguy
Copy link
Contributor

appleguy commented Sep 8, 2016

The memory cache should be emptied in case of memory warning. I think your issue is more likely to be parts of your app that are holding onto the image references directly - like view controllers or views that aren’t being released.

Have you tried looking at the Instruments Object Allocations / Leaks tools, or the Xcode 8 Memory Graph features? It should help find it quickly.

On Sep 8, 2016, at 11:53 AM, Rodrigo Mato notifications@github.com wrote:

I am using PINRemoteImage to download images and cached them in my app. My app uses lots of images, just like instagram so the images memory increases all the time. And I have lots of crashes due to out of memory.

Is there a way to set a limit of MB to store in cache? Is there a way to free memory once it's full before the app crashes?

Thanks!


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #243, or mute the thread https://github.com/notifications/unsubscribe-auth/AAigA7mVZdTqqp0uTeoCs2KpHQFzQUOfks5qoFnEgaJpZM4J4U8w.

@lukeivers
Copy link

So I am using PINRemoteImage to download and cache progressive images in a table view. The memory balloons massively while I am scrolling, but quickly smashes itself back down when scrolling stops. When testing on my phone, I can cause crashes with this. On the simulator, I can't usually get it to crash, but I get horrible stuttering in scrolling the table and have seen memory usage jump as high as nearly 1GB before dumping back down to ~200MB (which is reasonable for what I'm doing).

So I think this is not due to having multiple images cached, as much as it is an issue with the download and caching process. Once everything IS cached, everything is great.

@danielgalasko
Copy link

Yeah I do think this might be a result of your implementation and not specifically PINRemoteImage. @lukeivers if you could drop some stack traces and maybe snippets of how your cells are using image loading etc that would be great 😄

@lukeivers
Copy link

Sure thing. I don't have any stacktraces yet (I have some configuration wrong and am not properly capturing debugging info from running it on my phone, and it doesn't actually CRASH on the simulator, just uses massive amounts of memory).

A few related notes, first:

  • Telling it to prefetch all the URLs for my entire table data source made it jump to ~4GB of memory consumed
  • Because of that, I was able to find an old issue saying to use the costLimit to keep this from happening.
  • Using costLimit did not, in fact, change memory usage of prefetching at all
  • It DID, however, change memory usage and massively improve performance in the simulator for this table.
  • Unfortunately, it appears to have no effect when running the app on my phone. It still crashes.
  • I did catch in the device logs that it was having problems allocating memory for an image just before it crashed (I'll see if I can find the exact error message here in a bit).

Anyway, here's some code I'm using (this is literally the entire cellForRowAtIndexPath code):

let cell = tableView.dequeueReusableCell(withIdentifier: "MyCell", for: indexPath as IndexPath) as! MyTableCell

cell.imageView.pin_updateWithProgress = true

cell.selectionStyle = .none

let itemForIndexPath = myCollection.sections[indexPath.section].items[indexPath.row]
if cell.item == nil || (cell.item != nil && cell.item != itemForIndexPath) {
    cell.item = itemForIndexPath
    cell.imageView.pin_setImage(from: cell.item.escapedWideScreenImageNSURL, placeholderImage: MyTableViewController.blankImage)
}

return cell

Also, it only has the super high memory usage / crashing behavior when it is first loading in the images. Once they are locally cached, say on disk, then it behaves just fine. And if I scroll slowly down the page, it's laggy but it has no issues.

It appears as though it's allocating tons of memory during the initial download process that it then immediately clears out after the image has been locally cached. Maybe this is related to the progressive jpeg code where it's trying to create the lower resolution partially downloaded images for each cell's image?

Keep in mind that every one of my images, which I have 50-60 of in the test case I'm running (manually, not as an actual UITest or whatever), is at like 4-5k x 4-5k pixels, so it's a LOT of data / pixels.

@lukeivers
Copy link

One can see here that it's consuming MASSIVE amounts of memory in CG raster data:

screen shot 2016-09-16 at 10 51 36 am

@lukeivers
Copy link

Ok, the last one was on my simulator, this one is from my actual phone:

screen shot 2016-09-16 at 10 55 03 am

@lukeivers
Copy link

One final note: I'm not actually getting a crash on my phone. The app just disappears. I think that the system memory watchdog is killing off my app due to excessive memory usage, as opposed to it being a genuine crash from some direct bug in the code.

@lukeivers
Copy link

Update: if I set the defaultOptions for my imageView to include DownloadOptionsSkipDecode, it reduces the performance of scrolling on my table to... well, completely non-viable BUT it does completely prevent the crash / memory issues from occurring.

I found this because I finally got some error messages in a console to appear, and they looked like this:

CGBitmapContextInfoCreate: unable to allocate bytes for bitmap data

I got about 10-15 of them, then the app was terminated due to memory pressure.

So I dug around to find out where CGBitmapContext objects were being created, bounced up the call chain until I found out that they're only created if it's decoding in the images in the background thread.

I'm not sure if it's leaking something until the image is finally done downloading, or if whatever function it is that prevents an image from calling it's completion block if it's no longer going to be displayed doesn't prevent it from continuing to render the data it has into bitmaps for display, but it's somewhere right down in that section of code.

Specifically, in lines 100-104 of PINRemoteImage/core/PINImage+DecodedImage.m, if it follows the code path on line 101, it doesn't crash. If it follows the code path on line 103, it does.

@appleguy
Copy link
Contributor

You may need to differently configure your .memoryCache object, e.g. set a different cost limit for the cache.

I'm not too familiar with the intended behavior here — with AsyncDisplayKit (which closely integrates with PINRemoteImage), the framework proactively frees memory for images that are a certain distance from the screen. I believe with a traditional UIKit approach, you either need to manually clear cache elements, or rely on hitting some maximum size (even if it's actually a lot more images than you need).

The disappearing without crash log behavior is definitely consistent with memory kills, so I think you're on the right track there. The question is how you want to free the memory as infinite scrolling proceeds (the bounding factor).

On Sep 16, 2016, at 4:44 PM, Luke Ivers notifications@github.com wrote:

Update: if I set the defaultOptions for my imageView to include DownloadOptionsSkipDecode, it reduces the performance of scrolling on my table to... well, completely non-viable BUT it does completely prevent the crash / memory issues from occurring.

I found this because I finally got some error messages in a console to appear, and they looked like this:

CGBitmapContextInfoCreate: unable to allocate bytes for bitmap data

I got about 10-15 of them, then the app was terminated due to memory pressure.

So I dug around to find out where CGBitmapContext objects were being created, bounced up the call chain until I found out that they're only created if it's decoding in the images in the background thread.

I'm not sure if it's leaking something until the image is finally done downloading, or if whatever function it is that prevents an image from calling it's completion block if it's no longer going to be displayed doesn't prevent it from continuing to render the data it has into bitmaps for display, but it's somewhere right down in that section of code.

Specifically, in lines 100-104 of PINRemoteImage/core/PINImage+DecodedImage.m, if it follows the code path on line 101, it doesn't crash. If it follows the code path on line 103, it does.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub #243 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAigA6bl6YvznDagvzkIEawWzXBBilteks5qqynngaJpZM4J4U8w.

@lukeivers
Copy link

So, I think that part of the issues it that (from what I can tell) it doesn't appear to calculate the cost and validate that against the costLimit until after it has fully materialized the remote URL locally. In the mean time, it's building the progressive images in memory (obviously), and those are consuming nearly or as much memory as the fully materialized object will in some cases.

So when you're doing progressive images, the costLimit is not really an effective method to manage memory consumption.

I really think the issue is largely hinged on progressive images. I understand that the frameworks that layer on top of this have their own methods for managing the memory that may prevent this issue in those cases, but as a separate library overall, there seems to be something missing when it comes to managing memory capacity when downloading progressive images. If a bunch of them get kicked off, it loads the first set of data in and generates a complete scan from it before it even realizes that the overall associated task has been cancelled because the displaying imageView is no longer on the screen.

Luke

@garrettmoon garrettmoon self-assigned this Sep 21, 2016
@garrettmoon
Copy link
Collaborator

garrettmoon commented Sep 21, 2016

@lukeivers so it's not perfect, but there are several ways of limiting the amount of images being rendered at once:

  • You can limit the number of concurrent downloads with setMaxNumberOfConcurrentDownloads:completion:. This is probably not the right solution.
  • You can limit the number of concurrent operations (rendering a progressive scan is an operation) with setMaxNumberOfConcurrentOperations:completion:. This is probably what you want.
  • You can reduce the number of scans rendered per progressive image with setProgressThresholds:completion:. The default is [0.00, 0.35, 0.65] which will result in up to 3 scans. You could reduce this by dropping one or two of those.

Let me know how that works!

@lukeivers
Copy link

I ended up actually switching over to AsyncDisplayKit (which I was mostly only avoiding because I didn’t want to rebuild my UITableViewController, and then I had to do it for other reasons), which totally fixed my issues.

I did have max concurrent downloads set. I did not have max concurrent operations set. However, I think the primary issue is due to the way that cancelling operations/tasks works. It seemed like it would only allow a certain number of tasks to happen simultaneously, but as you scroll quickly, it will start and then cancel multiple tasks repeatedly. However, each of those tasks has started downloading data for a progressive image, and when it receives the data, it checks whether it has enough to render at least one scan of the progressive image. Each one of those consumes some amount of memory, and I don’t think it knows to not try to composite the image from the data it receives (i.e. that the task has been cancelled) until after it’s already done so.

So, if you’re on a slow connection, I think this isn’t as much of an issue, because you won’t get enough data to create even the first progressive scan with each chunk of received data. If you have everything cached, it goes straight to a full image and doesn’t build the intermediate bitmap/contexts, so also doesn’t have issues.

But if you have a fast connection and nothing cached, and scroll a table with a lot of rows (I think I had around 50 or 60), and each image has a large pixel space (most of my images were 4000x5000 or so pixels), it exposes this as an issue.

So, not that everyone is doing that exact thing, I understand that. But I think there’s probably a relatively straightforward method to ensure that cancelling a task guarantees that NO further operations that consume memory (creating bitmaps, etc) will take place for that task, and that would have completely solved the problem for me.

If I had time I’d dig far enough into the task management/scheduling to figure out where to put it in myself and submit a PR but the code was too dense for me to do that in the time I had available.

Luke

On Sep 21, 2016, at 4:13 PM, Garrett Moon notifications@github.com wrote:

@lukeivers https://github.com/lukeivers so it's not perfect, but there are several ways of limiting the amount of images being rendered at once:

You can limit the number of concurrent downloads with setMaxNumberOfConcurrentDownloads:completion:
You can limit the number of concurrent operations (rendering a progressive scan is an operation) with setMaxNumberOfConcurrentOperations:completion:. This is probably what you want.
You can reduce the number of scans rendered per progressive image with setProgressThresholds:completion:. The default is [0.00, 0.35, 0.65] which will result in up to 3 scans. You could reduce this by dropping one or two of those.
Let me know how that works!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #243 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AA6wiFXLj86Sx-pkf2O9OTmEM-HcjYo1ks5qsboagaJpZM4J4U8w.

@lukeivers
Copy link

lukeivers commented Sep 29, 2016

So, after working a lot more with this, I'm pretty sure I've identified what was my base issue. In one case I worked around it via AsyncDisplayKit, but in another I couldn't figure out how to use it (can't figure out how to do ASScrollNode with zooming).

So, the place where I was using it most recently was in an UIPageViewController. I was doing imageView.pin_setImage in viewDidLoad of the child view controllers for the page view controller, and in viewDidDisappear, I was calling pin_cancelImageDownload. Now, for images I didn't have cached, this worked great.

But for images I DID have cached, a quirk of the implementation of UIPageViewController when you're using UIPageViewControllerDataSource (so viewControllerAfter/viewControllerBefore) is that if you swipe VERY quickly between pages, it doesn't properly deinit the ones that are moving off screen. I think a similar thing is happening with a UITableViewController and its cells.

What this meant in practice is that, despite the fact that under normal circumstances I would only have 3 controllers loaded which wasn't too much of a memory issue, if a user swiped quickly, it would call viewDidLoad on multiple (8-10, lets say) controllers and wouldn't deinitialize them, and the cancel download didn't have any effect because it was loading them directly in from the cache.

So it would decode 8-10 (in my case, very large) cached images into memory simultaneously and subsequently get memory killed.

Similarly with UITableViews and scrolling very fast through a table where you have most images cached and they are large images.

Setting a max concurrent operations and max concurrent downloads didn't really seem to have an impact on this.

Is there some way to a) serialize pulling images from the cache (as a configuration option, obviously) and b) have those serialized requests respect calls to cancelImageDownload (or some other function call specifically designed for this)?

I think that would allow this sort of use of PINRemoteImage by just setting a few configuration options, instead of the crazy workaround I had to do (basically I have now created my own queue for calling pin_setImage on the image views in my page view controller, and I check to ensure that the view in question is still within rendering distance (on screen, or just off screen) by the time it gets to calling setImage for it)

@appleguy
Copy link
Contributor

Thanks for the awesome description Luke, Garrett and I really appreciate the opportunity to learn from your use case.

It's certainly concerning to think that UIPageViewController may have a bug related to this. As you may have found with AsyncDisplayKit, there is a formal -clearContents and -clearFetchedData callback, which can be set to trigger a predefined distance away from the screen.

If you haven't tried ASPagerNode for this (with ASNetworkImageNodes, powered by PINRemoteImage) — it should be a tremendous help for managing the memory footprint and loading strategy for large images like you describe. You may want to tweak the default values for the range tuning parameters.

Have you considered using a CATiledLayer if you are displaying content that is higher resolution than the screen? Especially if you want to implement zooming, this would be the correct approach. You could even create an ASDisplayNode with +layerClass of CATiledLayer, and use its -didEnterPrefetchState, -didEnterDisplayState, and the clear methods to manage loading into the tiled layer.

On Sep 29, 2016, at 2:14 PM, Luke Ivers notifications@github.com wrote:

So, after working a lot more with this, I'm pretty sure I've identified what was my base issue. In one case I worked around it via AsyncDisplayKit, but in another I couldn't figure out how to use it (can't figure out how to do ASScrollNode with zooming).

So, the place where I was using it most recently was in an UIPageViewController. I was doing imageView.pin_setImage in viewDidLoad of the child view controllers for the page view controller, and in viewDidDisappear, I was calling pin_cancelImageDownload. Now, for images I didn't have cached, this worked great.

But for images I DID have cached, a quirk of the implementation of UIPageViewController when you're using UIPageViewControllerDataSource (so viewControllerAfter/viewControllerBefore) is that if you swipe VERY quickly between pages, it doesn't properly deinit the ones that are moving off screen. I think a similar thing is happening with a UITableViewController and its cells.

What this meant in practice is that, despite the fact that under normal circumstances I would only have 3 controllers loaded which wasn't too much of a memory issue, if a user swiped quickly, it would call viewDidLoad on multiple (8-10, lets say) controllers and wouldn't deinitialize them, and the cancel download didn't have any effect because it was loading them directly in from the cache.

So it would load 8-10 (in my case, very large) decode 8-10 cached images into memory simultaneously and subsequently get memory killed.

Similarly with UITableViews and scrolling very fast through a table where you have most images cached and they are large images.

Setting a max concurrent operations and max concurrent downloads didn't really seem to have an impact on this.

Is there some way to a) serialize pulling images from the cache (as a configuration option, obviously) and b) have those serialized requests respect calls to cancelImageDownload (or some other function call specifically designed for this)?

I think that would allow this sort of use of PINRemoteImage by just setting a few configuration options, instead of the crazy workaround I had to do (basically I have now created my own queue for calling pin_setImage on the image views in my page view controller, and I check to ensure that the view in question is still within rendering distance (on screen, or just off screen) by the time it gets to calling setImage for it)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub #243 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAigA-7TH68N-MxRAt7QmL9OTFzgNMK1ks5qvCoYgaJpZM4J4U8w.

@garrettmoon
Copy link
Collaborator

Going to close this one out due to a lack of activity.

tinder-garricnahapetian pushed a commit to TinderApp/PINRemoteImage that referenced this issue Mar 22, 2024
pinterest#243)

* Improves testDiskCacheEmptyTrash to specifically test that the shared trash url is used and then removed.

* trashDirPath -> trashPath

* Update testDiskRehydrationOfObjectAgeLimit and testDiskReadingAfterCacheInit to hopefully be a bit more reliable.
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

No branches or pull requests

5 participants