Repository navigation
More efficient MemoryAllocator #1596
Description
Activity
Idea 2 is interesting. I wonder, though, if you can manage discontinous buffers, is it much of a stretch from there to go fully streamed?
For the record, I also hate idea 4, but that may be necessary if you really do have to materialize large images and care at all about 32-bit hosts. The machine I ran my tests on was only a quad core as well. I wonder what would happen on a 16+ core machine... I reckon even in 64-bit you'd start paging.
I wonder, though, if you can manage discontinous buffers, is it much of a stretch from there to go fully streamed?
Implementing the ability to have discontinous buffers was relatively easy, since our processing code usually goes row-by row. We just had upgrade
buffer2d.GetRowSpan(y)so it can take the row from different buffers (buffer boundary is always between rows). Going fully-streamed needs much bigger architectural changes, I would rather start with the "decode into target size" first, and see where we can get from there.Currently we have a very high limit for images being split to different buffers, what I'm considering is to check what happens if we aggressively lower that limit. I'm unable to assess if it would make thing worse or better.
but that may be necessary if you really do have to materialize large images and care at all about 32-bit hosts.
That's why I want to avoid materializing them :) Will open an issue for tracking "decode into target size" stuff tomorrow.
The machine I ran my tests on was only a quad core as well. I wonder what would happen on a 16+ core machine...
I was wondering why do are using the basic
Parallel.Foroverload that scales parallelism with the machine's CPU count? Is this a way of modeling typical server-side multi-threaded stress?I'm unable to assess if it would make thing worse or better.
I don't think it would make things worse. If I understand right, your current mode of processing is to apply each transform to the entire image before moving on the next? If so, you're kind of trashing the cache at each stage, so it wouldn't matter whether the memory is contiguous or not. I'm not sure it would make things better either, though. You might want to look at your LOH fragmentation under those high memory conditions and see whether being able to stuff a smaller buffer into an LOH hole would gain you anything. Switching to unmanaged buffers may help a bit there, but heap fragmentation can be a problem in unmanaged as well.
Of course being able to aggregate small buffers in place of a single larger one would be of benefit, provided you don't need them at the same time. I assume if that works for RecyclableMemoryStream, it should work for ImageSharp 🤷♂️
Is this a way of modeling typical server-side multi-threaded stress?
I must admit I didn't put that much thought into it. But yeah, the point was to make sure there were enough threads running to check whether the libraries were capable of true linear scale-up. As long as there are free processors to hide work on (as Vips does today -- and ImageSharp did before your windowed convolution implementation), you can't be sure. If you think about a high-load web server scenario, you would assume the managed thread pool would fully occupy all processors, so I want to know how a library will perform in that environment -- and to be sure that a large number of image processing operations doesn't make for a self-DoS.
BTW, I suspect the anomalous CPU measurements I got for Vips running under the profiler might have actually come from an oversubscription problem. Could be the CPU overhead of the profiler was just enough to throw the whole system into chaos and drop the speed by several multiples. I plan on doing some followup testing to see if I can nail that down.
Couple of other interesting stress cases you might want to look at.
- 3400+ frame GIF: http://i.imgur.com/Xv4UKYL.gif
- Lots of good giant JPEGs available on the ESO site, like: https://www.eso.org/public/usa/images/eso1719a/
The reason I'm thinking about "discontiguous memory by default" is not cache coherence, but the fact that it may enable more efficient pooling, reducing the GC pressure (or OS allocation traffic in case of unmanaged buffers). With large images it is very likely that a the requested buffer will outgrow the pool deferring to GC/OS allocating new giant arrays of non-uniform sizes, which would probably result in heap fragmentation even if we are using unmanaged memory.
I'm thinking of a solution that works as following:
- Under 1MB we use a standard ArrayPool (can be
ArrayPool<byte>.Shared) - Over that, we utilize a custom pool that provides uniform 2MB buffers. We then build the large discontiguous buffers aka. MemoryGroups out of these 2MB subbuffers.
- This pool might be built of LOH arrays or unmanaged buffers. (Both approaches seem to have pros and cons.)
- Obviously there is an upper limit for the retained/pooled memory
- If we outgrow the discontiguous pool, we may consider to defer new allocations to unmanaged memory, so at least we don't stress the GC
- There is a logic that periodically shrinks the pool so the solution works well with ImageSharp.Web (which typically places much lower load on the core library after a warmup period, so it doesn't make sense to retain memory in the pools)
- The periods can be synchronized with actual GC calls with a trick similar to
Gen2GcCallbackinTlsOverPerCoreLockedStacksArrayPool. For example we may halve the retained memory before each gen2 GC.
- The periods can be synchronized with actual GC calls with a trick similar to
@saucecontrol any thoughts on this particular plan? I wish I could pull in a GC expert to assess it ...
- Under 1MB we use a standard ArrayPool (can be
That plan looks sound to me, but I'm no GC expert. I do expect that inability to grab a contiguous large block of memory due to LOH (or process heap) fragmentation is at play in the 32-bit OOM scenario, but it would be good to verify that before diving in to the work if possible.
It seems like if you end up doing the work to completely manage the allocations, you may as well go straight to unmanaged memory so that when you say to free something you don't have to then wait for GC to comply and then do its LOH compaction and all that. Plus, you'll probably always have to deal with some allocation requests that will be over whatever pooling threshold you set, and you'll want those to go straight to unmanaged so they are as short-lived as possible.
you may as well go straight to unmanaged memory so that when you say to free something you don't have to then wait for GC to comply and then do its LOH compaction and all that
The problem is that I don't really know when to free up memory. With LOH + the
Gen2Callbacktrick I can at least rely on GC heuristics with this. (Otherwise it feels like implementing my own GC)@JimBobSquarePants it just allocates / returns unmanaged memory without pooling. Maybe it's good enough (or even preferable) approach with unmanaged memory, not sure.
My main concern about unmanaged memory is that I would be hesitant to use it as a default, because of the users who may implicitly depend on not disposing
Image. An update would introduce a reliability / security issue for them instead of their existing performance issue.Instead of holding the raw
IntPtras a member of theMemoryManager<T>, that should hold aSafeHandlewrapping it. Something like:internal sealed class SafeHGlobalHandle : SafeHandle { private readonly int byteCount; public override bool IsInvalid => handle == IntPtr.Zero; public SafeHGlobalHandle(int size) : base(IntPtr.Zero, true) { SetHandle(Marshal.AllocHGlobal(size)); GC.AddMemoryPressure(byteCount = size); } protected override bool ReleaseHandle() { if (IsInvalid) return false; Marshal.FreeHGlobal(handle); GC.RemoveMemoryPressure(byteCount); handle = IntPtr.Zero; return true; } }
Since
SafeHandlehas a finalizer, it won't leak, but you can callDisposeto free it immediately if theMemoryManager<T>is Disposed properly.It might be worth implementing something like that for any allocations larger than your max pool buffer size just because you know those will happen and will have a large GC cost. It's also a lot easier to trial than the segmented buffer idea.
Reacted by James Jackson-South, Anton Firszov and Fabien BarbierFYI dotnet/runtime#52098 is tracking some possible scenarios to improve in ArrayPool/GC interaction. I think Jan's scenarios 1 and 3 (and maybe 2 as well) are applicable to ImageSharp.
Reacted by Anton FirszovThere are so many things I hate about ArrayPool, especially
ArrayPool<T>.Sharedwhich is a library-level public shared global state, and as such something that shouldn't exist at all IMO.All the magic ArrayPool.Shared is doing should be a runtime/GC implementation detail hidden behind standard
new T[]allocations. (Edit: OK, it was mostly ranting, this approach would require an API likeGC.UnsafeReleaseArray(...), but but AFAIK array pooling is much less common in Java for example, I assume GC does better job there)Hi everybody!
Have a couple of thoughts on this project I'd like to share. Thanks for such a good topic on memory performance.First of all, current state of pooling already supports discontiguous buffers. It's just hidden behind
MemoryGroup<T>.Allocate(,,,)call:First it checks if single stride can fit in single contiguos buffer:
Then it does strange things:
ImageSharp/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs
Lines 99 to 107 in a8cb711
Details aside - this alloc call either throws or pools single contiguous buffer each time. So here goes first problem: most of the time pool is stressed by huge arrays from buckets of higher end of indices when low indices are empty which makes
maxArraysPerBucketconfig value completely irrelevant.
Edit: this can't be fixed by simply rewritingMemoryGroup<T>.Allocate(,,,)call, at least resizing processor assumes that entire underlying Buffer2D of pixel data is a single contiguous buffer and throws specific exeption. Haven't checked other processors but I guess it might also be a problem somewhere else.Edit: huge mistake in calculations and hardcoded upper bound of pool max size (it's hex, not dec), this still implies but for certain pool setups, see post below.
But there's a far more complicated problem: pooling doesn't work in certain situations. This is solely due to current implementation of the pool in .net, it silently clamps maximum size of the array:const int MinimumArrayLength = 0x10, MaximumArrayLength = 0x40000000; if (maxArrayLength > MaximumArrayLength) { maxArrayLength = MaximumArrayLength; }
Which leads to unexpected LOH allocations when working with images up to ~40mb of pixel data and that isn't very rare:
rgb24 1080p jpeg is 24 * 1920 * 1080 = ~49mbof decoded pixel data.
Bee heads test completely ignores pooling because it consists of rgb24 jpeg`s with at least 3000x3000 pixels.Parallel.Foreach bee heads can skyrocket memory usage (been counting gigabytes of work set during execution) with frequent GC(2) collections.
27 remaining items
Very late to the party (super interesting conversation by the way!), just thought I'd throw this out there too as it seems somewhat relevant to the new issue Anton opened, and potentially useful?
This is essentially meant to be a more reliable and less hacky way to get memory pressure based callbacks than what is achievable today by using
Gen2Callbackthat Clint mentioned 😄@Sergio0694 yeah, seen that, cool stuff, thanks a lot for pushing it!
Reacted by Sergio PedriReacted by Sergio PedriI made some progress with a prototype on a branch. As a next step, I need to start some benchmarking to determine optimal parameters, however I'm not sure what the pool sizes are considered to be still "sane".
It can be any value between a few megabytes, up to several gigabytes on many-core systems. Note that dotnet/runtime#55621 went very aggressive for
ArrayPool<T>.Shared.Maybe we could define it as a percentage of
TotalAvailableMemoryBytes? (However it won't work well automatically with stuff like docker's--memoryswitch.)Reacted by James Jackson-SouthWow, the limit has been removed entirely!
So what's you're current plan then? An admittedly very quick look at your branch yields to me what seems to be 3 pools now?
ArrayPool<byte>.Shared<= 1MBUniformByteArrayPool- Which doesn't appear to do any cleanup? <= 48 MBUnmanagedMemoryAllocator> 48MB
Isn't using the
UniformByteArrayPoolin this manner going to lead to the same issue as we are seeing where we hold on to memory long after it is useful?I feel like the
DefaultMemoryAllocatoris currently offering too many options in its constructor and that users will find them confusing (I find the two contiguous options confusing and I know how the library works). If someone wants to wrap memory or cares about contiguous buffer length perhaps they should simply switch to theSimpleGcMemoryAllocatoror roll their own?@JimBobSquarePants it's still in a prototype state. Trimming is not yet implemented, first I want to figure out the optimal parameter sizes for the pool size and other things. Current WIP default is 256 MB, 48 MB is too low for stress cases.
I think we should hide the implementing type (
DefaultMemoryAllocatoror whatever) and make the public API implementation-agnostic this time:public class MemoryAllocator { public static MemoryAllocator CreateDefault( int maxContiguousBlockSizeInBytes = DefaultContiguousBlockSizeInBytes, int totalPoolSizeInBytes = DefaultTotalPoolSizeInBytes); }
We can introduce utilities to help interop use cases, which we will likely break by scaling down the default contiguous buffer size (see #1307 and #1258):
public static class SizeCalculator { public static GetImageSizeOf<T>(int width, int height); } // Customize the allocator for GPU interop use cases: Configuration.Default.MemoryAllocator = MemoryAllocator.CreateDefault( maxContiguousBlockSizeInBytes: SizeCalculator.GetImageSizeOf<Rgba32>(4096, 4096));
Reacted by James Jackson-SouthYep. I like all these ideas!
@JimBobSquarePants @saucecontrol I was already at final steps polishing my implementation, when I discovered an issue with the
UniformByteArrayPoolapproach. Looks like the GC is unable to decommit it's LOH pages despite trimmingUniformByteArrayPoola couple of times (or even clearing it entirely with a call to.ReleaseRetainedResources()) if the pages are even slightly interleaved with other LOH allocations (typically coming fromArrayPool.Shared).The following graph shows the committed memory for an experiment which attempts to free all the memory in the end:
For comparison here is the same experiment, with a slight alteration: setting
GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOncebeforeGC.Collect(), showing that after compacting the LOH the memory is decommitted:
What to do now?
We can do better job by getting rid of GC for large buffers entirely, and refactoring
UniformByteArrayPoolinto a pool of unmanaged buffers, but this could possibly prolong the PR for a couple of weeks.To deliver something to the users in the meanwhile, we can also consider to PR an unmanaged allocator without any pooling. From what I see now in large scale stress benchmarks,
UniformByteArrayPooladds about 5% to the throughput in comparison to a non-pooling unmanaged allocator, but a non-pooling unmanaged allocator has already 9% better throughput than our currentArrayPoolMemoryAllocator, and much better memory characteristics.Reacted by James Jackson-SouthOh wow! Interesting and slightly disappointing result. I'm glad you've shown your level of thoroughness. I would have likely missed something like this.
A few more weeks is no problem. It gives us time to review and merge the WebP code and fix a few more issues. Plus I'm trying to focus on Fonts and Drawing right now to get them closer to RC status.
@jaddie there is a WIP PR #1730, and an API proposal in #1739, discussions shifted there. The required change is so fundamental, that we decided to do a major version bump, so our next release containing the memory fix will be ImageSharp 2.0.
The memory PR is expected to be finished within 1 or 2 months (not that much work left, but I'm doing it in my free time), however we also want to include WebP in that release, which may prolong things too. Hopefully we will be still able to deliver 2.0 before the end of 2021.
If you are considering to help: I need users with production-like environments to test and validate #1730. Let me know if you are interested.
@antonfirsov With regard to the issue that LOH cannot decommit memory, have you tried the GCSettings.LargeObjectHeapCompactionMode Property?
Yes, the second graph in #1596 (comment) is made with
GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnceset in the end.The problem is that I'm not sure if it's a good practice to touch that property from library code. The compaction comes with extra GC cost, unexpected by the user, and we don't know when / how often to compact. I'm not excluding that it can be a lesser evil in our case than going unmanaged and sacrificing the simplicity of our pixel manipulation APIs as a result (see #1739), but I can't commit to such a decision without consulting a GC expert.
@cshung if you think you have some time to chat about this, let me know, I would really appreciate the help!
The problem is that I'm not sure if it's a good practice to touch that property from library code. The compaction comes with extra GC cost, unexpected by the user, and we don't know when / how often to compact.
Not compacting the LOH automatically by default is a bit sad. Historically, we never compacted the LOH, some customers depended on that fact and assume LOH allocated objects are pinned, so we cannot automatically compact the LOH without some kind of user opt-in, otherwise we might break some users. The
GCSettings.LargeObjectHeapCompactionModeproperty seems to solved most people's pressing issue and therefore we were not looking into it.Those days are long gone, now we can automatically compact the LOH with minimal configuration. Starting with .NET 6, you can specify the
GCConserveMemorysettings. It is a number ranging from 0 to 9, indicating how much you want the GC to work to conserve memory instead of giving the best possible speed.Unfortunately, the setting is not documented yet, it will be. For the time being, we can explore what the setting could do by looking at the code here(*).
If you search for
compacting LOH due to GCConserveMem setting, you should find the relevant logic for deciding LOH compaction. As an overview, the implementation is checking the fragmentation ratio (i.e. how much percent of memory is wasted as free space). If it is larger than a threshold derived from theGCConserveMemorysettings, then it will turn on LOH compaction.This should alleviate the need to set that property - whether or not to compact the LOH is best left for the GC to decide.
(*) Whatever we actually do in the code is an implementation detail that is subjected to change, we do need the flexibility to avoid painting ourselves into a corner, like what we did with LOH not compacting.
@cshung if you think you have some time to chat about this, let me know, I would really appreciate the help!
I am more than happy to reach out to developers like you who care about garbage collection performance. My personal goal is to understand the needs and ideally come up with benchmarks that are representative to work on. What is the best way to reach you?
Reacted by Anton Firszov@cshung thanks a lot for the answers!
Starting with .NET 6, you can specify the
GCConserveMemorysettings.From our perspective the problem with an opt-in setting is that we can't configure it on behalf of our users. Even if we would promote it in our documentation, most users would still miss it, and complain about poor scalability compared to unmanaged libraries like skia(sharp). It's much better if the library "just works" without any extra configuration thanks to good defaults.
For now I decided to go on with our switching to unmanaged memory because of two reasons:
- The compaction issues discussed above
- Unmanaged memory has better characteristics when allocations are overflowing the pooling threshold, and the images are disposed immediately. (this instead of this)
However, thinking in longer terms, this feels wrong to me. Ideally, a managed library should be able to meet all of it's requirements using managed memory only. Would be cool to switch back to GC in a future version. I wonder if ImageSharp is some sort of special animal here, or are there other memory-heavy libraries or apps facing similar issues.
My personal goal is to understand the needs and ideally come up with benchmarks that are representative to work on. What is the best way to reach you?
That's great to hear, we can chat on Teams I think.
One way to address the concerns in #1590 is to come up with a new
MemoryAllocatorthat meets the following two requirements even under very high load:(A) Steady allocation patterns, instead of GC fluctuations
(B) Lower amount of memory being retained, at least "after some time"
Some ideas to explore:
1.1 Consider pooling unmanaged memory, especially if we implement the next point.
Point 1. seems to be very simple to prototype. We need an allocator that uses
Marshal.AllocHGlobalover some threshold, and anArrayPoolbelow it, and see how the memory timeline goes with the bee heads MemoryStress benchmark in comparison toArrayPoolMemoryAllocator.@saucecontrol any thoughts or further ideas? (Especially on point 2.)