Implement pluggable memory management

[antonfirsov]

Problem

• In some use cases the default ArrayPool behavior does not fit user needs (see Encoding an animated image requires all frames to be loaded into memory, can lead to OutOfMemoryExceptions #222). Users should be able to configure + reset + disable pools.
• Some users may want to back Image<T> by memory resources other than managed memory. (Eg. native buffers).

Solution

Implement a pluggable MemoryManager, make the default pooling memory manager configurable.

Update:
feature/memory-manager is the WIP branch for this refactor. It's having #431 merged. If anyone wishes to contribute, please file your PR-s against that branch. The tasks below are being updated according to the progress on that branch.

Tasks for beta-3

• Refactor all raw ArrayPool and PixelDataPool usages to use Buffer and Buffer2D instead.
• Buffer2D<T> should compose Buffer<T> (as IBuffer<T>) instead of inheriting it
• Move the responsibility of providing Buffer<T> instances to a generic MemoryManager class. Refactor all new Buffer<T> and new Buffer2D<T> instantiations to call factory method on MemoryManager instead.
• We need to cover all the encoders/decoders with proper regression tests before continuing the refactor work. (Working on this - would be filed as a separate PR)
• Extensive test coverage for ArrayPoolMemoryManager
• Allocations in DefaultPixelBlenders<TPixel> should use MemoryManager taken from the outside
• Replace the legacy PixelArea<TPixel> class with BufferArea<T> + extensions methods working on BufferArea<TPIxel> and/or IBuffer2D<TPixel> using PixelOperations<TPixel>
• Replace all most PixelAccessor<TPixel> usages with Buffer2D<T>. Drop the IBuffer2D<T> interface. - the full removal could be done later.
• MemoryManager should construct IBuffer<T> and IManagedByteBuffer instead of Buffer<T>, codecs should use IManagedByteBuffer.Array
• Based on user feedback, there also might be some strange, unexpected leaking when images are created/destroyed continously. Doing some load testing, identifying+understanding this behavior is inevitable!

Probably 1.0

• Passing custom MemoryManager to codecs

Most likely post- 1.0:

• Passing custom MemoryManager to Mutate() and Clone() (---> Allow passing custom Configuration instances to processors #650)
• Investigate new System.Memory API-s: Memory<T> and
  OwnedMemory<T>. Find a way to integrate these classes into ImageSharp.Memory.
• Image.Wrap<T>(Memory<T> memory) or similar
• Implement an MMFMemoryManager which uses Memory Mapped Files to reduce the pressure on system memory when processing large images.

[vpenades]

About processing large images, there's a common issue in .Net which is that very large memory chunks are not moved during a GC, which can result in out of memoty exceptions even when there's enough memory available.

A solution for this I found in the past was to use a low level bitmap primitive that split images into chunks of 64x64 pixels, all wrapped with a generic IBitmap interface. That way, relatively small images, or images that would require very fast procesing (requeted upon image creation) would use an IBitmap that simply wraps an Array. Large images would use instead an IBitmap implementation that stores multiple chunks.

[Toxantron]

@vpenades the reason the CLR does not move objects larger than ~85kB is the negative performance impact. Objects that big are stored in the LOH. Splitting the images into movable chunks disables that optimization and will negatively influence performance.

A large image still reserves the some amount of memory if split into chunks. Using the power of streams and the filesystem seems to be the better solution here.

[vpenades]

@Toxantron Not everybody is looking for performance; some years ago I was tasked to process images on the size of about 20.000 x 15.000 pixels, which allocated around 1gb RAM, in a single chunk. We also discovered that when compiling for x86, even if theoretically you can allocate 4gb, the Net Framework runtime was actually limited to 2gb only, essentially limiting the load of a single image of such size.

In practice, it was nearly impossible to load such image, because any time we had any object fragmenting the LOH, it was impossible to allocate more than 800 or 900mb in a single chunk, and ended with out of memory exceptions.

We looked for every single image library available at the time (+5 years), and we found all libraries of the time allocated a single chunk.

We ended developing our own custom image library which splitted images in chunks of less than 64kb, to allow the runtime to defragment memory, at the cost of performance.

It is true that time has passed, memory is not that scarce as it was 5 years ago and x64 is commonplace these days... but don't assume people is going to use image libraries the same way, or for the same reasons as you use them. For example, there's lots of areas related to scientific analysis, or design artists working with large form factors, which work with extremely large images.

[dlemstra]

@vpenades Did you take a look a ImageMagick back then? This can handle extremely large images.

p.s. I am one of the authors.

[antonfirsov]

@vpenades An MMF-based memory manager could achieve the same tradoff (running slower, but using less memory) in a more simple way.

BTW there is a new property GCSettings.LargeObjectCompactionMode since 4.5.1 to address the LOH fragmentation issue.

[Toxantron]

@vpenades thank you for the clarification.

@antonfirsov I fand that value as well. It comes with performance impacts, but at least we can leave it to the GC then and not implement our own version.

[vpenades]

@dlemstra I think we tried, but if I recall correctly, we had std:boost compiling issues with other third party libraries.

@antonfirsov that's great to know the GC also has the compact large objects feature, I wish I had it a while back.

@Toxantron 😄

[JulianRooze]

Some feedback on this: we currently use ImageSharp for all our image resizing needs in a cloud application and the optimization of speed over memory usage seems to run counter to the idea of horizontal scaling, because of how it holds onto memory. We'd like to provision many containers with a 2 GB memory limit, but currently if we do that, we'd end up with all those containers using 2 GB all of the time. Currently, we run only a few containers limited to 4 GB and they frequently get OOM killed by Docker, even though I already limited image resizing concurrency. This high memory usage seems to be mostly caused by a few outliers in image sizes, most images we process are in the 500 KB range, but some are 10-20 MB, which then consumes 500+ MB of memory just to decode that gets cached indefinitely. Do that concurrently and memory quickly starts to climb to 2-4 GB which never gets released.

From this issue, I can tell you're working on making it configurable and pluggable, but it sounds like a lot of very fundamental work and somewhat long-term. Is there any chance you would accept a more short term quick-fix? Correct me if I'm wrong here, but from what I can tell, most of our memory usage comes from the caching of Block8x8 which is done by the Jpeg decoder, and the memory is provided by the PixelDataPool. If you were to add properties to Configuration that allowed you to set the maxArrayLength / maxArraysPerBucker that get passed to ArrayPool.Create, you could turn off/tweak array pooling for a common scenario.

Something like:

Configuration.Default.Memory.PixelDataPool.MaxArrayLength = 0;
Configuration.Default.Memory.PixelDataPool.MaxArrays = 0;

I understand that this would expose some internal implementation details, but I think (haven't tried it yet, might later) it would also solve some real issues.

[antonfirsov]

@JulianRooze I plan to realize my roadmap in February, so it will be part of our 1.0 release or maybe even some beta. Is this late for you?
The only thing I can propose as a quickfix is is to add a temporal configuration API to control the behavior of PixelDataPool for beta-3:

[Obsolete("This is a temporal API, use it for your own risk!")]
public static class PoolConfiguration
{
    [Obsolete("This is a temporal API, use it for your own risk!")]
    public static int MaximumPooledBufferSizeInBytes { get; set; }
}

@JimBobSquarePants what do you think?

[vpenades]

Question: I thought the behaviour of ArrayPool was to keep the returned arrays as weak references, so if not used, or the GC needs memory, these arrays would be reclaimed by the GC at some point.

But from what I am reading, does this means the ArrayPool keeps the arrays forever??

[antonfirsov]

@vpenades

[...] if not used, or the GC needs memory, these arrays would be reclaimed by the GC at some point

The GC runs quite frequently, which means that in this case the "pooled" arrays would be GC-d instaneously in most cases. This would go against the whole concept of pooling.

@JimBobSquarePants an other (less dirty) proposal:
By lowering the value of MaximumExpectedImageSize we could ensure that those large outlier images do not eat up the memory of users with limited environments, while still having the pooling mechanism for more common smaller images.

[vpenades]

@antonfirsov So, from what you're saying, I understand the only proper way to use ImageSharp is in a short lived command line application that starts, does some image processing, and exits soon after?

What about these use cases:

• Game App uses ImageSharp to procedurally generate textures at startup, them runs for four hours, without using ImageSharp anymore. (*)
• Paint Application, running for hours, where the artists loads and saves images of wildly different image sizes.
• Any other long running application that needs image processing, but also uses a lot of memory for its own purposes, for example, scientific analysis.

(*) We wanted to use this technique as a trick to reduce application size on Android/iOS Apps. But, given the limited memory of these devices, it's simply not acceptable to leave an ArrayPool sitting there for the rest of the application's lifecycle... In Androd/iOS, 100mb can be the difference between staying alive or getting killed by the OS.

** It's not like I neglect performance optimizations, I really really appreciate the length of the efforts you're all doing trying to optimize and improve ImageSharp's performance... but, a memory object that, once it's no longer used can't never be reclaimed by the GC is, by all means, a memory leak.

Found this: ArrayPool's author not fond on ArrayPool.Shared for this very same reason.

[JulianRooze]

@antonfirsov Thanks for the quick reply! No that's not late at all, much sooner than I expected it to be completed 👍

And in the meantime the PoolConfiguration class would be a very welcome addition.

Limiting MaximumExpectedImageSize would also help, but the effect would be smaller as Block8x8 buffers seem to make up the majority of cached buffers (at least in our use case of decompress JPEG > resize > encode as JPEG).

This is the cached memory after resizing a large (20 MB, 8000x5000) JPEG concurrently (5 at a time) in a loop:

[JimBobSquarePants]

@antonfirsov @vpenades @JulianRooze

This is all great input, thanks for contributing!

Regarding PoolConfiguration I don't think adding a temporary API would be a wise idea. I'd much rather we focused on getting the design right with the help of the community.

If we continue our design discussion here and also ensure that the PR is delivered as WIP so that interested parties can chip in as it's developed then I'm sure we can deliver something powerful.

Regarding Block8x8 We'll definitely have to alter our behaviour within PixelDataPool<T> to act a bit smarter. We currently create a block of 50 arrays at an arbitary length (matching the default in ArrayPool.Shared) per T. @JulianRooze You've highlighted the issues there well.

Is 50 too much in this instance? Definitely. Per jpeg we max out at 4 Components. I would probably limit this pool to 4 x Processor.Count to handle parallelism.

Whatever we go with, let's ensure individual implementations are provided enough information to make good decisions and work granularly.

I see this stuff as a major priority so am happy to delay beta 3 until we have it in place.

@vpenades I did not know that about the shared pool. We don't use that for Buffer<T> and co but we might elsewhere still.

[vpenades]

@JimBobSquarePants @antonfirsov Just think that one of the case scenarios in which we wanted to use ImageSharp was on an android App that is required to do some image processing at startup and then carry on with other stuff.

On 1Gb Ram Android devices, apps throw OufOfMem when they go close 700mb usage, So after ImageSharp processing is completed at startup, we need the GC to free every single used byte for the App, even small byte arrays for small images... on Android/iOS, every byte counts.

My suggestions:

• Avoid ArrayPool<>.Shared (or any other kind of statically allocated pool that can't be released) at all costs... it only makes sense for a number of very specific, very controlled use cases, in others it's a plain memory leak.
• If required, use local, disposable ArrayPools, like for example, in the JPEG encoder/decoder (the pool would be disposed after encode/decode)
• I don't know the exact architecture of how processors work, but when you call Mutate, an instance of IImageProcessingContext is passed along several processors. Maybe that context could contain a local ArrayPool to be consumed by the processors using that context. No need to dispose because the ArrayPool itself would be reclaimed by the GC afterwards along with the processingcontext.

[antonfirsov]

@vpenades you are totally right about the issues you pointed out! I share all your concerns and fixing up our internal memory management is my top priority as soon as I get back to work on ImageSharp in February!

I just want to point out that having expensive resources retained by a pool for a longer period is just normal pooling behavior by definition. (Just have a look at other pooling mechanisms in the .NET framework. ThreadPools, connection pools etc.) So I disagree with your suggestions: having temporal "pools" is not pooling. It would hurt performance for the majority of our server users. (The vast majority of our user base!)

The big issue is that our pooling mechanism is not configurable at the moment, which makes the library to perform poorly in many scenarios, like yours. As I said, this is a top concern for me! I believe that solving this by providing generalized memory management is a very worthy strategy in long term, because it will enable cool features, big flexibility + integration with new Microsoft API-s & other libraries.

[antonfirsov]

Re ArrayPool<T>.Shared:
For me it was clear from the beginning, that it's a design mistake :P We are not using it AFAIK. We concentrated all our "Memory as a Resource" logic into PixelDataPool and Buffer<T> classes, so it would be easier to refactor + customize this behavior library-wide.

[antonfirsov]

@JulianRooze it's strange to see Jpeg decoder to still eat up that much. Are you using beta-2?

[antonfirsov]

Btw. the logic in CalculateMaxArrayLength is totally stupid. Gonna replace it in a lightweight PR in a way that will also help on the jpeg decoder + Block8x8 issue.

[JulianRooze]

@antonfirsov yes, we're running beta-2. It's a rather extreme image though, at 8000x5000.

[JimBobSquarePants]

@antonfirsov I'm hoping some of the new SIMD API's coming will allow us to run DCT without having to use singles. We could drop the whole thing by 75% then.

[denisivan0v]

Gonna replace it in a lightweight PR in a way that will also help on the jpeg decoder + Block8x8 issue

@antonfirsov When you expect to do this?

In my case the problem is the same - we run a web service in Docker container with very limited memory resources, so I will be highly appreciated for a quick changes. We're mostly using png and jpeg decoders.

For now is there any way for me to make some kind of PoolConfiguration (as you proposed earlier) and make a private build?

[tocsoft]

The way I had envisoned the updated IMemoryManager based api working would be as so.

We would add a new interface IMemoryManager and expose a public property on Configuration called MemoryManager. Also add to IImageEncoder IMemoryManager MemoryManager {get;set;} for overriding the memory manager on a per encoder bases.

All places that currently call Buffer<T> or Buffer2D<T> should be replaced to calls that back onto the MemoryManager sourced from eather the encoder or the images configuration.

Proposed interface design.

   public interface IMemoryManager 
   {
       IBuffer<T> Allocate<T>(MemoryUsageHint hint, int size);
   }

    public enum MemoryUsageHint 
    {
        // very short term buffer, things like a temp buffer for 
        // passing around a single row of an image
        Tempory, 
        
        // lifetime for an entire ImageProcessor action we should be only requesting 
        // a few of per-image prrocessor run, but will usually be a 
        // larger size, most likely the size of the image buffer
        Process,

        // this memory will live for the lifetime of the image 
        //itself (unless switched out during resize) 
        Image,
   }

   // this is basically an interface wrapper around some owned memory
   public interface IBuffer<T> : IDisposable 
   {
       // this is used to effeciently switch the backing data
       // from one buffer with another, we do this in some ImageProcessors
       // some backign providers could to a pixel copy, others could 
       // just return false for not supported, where as other would acutally
       // switch out the backing array from each.
       bool SwitchBuffer(IBuffer<T> buffer); 
       int Size { get; }
       Span<T> Span();
       Dispose();
   }

We would then have extension methods targeting IMemoryManager that exposes Buffer2D<T> Rent2DBuffer<T>(this IMemoryManager mng, MemoryUsageHint hint, int width, int height) and probably just some simpler IBuffer<T> TemporyBuffer<T>(this IMemoryManager mng, int size), IBuffer<T> ProcessBuffer<T>(this IMemoryManager mng, int size), IBuffer<T> ImageBuffer<T>(this IMemoryManager mng, int size) to simply internal usage.

notes

Our inital implementation for this should probably do the following.

• if MemoryUsageHint == Tempory the we use a array pool if size < a tempThreashold else we use new array
• if MemoryUsageHint == Process the we use a array pool if size < a procThreashold else we use memory mapped file (or just a simple array for now)
  if MemoryUsageHint == Image new array if size < imgThreshold else we use memory mapped file (or just a simple array for now)

The Array bool can acutally be backed by a single byte[] pool the the IBuffer<T> can use Span.UnsafeCast<T>()(or what ever the api is) to convert the backing byte array into a Span<TPixel> allowing us to use a single array pool for all all the memory across all the pixel types.

For the few places its acutally not possible to get a IMemoryManager to (Regions spring to mind) then we can pobably jsut get away with using a separate, less used, array pool that's entirely divorced from the memory manager until the API can be amended to get access to it.

[tocsoft]

As we would be required to expose IBuffer<T> on the public API then we should probably add new constructor to Image allowing to pass in a buffer to be used as the images backing store. (this would then allow us to have images that are backed by memory owned by other processes.)

[JimBobSquarePants]

var image1 = ArrayPoolMemoryManager.Default.CreateImage<Rgba32>(512,512);

This API is semanticly troublesome and unintuitive. If someone came to the library as a first time user I would be incredibly surprised if they logically surmised loading images in this way.

It's a graphics library, and, as such, the semantic focus has to centre around the Image<T> class.

my feeling is that the default "easy to use" API favours a certain use case of ImageSharp, which is running continuously as a web service, and makes it utterly complicated for all other use cases.

I strongly disagree here. Your proposed API gives us no advantage, that I can fathom, over passing a specific memory manager to an image constructor since any factory based logic could be internally derived.

[vpenades]

Okey then, I'll give it a try

[0xGeorgii]

Hi all, I faced with this memory consuming issue (you can see details here). If you need some help with the implementation I can grab some.

[JimBobSquarePants]

Hi @GeorgePlotnikov ,

@antonfirsov is point on this issue and is planning on working on it in the latter half of this month, I'm sure he'll welcome any use-case information you can provide. 👍

[antonfirsov]

@GeorgePlotnikov even answering a few questions would be very helpful:

• Do you use beta-2 or nightlies? If you work with beta, can you try nightlies, and report back? There was some improvement with PixelDataPool<T>: reduce maximum pooled array size #436!
• Based on your screenshots, it looks to me that your app's memory consumption stops growing at 160MB. Is my understanding correct? Or have you just stopped stressing your application?
• This is really not a classic "memory leak"! There is an optimization tradeoff: the more memory you allow to be taken (and "never returned") by ArrayPool-s, the less is the pressure on GC, increasing your applications throughput. In ideal case, this is how the memory consumption should grow with time in your app. What is the acceptable level of maximum retained memory for your application?

If you need some help with the implementation I can grab some.

I want to finish this work before the end of the next week. If you really have some time to join in now, let me know, and I gonna break down the remaining work into smaller tasks. The more help I get from the community, the more time remains for me to work on #411 (documentation)! 😄

[antonfirsov]

For everyone's invormation:

The work started with #431 is now being continued on the feature/memory-manager branch.

This is the API I'm targeting currently:

interface IBuffer<T> : IDisposable 
{
    Span<T> Span { get; }
    // No T[] is exposed!
}

interface IManagedByteBuffer : IBuffer<byte>
{
    // Exposing an array to work with .NET API-s consuming byte[]
    byte[] Array { get; }
}


// I suggest using an abstract class instead of an interface,
// because I don't want to make the methods and IBuffer<T> public at this point!
public abstract class MemoryManager
{
    // Let's keep the interface as narrow as possible! 
    // There will be shortcuts like AllocateClean() through extension methods.
    internal abstract IBuffer<T> Allocate<T>(int size, bool clear)
        where T : struct;

    // Mostly used by decoders working with streams.
    internal abstract IManagedByteBuffer AllocateManagedByteBuffer(int size);

    internal abstract void Release<T>(Buffer<T> buffer)
        where T : struct;

    public abstract void ReleaseAllRetainedResources();
}

@tocsoft I suggest adding MemoryUsageHint later, when we are more familiar with specific solutions like unmanaged memory and MMF.

[vpenades]

Some considerations:

What about the newly introduced Memory<T> as a wrapper of Span<T> , wouldn't IBuffer<T> and IManagedByteBuffer be a redundancy of Memory<T>??

Memory<T> article here.

What about mixing memory managers? As I understand from past conversations, if you want to create an image with a custom manager, it needs to be passed as an argument like this:

new Image(FancyMemoryManager, 512, 512);

So, under the same application context, it will be possible to create/load images using different memory managers.... and my concern is, what happens when you mix resources with different managers in the same pot.

I ask, because I've seen there's some "SwapBuffers" being used in some processors, so if the SwapBuffers swap memory created with distinct memory managers, it can lead to trying to release a buffer with the wrong manager.

Also, some processors seem to require extra memory, like creating temporary images, etc... which is going to be the policy here? will they use the memory manager associated with the "current image", or they will need a memory manager to be passed to them too?

[antonfirsov]

@vpenades

Memory as a wrapper of Span , wouldn't IBuffer and IManagedByteBuffer be a redundancy of Memory??

Memory<T> is not a wrapper of Span<T>. They are technically different views of the same thing. IBuffer<T> is the counterpart of OwnedMemory. I will consider switching to OwnedMemory as soon as it's on NuGet + I see more use cases & docs to understand it's reference counting logic better.

What about mixing memory managers?

I think it should be a goal to allow this, but not in the current iteration. UPDATE: I was thinking of composite "mixed" memory managers. We should definitely investigate what happens if different memory managers are used, and interfere in the same process.

I ask, because I've seen there's some "SwapBuffers" being used in some processors, so if the SwapBuffers swap memory created with distinct memory managers, it can lead to trying to release a buffer with the wrong manager.

Good point, we need to check this behavior in processors, and define rules for buffer moving & ownership management. In some cases copying them might be inevitable.

will they use the memory manager associated with the "current image", or they will need a memory manager to be passed to them too?

Passing a MemoryManager to Mutate() and Clone() could be a solution.

There are lots of tasks and concerns. In the current iteration I would like to focus on the absolute minimum listed in "Tasks for Milestone 1.0". Even implementing this takes more time than I expected. It is a deep codebase refactor and brings tons of seemingly unrelated tasks like #469.
We can add future points like Composite memory managers, or Memory<T> integration later in an iterative way through independent PR-s. As always, community contribution is welcome. If you feel some of those points are important, and urgent for your use case, feel free to grab them! 😄

[vpenades]

@antonfirsov

The worst case scenario I can foresee in the long run is this:

Let's say ImageSharp suceeds, and it leads to an explosion of ImageSharp based libraries; Obviously, it's up to how sloppy the developers of these ImageSharp derived libraries are, but you might end having derived libraries with these behaviours:

• Derived libraries using the default memory manager.
• Derived libraries using a custom memory manager.
• Derived libraries letting the consumer to pass a memory manager.

And at some point, a naive developer happily importing and mixing these derived libraries from nuget into a big application.

The issue about mixing managers is not only about how to do it, but also if it is allowed to be done.

From the point of view of a large application consuming ImageSharp derived libraries from different sources, it should be convenient to set an application wide memory manager... but if the API allows creating derived libraries that use a custom manager internally, overriding the application wide manager.... I think you can see my point.

In the end, I don't know how to solve this... we have developers with wildly different memory management needs, so some sort of configuration mechanism is required... but letting configure the memory management can lead to interoperability problems between derived libraries, which is a very bad thing in the long run.

[antonfirsov]

The situation is not that bad. I guess there is a solution evolving on my branch:

• The method MemoryManager.Release() is gone now. IBuffer<T> implementations are now coupled to their MemoryManagers, containing their own dispose/release logic.
• The swapping trick introduced by @tocsoft, is in fact coupled to owners of buffers, not the IBuffer<T> itself, and I'd like to keep it that way! (Buffer2D<T> is also a thin wrapper over IBuffer<T>, thus a buffer owner now.)

Let me know if I'm wrong, but I think these rules are sufficient to keep buffer ownership logic safe, and prevent different MemoryManager instances from interfering with each other.

[vpenades]

@antonfirsov Another curve ball (sorry, doing lots of tinkering with images lately)

I'm developing a small 3D scene structure, basic geometry, transforms, materials and textures for in-memory manipulation.

The scene contains nodes, the nodes contain materials and the materials contain textures, the graph allows things like two different materials sharing the same texture, The idea is that everything works with managed memory for ease of use and simplicity.

Now, since ImageSharp's image is IDisposable, the only way to properly dispose the textures is to make the material class IDisposable, and the nodes, and all the graph down to the tree root. And since texture instances can be shared, I would have to implement reference couting... this is definitely not what I want.

I know removing the IDisposable from Image, or even adding an image type not requiring IDisposable is out of the question, so I was planning to use my own bitmap container (managed, and not requiring Dispose), and only when I need to do some ImageSharp operation, do something like this:

using(var image = new ImageSharp(texture.GetBytes(),texture.Width,Texture.Height))
{
    image.Mutate(dc => dc.Resize(256,256) );
    texture.SetNewContent(image.Width, image.Height, image.GetBytes());
}

Would this be possible with the planned memory manager?

[antonfirsov]

@vpenades the method you are looking for is Image.LoadPixelData<TPixel>(data, width, height)!

Later, it will be also possible to wrap an existing memory area to be able to temporarily manage it as an "Image". Dispose() will have no effect in the case if memory ownership is external. This feature requires the availability of Memory<T>, so we can have a clean & standard API for that.

[vpenades]

@antonfirsov Okey, I've tinkered a prototype of the managed image class I would use here.

[0xGeorgii]

Hello @antonfirsov, apologise for the delay in the reply.

1. Currently, I'm using a beta
2. Yes I stopped stress my application
3. I think I don't want to consume more than 200 MB in single thread mode

[antonfirsov]

@GeorgePlotnikov thanks for the answers!

Can you try if it's any better for you with the latest nightlies? Would be very helpful information for future fine-tuning tips!

[0xGeorgii]

@antonfirsov, okay will do it and come back with a feedback.

[0xGeorgii]

@antonfirsov below the result

the process was finished with

An unhandled exception of type 'SixLabors.ImageSharp.Formats.Jpeg.GolangPort.Components.Decoder.EOFException' occurred in System.Private.CoreLib.dll
Reached end of stream before proceeding EOI marker!

[antonfirsov]

Thanks!

That's still quite too much of memory being eaten up :(

The exception that ended your experiment might be a Jpeg bug ... or a totally corrupt "jpeg", that can't be loaded by any library. Can you share the file?

[0xGeorgii]

@antonfirsov sorry, but not. I fetch files randomly 😄

[vpenades]

@antonfirsov

Checking the implementation of Buffer<T> IDisposable and Finalizer, I've noticed the finalizer is not disposing everything it should.

The current finalizer implementation here only unpins the GCHandle, but it doesn't free the actual buffer as the Dispose method does.

I am aware there's probably a lof of changes with the upcoming memory managers, so I don't know if this detail is being tracked already.

My point is that an array built from a memory manager must be considered as an unmanaged resource, which means it is eligible to be released in the finalizer, so I guess it would look something like this:

~Buffer()
        {
            this.UnPin();

            if (this.isPoolingOwner)
            {
                PixelDataPool<T>.Return(this.Array);
            }
        }

The problem is that, as far as I know, the GC does not guarantee the order in which managed resources resources are freed, so, it might happen that this.Array has been already freed if it was created by the "NullMemoryManager".

For my use case scenarios, I was considering to rely more on the finalizer, and less on the Dispose(), but I can see a loophole here (that is, to rely on the finalizer to release resources):

With the current implementation, where the finalizer only does UnPin();

• For images created with a PooledMemoryManager that requires releasing the resource, the finalizer would never return the memory back to the pool, causing a memory leak.
• For images created with a NullMemoryManager, it would work fine.

Now, let's fix the finalizer and add the release of the array:

• For images created with PooledMemoryManager, the resource would be returned back to the pool. Since an instance of the array exists in the pool, it is guaranteed the Array object exists within the finalizer execution context.
• For images created with NullMemoryManager, it can happen the GC has freed the Array before the Buffer's finalizer is called, so the call to the NullMemoryManager's Return might cause unexpected behaviour, maybe a GC exception.

Anyway, I think the finalizer behaviour needs to be clarified, and in any case, It must free any resources that the GC cannot free on its own.

The only way I can think to easily fix this is the MemoryManagers to have a property that tells if the resources needs to be returned back to the manager, so for the NullMemoryManager, the Buffer would set isPoolingOwner to false, preventing the finalizer to do "weird things" with a potentially disposed object.

[antonfirsov]

@vpenades It's not allowed to touch managed objects in a finalizer! It's only here to free up the unmanaged handles if the developer forgot to call Dispose().

A developer should not rely on finalizers, you should properly manage the disposal of your objects.

[vpenades]

@antonfirsov Indeed!, that's precisely my point!

[antonfirsov]

Sorry, misunderstood your comment. It's a bug in my code! Gonna be fixed.

[antonfirsov]

I'm almost done, a PR is coming soon.
However .. I'm not yet happy. Based on the feedback in this + other threads, there is some strange behavior I haven't understood yet. 👻 The memory keeps growing for users even when I supposed it should stop doing so.

We need to do a proper load testing investigation before releasing beta-3!

[antonfirsov]

I'm closing this epic, because it's 99% done. Individual issues like #650 should cover the rest.