Repository navigation
ImageSharp.Formats.Jpg.DecodedBlock ArrayPool is using up WAY too much memory #151
Description
Prerequisites
- I have written a descriptive issue title
- I have verified that I am running the latest version of ImageSharp
- I have verified if the problem exist in both
DEBUGandRELEASEmode - I have searched open and closed issues to ensure it has not already been reported
Description
I'm basically reporting issue #123 again, but with a slightly different spin. I'm running my little web app on a VM with 1GB of RAM, and I'm constantly getting killed with out of memory because of the ArrayPool being used for ImageSharp.Formats.Jpg.DecodedBlock[].
My app concurrently fetches RSS feeds, then for each entry in each RSS feed concurrently fetches all the images and evaluates all the images for a given entry to see which one will make the best thumbnail. I don't need to get into the large number of feeds before I'm decoding a lot of images in parallel.
After a single fetch session, I dumped the process and examined the heap; I discovered that ImageSharp.Formats.Jpg.DecodedBlockArray.ArrayPool was holding on to 337MB all on its own; the largest entry was 262,144 elements long for a size of 70,254,616. (Obviously that's just the current state of the pool; as currently configured by ImageSharp it can grow way past that.)
I need some way to get a handle on this memory usage; I'm looking at other constraints but some kind of configuration to let me limit the pool would be really nice.
Steps to Reproduce
N/A
System Configuration
- ImageSharp version: 1.0.0 alpha 4
- Environment (Operating system, version and so on): linux
- .NET Framework version: .net core 1.1
Activity
antonfirsov commentedon Mar 24, 2017
@DeCarabas Thank you for your detailed report!
@JimBobSquarePants based on these issues it seems obvious to me, that we need to make our pooling configurable. My proposal:
https://gist.github.com/antonfirsov/25301496604015fbfe7f2b90d608a190
tocsoft commentedon Mar 24, 2017
totally agree with @antonfirsov here. Lets our users limit the max pool size to use... the difficulty might be around the
LimitInBytesas it looks like the total size will be limited not a limit by Type... might have to see if there's a way we can solve that and have it be a shared size (maybe internally we use a common byte[] pool and use Unsafe.As<> to cast them to the correct type thus the array pool will share a common memory limit.)DeCarabas commentedon Mar 24, 2017
I'd take something as simple as the ability to reach in and tweak the individual pool settings. An overall memory usage policy sounds appealing but I think would be (a) hard to implement and (b) hard for me to really tune properly. Whereas I have a bunch of heap dumps and statistics around pool sizes that I can reason about for various pools...
antonfirsov commentedon Mar 24, 2017
Individual pools could be customized by implementing custom
IArrayPoolProvider-s. There will be no such thing asConfiguration.DecodedJpegBlockArrayPoolLimitbecause it's an implementation detail we want to hide.@DeCarabas why do you feel a byte-measured limit is hard to tune?
@tocsoft I would be much happier to operate on "typeless" generic
byte[]buffers but I'm afraidUnsafe.As<>is just too unsafe, and it would lead to bugs like #146.The problem is that
Unsafe.As<int[]>(new byte[1]).GetType() == typeof(byte[]).tocsoft commentedon Mar 24, 2017
ah well that's unfortunate 😞
JimBobSquarePants commentedon Mar 24, 2017
We can certainly make our pool configurable but we have to look at a use case issue here also.
Using 1GB ram for any serious image processing is simply a no-go. You'll run out of contiguous memory in the LOH pretty quickly if you work with multiple images. We're pooling yes but the pool is smart and will not grossly over-deliver on array requests. Making the pool more configurable will still lead to very similar memory consumption as it will end up allocating a new array anyway.
I would like to double check our codebase though to ensure we're not missing something in Jpeg. I'm sure we're good but maybe we can make it leaner than it is.
DeCarabas commentedon Mar 24, 2017
@antonfirsov I appreciate the desire to hide the pools from the caller, but then you're left with several unappealing options.
The problem with option 1 is that picking a good policy is hard-- how do you trade DecodedBlock off vs. Pixel? What if I consume the entire pool with DecodedBlock at the beginning of my program and then never it again, since I spend the rest of the time doing image manipulation on uncompressed pixels?
The problem with option 2 is similar-- if you try to protect the pool for pixels by dividing the limit up "statically", then you waste by reserving memory even when you don't know the pool will ever be used. Why is the array pool for pixels smaller even if I never end up deocoding JPEGs?
The problem with option 3 is that you haven't really hidden the number of pools from me-- you've just made it harder for me to figure out how I should set the limit, since the real limit is now the per-pool-cap times the number of pools. I can do this; I have the memory statistics, I've dumped the heap, I know where to tune, but you haven't hidden anything from me, really, just made it harder to figure out.
DeCarabas commentedon Mar 24, 2017
@JimBobSquarePants Yeah I'm not entirely sure what I was trying to load that allocated 70MB of DecodedBlock; I probably need to do something to protect myself from that anyway.
But the pool is not that smart-- it doesn't have any kind of idle shrinking capability, it doesn't shrink on memory pressure, and it has this annoying power-of-two thing that can end up wildly over-allocating in order to hit the bucket.
The power-of-two thing is annoying because I was not necessarily using that 300MB concurrently-- if I needed an array of 131,073 DecodedBlocks I would have ended up allocating 262,144 of them. In the dump I'm looking at now, I have one such array at that size, for 70MB, and three at 131,072, at 35MB a piece. I only needed to get asked for an array of 65,537 blocks to allocate one of those... at those sizes I would much rather stop using the array pool and ask the allocator for an exact fit instead.
antonfirsov commentedon Mar 24, 2017
@DeCarabas our main difficulties:
We can only solve these by thinking in a general way.
What could be a fair compromise for me: we can introduce
IDecoderOptions.ArrayPoolProviderOverride, so you can tune the pooling for a specific decoding process. It will limit down, or disable pooling forDecodedBlockand temporal jpeg image buffers (JpegPixelArea.Pixels) as well.Also totally agree with @JimBobSquarePants: a fragmented LOH could also lead to
OutOfMemoryException-s.DeCarabas commentedon Mar 24, 2017
@antonfirsov Totally understand on both points; I expect tuning parameters to be something weird and fiddly and likely to break at a moment's notice.
This is always the difficulty with adding caches to libraries like this-- you can't meaningfully add a cache without being in complete control of the resources you're using for your cache, and your library is not in control over the memory in my app. Alas.
Another couple of options:
Either of these would fix my problems without exposing the internals. (I'd personally take the hit of just relying on the GC more heavily over accidentally permanently consuming large blocks of memory.(
tocsoft commentedon Mar 24, 2017
In that case If/when we add the
IArrayPoolProvideryou would be able to make one that fakes out theArrayPool<T>abstract class with one that just allocates arrays and ignores theReturn(...)that will just allow the GC to do its work on the arrays.antonfirsov commentedon Mar 24, 2017
I think we would provide an easy way to disable pooling by setting
Configuration.ArrayPoolProvidertonullor a similar construct. By implementing your ownArrayPoolyou can take even more control, eg. add flushing. (There is no such builtin feature in the defaultSystem.Buffersimplementation ).dlemstra commentedon Mar 29, 2017
If we are going to create an ArrayPoolProvider (sounds like a good idea) we should offer two implementations. One that reuses the pool (our default) and one that just creates a new one all the time. Otherwise we need null checks everywhere we use pools.
BrianThomson commentedon Mar 30, 2017
Same problem here. Using a 13MB JPEG image resulted in 2.4GB private memory allocation. The second upload was even higher. This brings any server down.
antonfirsov commentedon Mar 30, 2017
hmm ... maybe this isn't just normal ArrayPool behaviour, it can be an actual leak.
@BrianThomson I'm planning to reproduce your use case within a console application.
Have you used the same 13MB JPEG multiple times? How many requests resulted in 2.4GB allocation? Is it growing linearly with the number of requests?EDIT:
If this was the result of a single
Image.Load()call, can you post that image?@dlemstra re ArrayPoolProvider:
I think we need to finish this investigation before implementing anything. Also: if I understand you properly, an ArrayPoolProvider returning a new ArrayPool on every request is basically implementing the non-pooling case in a complicated way: allocating 1 instance of
ArrayPool<T>and 1 instance ofT[]at for eachRent()request. I think we need to find something better for this.26 remaining items
vinh84 commentedon Apr 15, 2017
Hi @JimBobSquarePants
64bit IIS Asp.Net 4.6.2
I use procdump -ma pip (w3p.exe)
windbg with psscor4
!dumpheap -stat
antonfirsov commentedon Apr 17, 2017
I've been thinking on this issue for weeks. It is a hard one not only because the tricky implementation details. Defining a public memory management API which allows the right customization options is a non-trivial design task, because the solution has to be maintainable and future-proof.
We need to keep the doors open for the future standard memory management libraries like:
I think I managed to came up with a design which meets all the user requests described in this discussion, and also keeps things extensible. I'd like to introduce a
MemoryManagerclass, which could solve following stuff:ArrayPool-sHere is my proposal
It doesn't deal with implementation details, only with the API. To integrate the MMF stuff, we need to change
BufferSpanto make it work with unmanaged pointers. (Just likeSystem.Spandoes,)@JimBobSquarePants @tocsoft @dlemstra @blackcity @DeCarabas can you have a look?
tocsoft commentedon Apr 17, 2017
Looking good to me.
Only thing I can see is I think
ResetAllPools()should be calledReleaseMemory()JimBobSquarePants commentedon Apr 17, 2017
👍 Bravo. This looks very comprehensive. Agree on the naming change.
Quick Q on this comment. I'm assuming it has to do with .NET Core 2.0 making
Span<T>available or theBufferPool?antonfirsov commentedon Apr 17, 2017
I think only
Span<T>will be finished for that time. We can decide then to:System.Buffers.Primitivesto be finalizedI made a big mistake in the proposal ignoring the fact that
ArrayPool<T>is generic. Gonna fix it now.JimBobSquarePants commentedon Apr 17, 2017
I'm gonna use it being nearly 1am here as an excuse for missing that! 😖
dlemstra commentedon Apr 17, 2017
@antonfirsov In ImageMagick we swap to disk when we run out of memory. Maybe we could make the MemoryManager do that automatically?
antonfirsov commentedon Apr 17, 2017
@tocsoft
ReleaseMemory()might be misleading, specially for an unmanaged MemoryManager, because it does not release anything that is being used by the library at the moment of theReleaseMemory()call. I think we need to find a better name. Renamed it toReset()for now.@JimBobSquarePants I also have an excuse: never should make gists before lunch! :) Updated it now. Things became a bit more complex around the pooling manager.
@dlemstra It might be possible to implement a
MemoryManagerwhich does swapping, but it seems to be a really difficult at the moment. It might be good enough for our users if we allow them switching to MMF over certain buffer sizes (see theJpegMMFexample).blackcity commentedon Apr 17, 2017
Looks good. I like that you provide extensibility, because the library itself cannot implement optimized code for all conceivable runtime environments, 👍
antonfirsov commentedon Sep 16, 2017
@DeCarabas @BrianJThomson If you are still using ImageSharp, check out the beta (on NuGet!), the Jpeg decoder uses much less memory now! Let me know if the situation is still critical for you!
rdcm commentedon Dec 6, 2017
@antonfirsov did you mean SixLabors.ImageSharp 1.0.0-beta0002?
JimBobSquarePants commentedon Dec 6, 2017
Beta 1 contained the relevant changes, beta 2 builds on those changes