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

NSAllocateMemoryPages fails in ImageDecoder #64

Closed
adomanico opened this issue Apr 11, 2016 · 14 comments
Closed

NSAllocateMemoryPages fails in ImageDecoder #64

adomanico opened this issue Apr 11, 2016 · 14 comments
Labels
improvement Non-functional change that improves existing functionality

Comments

@adomanico
Copy link
Contributor

Seeing crashes come through in prod for cases where ImageDecoder is failing to initialize the UIImage with data

Image decoder failing to create UIImage

Fabric is reporting that most of these devices have low free memory.

What can be done to handle this exception?

@adomanico
Copy link
Contributor Author

Also, we were not seeing this in 2.2.0 Nuke.

This appeared once we upgraded to 3.0.0

@kean
Copy link
Owner

kean commented Apr 11, 2016

Hmm, I can't think of anything that could have affected this. I've mostly just removed some excessive features in new versions.

@adomanico
Copy link
Contributor Author

Please let me know if there is any thing I can do / any more information I can provide

@kean
Copy link
Owner

kean commented Apr 11, 2016

My first step would be to check the app for memory leaks.

@adomanico
Copy link
Contributor Author

Similar issue in ImageLoader

Going to fire up instruments and check as well

Edit: fixed link

@adomanico
Copy link
Contributor Author

So after profiling what I am seeing are substantial memory increases happening in ImageDecompressor

ImageDecomp

since our app loads multiple map images simultaneously many of these workers are dispatched and will continue until in some cases the device runs out of memory

Multiple threads

@adomanico
Copy link
Contributor Author

Still digging but if you see anything here that looks off let me know

@adomanico
Copy link
Contributor Author

Also, not seeing any leaks, just seeing the Nuke image cache grow really large

@kean
Copy link
Owner

kean commented Apr 12, 2016

I've checked diffs and there are at least two things that might have affected this:

  1. ImageLoader reuses NSURLSessionTasks for equivalent requests (for example, same URL). After the data is loaded it needs to be decoded (NSData -> UIImage). In Nuke 2.2.0 the data was decoded once for all of the registered equivalent requests. Now it is decoded once for each request. Note: this change was made to simplify implementation to support custom on-disk caching, it is reversible though.
  2. The default ImageLoadingView protocol implementation no longer resizes loaded images by default. You have to opt-in to resize images. Note: I've found resizing to be ineffective when images size is close to the view size.

Could any of these changes affect your app? More specifically, do you start (intentionally?) multiple equivalent requests for the same URL? Did you rely on automatic resizing of loaded images?

@adomanico
Copy link
Contributor Author

Yeah definitely is case 1.

We have a map interface where we re-query the same images if the user scrolls around (its tile based).

Is there a change we should make on our end , or is this something you will address?

@adomanico
Copy link
Contributor Author

I think for now I'm going to just throw the map UIImages in an NSCache as a temporary solution until you have a release ready

@kean
Copy link
Owner

kean commented Apr 12, 2016

Is there a change we should make on our end , or is this something you will address?

There's a performance regression on my end. But you could stop making too much tasks; maybe cancel tasks when the tiles go offscreen.

I think for now I'm going to just throw the map UIImages in an NSCache as a temporary solution until you have a release ready

Nuke already has a built-in memory cache.

I'm going to ship a release that fixes those regressions in a matter of days, maybe even tomorrow. I also have some new performance improvements in mind. I would suggest to stick with version 2.2.0 for now.

@kean kean added the improvement Non-functional change that improves existing functionality label Apr 12, 2016
@adomanico
Copy link
Contributor Author

Ok awesome, I will block duplicate requests.

Thanks for the fast response

@kean
Copy link
Owner

kean commented Apr 15, 2016

@kean kean closed this as completed Apr 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Non-functional change that improves existing functionality
Projects
None yet
Development

No branches or pull requests

2 participants