PixelDataPool<T>: reduce maximum pooled array size #436
Conversation
Codecov Report
@@ Coverage Diff @@
## master #436 +/- ##
==========================================
+ Coverage 80.7% 80.71% +<.01%
==========================================
Files 514 514
Lines 20267 20272 +5
Branches 2212 2213 +1
==========================================
+ Hits 16357 16362 +5
Misses 3227 3227
Partials 683 683
Continue to review full report at Codecov.
|
|
Many thanks @antonfirsov, but in my cases it's still a big amount of allocated memory since in I'm looking forward to #431 and now rely on these changes to do my tests. |
|
I've read the ArrayPool API docs and the DefaultArrayPool implementation. Those parameters mean that you've got up to 50 32MB arrays, up to 50 16MB arrays and so on -- the first argument being the max size within a bucket and the second being the maximum number of "similarly-sized" arrays within a single bucket of the pool. The size selection of DefaultArrayPool for a bucket works in powers of two, and you'll always get the size that's the smallest available but at least as large as you asked for. Out of curiosity, I ran the bucket count and bucket size algorithms for a DefaultArrayPool with those parameters, and the absolute maximum retained size would be just over 3 GB if every single array was in use. Of course I know that it's extremely unlikely that any allocation pattern will hit very close to that mark, but even a 50% fill factor will swallow a large portion of the memory space in 32 bit or in a low-memory container. Add to this the fact that you've got one ArrayPool for every TPixelData you use within the app: for instance, if all you do is construct Rgba32 images from Jpegs, you'll have one ArrayPool of Block8x8's and another of Rgba32's. -- this is the exact case I had when I came along. This means that the max retained size for the pools is north of 6GB. I'd suggest that instead of dropping the max size of an individual array, you'd drop the max bucket count instead -- or, possibly, both. If you try to allocate beyond what the pool is capable of holding, it will just return a regular heap-allocated array instead. (Caveat: I'm currently undergoing a case of the Man Flu, so there's a possibility that I've messed up my math at some point. If you wish to double-check, the sources are at https://github.com/dotnet/corefx/blob/master/src/System.Buffers/src/System/Buffers ) |
|
@rytmis good experiment and useful calculations! Gonna reduce I'm also unsure about my math, but if your service is kinda stateless (so buffers are always returned to the pool when a request is finished), you need to stress your application in a way so it reaches a maximum peek of ~10 concurrent For me it feels that if this is a realistic scenario in your environment then:
But instead of guessing, I really want to model this by defining proper load testing benchmarks. It's not easy for me to design them to be realistic, so any input and review would be really appreciated! |
|
My scenario is a batch processor that will download and convert images according to a number of different presets. After each resize, the resized image is stored on disk and the image itself is disposed, so that should mean having at most two images in memory at a time. No concurrent processing. After running a batch where there were lots of sequential resizes happening, my memory usage peaked somewhere around 1,5 GB and the app began to slow down (I'm guessing GC and disk thrashing due to page file usage). Note that this is a scenario that has no such issues with System.Drawing. I'm not talking hypotheticals here: what I did was take an existing batch app that targeted the desktop framework and ported it to run on CoreFX, replacing System.Drawing with ImageSharp. The app has been running for over two years without any memory-related issues, but the ported version choked almost immediately. The main problem the current pool approach is that if you happen to load a single image that is, say, 90 MB in size, you'll permanently retain 128 MB of memory in that pool bracket, even when it has been released back to the pool. If you then load a 130 MB image, you'll permanently retain 256 MB. So going through a series of images that get ever larger, you'll eventually exhaust the heap space even when you'd only need enough capacity for the one you are currently working with. In other words, if you ever, at any point, allocate and then release one array from each bucket up until you reach the 130 MB mark, you will have retained at least half a gigabyte until the app terminates. Multiply that by the number of pixel formats, and... you get the picture (pun not intended). (These calculations assume byte arrays -- I'm not sure, but for TPixel arrays this might change the constants by a little bit. In any case, it's still the same order of magnitude). |
|
@rytmis That's pretty much the same scenario I have. I still believe that using pools for caching lots and lots of very small, similar sized arrays, like handling server messages with lots of small POCOs, it's fine.... But images are too large, if the pool's memory cannot be completely disposed by the GC, or at will, there's many scenarios in which you can end up with your application running with less than half of available memory. In fact, if input images can be uploaded by users, AND an attacker knows that you're using that kind of pool, at can devise some sort of attack by uploading carefully sized images that would fill all the pool's buckets rapidly, rendering the whole service underperforming, or throwing OutOfMem quite often. @antonfirsov , so, filling all the buckets of the pool is unlikely, unless we consider attacks to bring down a server scenarios. |
|
@rytmis I think you really need outliers of size Whether (updated graph) I'm quite sure I will be able proove this later with benchmarks. This limit might be lower for certain applications + scenarios, pretty high for others, but never zero, if you optimize for throughput. It's really unfortunate, that bugs we made in ImageSharp (+lack of possibility to customize stuff) led to a debate on ArrayPools with those strong counter-opinions 😞 If you are using ASP.NET MVC, you already have ArrayPools in your application! |
|
@antonfirsov I'm not against ArrayPools, I agree they provide a performance benefit, I also want speed too!. 😄 What am I against is statically allocated, non disposable array pools. From your chart, what I want is that if you're no longer performing any sort of image processing, the memory usage to drop back to zero. |
|
@vpenades totally understand it. You'll be able to implent this kind of logict after merging #431. |
|
Apologies if I sounded like I don't like ArrayPools -- that was never my intention. 🙂 I was just trying to point out potential issues in this particular mitigation strategy, that's all. |
so we never have more than 8 buckets for large buffers for a single value type.
|
I'm merging this now. If anyone is about to try our next nightlies because of this change: your feedback is really appreciated. |
Prerequisites
Description
It might be a longer process to finish #431, however users experiencing
OutOfMemoryException-s need a stable ImageSharp for yesterday :) In this PR I try to provide a solution for memory issues reported in #225, #409, #325, #224 and #151 by a simple change:The maximum pooled buffer size is now limited to 32MB (equivalent to 8 megapixels of raw
Rgba32data). It's 32-times lower forImage<Rgba32>than before. I think most images are going to fit into this size without eating up the memory of the process while still using pooled memory. For outliers however, it might be better new-up those arrays. (You can still run out of usable memory because of LOH fragmentation issues! I suggest to touchGCSettings.LargeObjectHeapCompactionModeregularly in environments with strict memory limits!)The same byte limit is used for non-
IPixelvalue type arrays. This should probably fix the issues caused by pooledBlock8x8buffers in JpegDecoder.We'll keep working on #431 to provide a full-scale solution. @rytmis already did the majority of the work, big thanks for that! (However we still need to fix the array conversion issues, provide proper test coverage, polish the API further before merging it.)
/cc @denisivan0v @xakep139 @vpenades @JulianRooze