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

Potential Memory Issue when using any processors such as Resize #776

Open
alexjameslittle opened this issue Apr 17, 2024 · 11 comments
Open
Labels

Comments

@alexjameslittle
Copy link

alexjameslittle commented Apr 17, 2024

We're seeing issues when fetching images for the first time and resizing them with LazyImageView, the memory usage continues to rise indefinitely. However we store those resized images on disk using nuke and on next app launch when nuke uses the already resized images, the memory usage is normal.

If we don't use any processors we don't see any issue with memory.

Edit: This seems to also be reproducible without processors if you use skipDecompress in the image request options

@alexjameslittle
Copy link
Author

Just following up on this with more information, we've discovered this only happens with .storeAll. If we use the .storeOriginalData option instead then this issue goes away.

@kean
Copy link
Owner

kean commented Apr 18, 2024

Hey @alexjameslittle,

Could you please share the pipeline configuration that you are using? What is the usage pattern? Is the app loading large number of images with high resolution?

the first time and resizing them

Resize and decompression use a lot of memory because both of these operations require bitmapping the image, so high memory usage is to be expected, especially if displaying large images.

the memory usage continues to rise indefinitely

Would you mind sharing some more details about how you are measuring the memory usage?

There a distinction between persistent and transient memory. The only persistent memory the framework uses is managed by the internal memory cache that has strict limits and clears automatically on memory warnings. As long as the app periodically clears used memory, a high memory usage should not going to be a concern. It's nearly impossible to get a modern app to crash because of the high memory usage. It only happens if you hit a hard memory limit.

You can find some more information about performance and caching in the documentation. It can help you guide how to adjust the configuration or the usage patterns to best fit your app's needs.

@alexjameslittle
Copy link
Author

alexjameslittle commented Apr 18, 2024

Hey @kean,

Of course, our pipeline is setup like this:

let pipeline = ImagePipeline {
    let dataLoader: DataLoader = {
        let config = URLSessionConfiguration.default
        config.urlCache = nil
        return DataLoader(configuration: config)
    }()

    $0.dataLoader = dataLoader
    let path = FileManager
        .default
        .containerURL(forSecurityApplicationGroupIdentifier: config.appGroupIdentifier)!
        .appendingPathComponent(Self.cacheFolderName)
    let cache = try? DataCache(path: path, filenameGenerator: Self.filenameGenerator)
    cache?.sizeLimit =  1024 * 1024 * (5 * 1024) // 5GB
    $0.dataCache = cache
    $0.isStoringPreviewsInMemoryCache = true
    $0.isProgressiveDecodingEnabled = true
    $0.dataCachePolicy = .storeAll
}

We've been using Nuke for almost 2 years without using any of the processors and we've not seen memory usage grow like it has until we started trialling the processors for resizing and blurring, we do expect an increase in memory when doing these operations, however it seems there is an usually large growth in memory that just doesn't happen if we don't ask the pipeline to store all images, we believe it is most likely a memory leak.

We're seeing the memory usage of the app quickly going over 1gb after a scrolling through 20-30 resized images (jpegs at ~200kb, 1080x1440...resized to approx). If we use the .storeOriginalData option instead we do not have this issue. This indicates to me there is nothing wrong with the processing/processors but rather storing the processed images to disk after processing.

If there is anything else I can do to help debug this, please let me know!

@kean
Copy link
Owner

kean commented Apr 18, 2024

There are multiple things that will contribute to high memory usage:

  • Blurring
  • isStoringPreviewsInMemoryCache - more images in the memory cache
  • progressive image decoding – both computationally and memory-expensive
  • .storeAll – requires encoding for processed images, which can also be expensive

If you combine these four things together, and the app does in fact load progressive JPEGs, I would expect an extremely high memory usage. It isn't necessarily a problem if that's what you are going for in terms of the UI/UX. The high memory usage is not the indication that the app will be terminated by the system.

trialling the processors for resizing

Resizing is usually a good thing. It will cause a quick spike in CPU and memory usage, but will ultimately result in a smaller footprint.

If we use the .storeOriginalData option instead we do not have this issue

It seems unlikely, but I would not exclude is as a possibility. I don't think it's a popular option. I personally always go for storing the originals. I don't have a lot of practical experience with encoding and I'm not sure what performance characteristics to expect from it.

I also wouldn't recommend using .storeAll unless there is a clear need to.

If there is anything else I can do to help debug this, please let me know!

I would appreciate if you could run the Leaks instruments and see if there are any.

@alexjameslittle
Copy link
Author

The high memory usage is not the indication that the app will be terminated by the system.

Yeah, we're aware we have a much higher ceiling to go to with the memory usage generally before the app gets terminated, however in this instance the memory usage is constantly rising to a point where which is causing visible performance issues to the user, if you continue to scroll through even more images the app then does terminates itself due to excessive memory usage.

Whilst we have set the isStoringPreviewsInMemoryCache and isProgressiveDecodingEnabled options, we're not actually serving progressive images anymore.

We chose storeAll as users are often viewing the same images over and over again between sessions (their own memories) so we wanted to store those resized images to avoid having to do the work again on every app launch.

We did check the leak instruments but there is no visible leaks in instruments, we will revert to storeOriginalData for now as this fixes all of our immediate issues, however I think it's worth keeping this issue open. Would it be helpful for us to send you an example project that illustrates the issue?

@kean
Copy link
Owner

kean commented Apr 18, 2024

if you continue to scroll through even more images the app then does terminates itself due to excessive memory usage.

It does indicate that the might be a leak somewhere.

Would it be helpful for us to send you an example project that illustrates the issue?

Yes, that would be ideal.

@kean kean added the bug label Apr 18, 2024
@alexjameslittle
Copy link
Author

Where is best to send you the project as we'd prefer not to upload it publicly as it's for the company I work for.

@kean
Copy link
Owner

kean commented Apr 18, 2024

If it has any closed source code, I would not recommend sharing it. I should be able to do an investigation based on the details you provided.

@alexjameslittle
Copy link
Author

We created an example app without any closed source just now that demonstrates the issue. Feel free to use it to help debug.

nuke-test.zip

@kean
Copy link
Owner

kean commented Apr 20, 2024

Thanks @alexjameslittle.

I've tested the demo project and haven't identified anything beyond what I would expect, considering the size of the images. On my device, the memory footprint never grows beyond ~500 MB. When I background the app, it frees almost all of the memory, confirming that it's not persistent and that there are no leaks.

Screenshot 2024-04-20 at 1 05 00 PM

I believe the main cause of the performance issues is the sheer size of the images (2000×2000px and higher) and the target size of the resize processors (1500×1500px on @3 devices). You can get a ~5x improvement in memory and CPU usage by reducing the target size used for ImageProcessors.Resize. Here's the result of dropping it to 400×400px which is more suitable for the use case.

Screenshot 2024-04-20 at 1 10 24 PM

I would also recommend loading images of a smaller size, if possible, to reduce the networking footprint.

Image encoding does use a lot of CPU and memory, so if you disable it, you get an even better result. I haven't found any opportunities for optimizing it – it's just expensive.

Screenshot 2024-04-20 at 1 08 14 PM

I did identify a regression in one of the recent Nuke versions that broke one of the optimizations that would skip redundant image decompression and prepared a fix #777. This issue is not as impactful as I would expect it, but the fix will ship in the upcoming version.

I'm also planning to remove the .points option (the current default) when specifying target size for images. It makes little sense now considering that Apple ships @3 devices and you typically don't want to display @3 images as they would be just too large. It will also help make the code fully concurrency compliant/safe.

@alexjameslittle
Copy link
Author

hey @kean,

I can also confirm the memory does get cleared on backgrounding, however for now we're going to go with your original suggestion and not store all images, only storing original image data. In terms of networking footprint & resizing, that demo application isn't an exact replica of what we have internally, our own pipeline is much more optimised but we will keep in mind all of your suggestions going forward.

Good to know about the decompression fix & the removal of points based for resizing, I will keep an eye out for these updates!

Thank you for investigating and the detailed response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants