WIP: swappable memory pools #431

rytmis · 2018-01-11T21:26:27Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This is very much a work in progress -- so far, I've moved Buffer construction to use a singleton MemoryPool with an ArrayPool backed implementation.

- Add a null memory manager that doesn't do any actual memory management

CLAassistant · 2018-01-11T21:26:33Z

All committers have signed the CLA.

antonfirsov · 2018-01-12T00:32:06Z

@JimBobSquarePants check out #225 and Gitter conversations if you want (+have some time) to keep track of this! :)

JimBobSquarePants · 2018-01-12T00:33:15Z

Will do! 😄

code just before going to sleep

need it (aside from a few exceptions)

codecov · 2018-01-12T13:35:14Z

Codecov Report

Merging #431 into master will decrease coverage by 0.03%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
- Coverage   80.72%   80.68%   -0.04%     
==========================================
  Files         512      514       +2     
  Lines       20152    20151       -1     
  Branches     2196     2200       +4     
==========================================
- Hits        16267    16259       -8     
- Misses       3210     3212       +2     
- Partials      675      680       +5

Impacted Files	Coverage Δ
src/ImageSharp/Memory/NullMemoryManager.cs	`0% <0%> (ø)`
...harp.Drawing/Brushes/Processors/BrushApplicator.cs	`44.44% <0%> (ø)`	⬆️
...mageSharp.Drawing/Processors/DrawImageProcessor.cs	`100% <100%> (ø)`	⬆️
...eg/PdfJsPort/Components/PdfJsQuantizationTables.cs	`96.42% <100%> (+0.27%)`	⬆️
...ts/PixelBlenders/DefaultPixelBlenders.Generated.cs	`96.42% <100%> (ø)`	⬆️
...ing/Processors/Effects/BackgroundColorProcessor.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Processing/Transforms/Resize.cs	`98.46% <100%> (ø)`	⬆️
src/ImageSharp/Processing/Overlays/Glow.cs	`83.33% <100%> (ø)`	⬆️
src/ImageSharp.Drawing/Processors/FillProcessor.cs	`88.23% <100%> (ø)`	⬆️
src/ImageSharp/Memory/Buffer{T}.cs	`91.22% <100%> (-1.08%)`	⬇️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f438da...22206f1. Read the comment docs.

JimBobSquarePants · 2018-01-13T07:40:56Z

src/ImageSharp/Memory/ArrayPoolMemoryManager.cs


        /// <inheritdoc />
-        internal override Buffer<T> Allocate<T>(int size, bool clear = false)
+        internal override Buffer<T> Allocate<T>(int itemCount, bool clear = false)


Let's use two distinct methods here with overloads. I'm not a big fan of using optional parameters in inherited methods; it can often lead to breaking changes since this could well become a public API.

rytmis · 2018-01-12T13:35:04Z

src/ImageSharp/Image/ImageFrame.LoadPixelData.cs

            Guard.MustBeGreaterThanOrEqualTo(data.Length, count, nameof(data));

-            var image = new ImageFrame<TPixel>(width, height);
+            var image = new ImageFrame<TPixel>(Configuration.Default.MemoryManager, width, height);


I think this usage of Configuration.Default makes sense here.

I disagree. We should pass the MemoryManager from ImageFrameCollection<TPixel> to this method!
This is the MemoryManager responsible for the allocation of all the Image<T> pixel data!

rytmis · 2018-01-12T13:35:52Z

src/ImageSharp/Image/PixelAccessor{TPixel}.cs

        /// <param name="height">The height of the image represented by the pixel buffer.</param>
        public PixelAccessor(int width, int height)
-            : this(width, height, Buffer2D<TPixel>.CreateClean(width, height), true)
+            : this(width, height, Configuration.Default.MemoryManager.Allocate2D<TPixel>(width, height, true), true)


This is a bit unfortunate, but parameterizing this spreads pretty wide. What do you think?

Unfortunately we need to propagate that parameter here :(
It's possible to do this incrementally by defining a temporal construct with an optional memoryManager = null parameter and calling memoryManager ?? Configuration.Default.MemoryManager.Allocate2D<TPixel>(width, height, true).

We should get rid of PixelAccessor anyways, so leave it for now if you want

rytmis · 2018-01-12T13:36:24Z

src/ImageSharp/Image/PixelArea{TPixel}.cs

            this.Length = this.RowStride * height;

-            this.byteBuffer = MemoryManager.Current.Allocate<byte>(this.Length, true);
+            this.byteBuffer = Configuration.Default.MemoryManager.Allocate<byte>(this.Length, true);


Same as above -- taking this as a ctor dependency has far-reaching consequences.

If you feel the chore work is just too much, leave it then :)

We should get rid of both PixelAccessor<T> and PixelArea<T> anyways. They are legacy constructs.

rytmis · 2018-01-12T13:37:30Z

src/ImageSharp/PixelFormats/PixelBlenders/DefaultPixelBlenders.Generated.tt

                    Guard.MustBeGreaterThanOrEqualTo(amount.Length, destination.Length, nameof(amount.Length));

-                    using (Buffer<Vector4> buffer = MemoryManager.Current.Allocate<Vector4>(destination.Length * 3))
+                    using (Buffer<Vector4> buffer = Configuration.Default.MemoryManager.Allocate<Vector4>(destination.Length * 3))


This is the hardest usage to change, because while I could take this in as a ctor parameter, I'd have to pass in something for the singleton instances, and the only thing available... is Configuration.Default.MemoryManager.

Yeah this is actully forcing us to make API design decisions. I think it should be a parameter of Blend(). @tocsoft thoughts?

rytmis · 2018-01-12T13:39:28Z

src/ImageSharp/IImageProcessingContext{TPixel}.cs

        /// <returns>The current operations class to allow chaining of operations.</returns>
        IImageProcessingContext<TPixel> ApplyProcessor(IImageProcessor<TPixel> processor);
+
+        MemoryManager GetMemoryManager();


In order to be able to pass the currently-used memory manager to the processors that need it, the easiest way I could think up was to expose the MemoryManager from the processing context.

rytmis · 2018-01-12T13:41:40Z

src/ImageSharp/Memory/ArrayPoolMemoryManager.cs

+
+            if (this.minSizeBytes > 0 && bufferSizeInBytes < this.minSizeBytes)
            {
-                return new Buffer<T>(new T[itemSize], itemSize);


I had this one down correctly before I changed from Marshal.SizeOf to Unsafe.SizeOf -- and then, for whatever reason, I messed it up entirely and didn't re-test it. Oh well.

rytmis · 2018-01-12T13:47:07Z

src/ImageSharp/Configuration.cs

+        /// <summary>
+        /// Gets or sets the <see cref="MemoryManager"/> that is currently in use.
+        /// </summary>
+        public MemoryManager MemoryManager { get; set; } = new ArrayPoolMemoryManager(1024 * 80);


This threshold here is rather large (80 kB). @antonfirsov was of the opinion, that the threshold for allocating from the pool should be half a kilobyte at the most, so we're going to want to change this, at least.

@rytmis ArrayPool is actually really useful in small-mid dynamic array allocation use cases, because it reduces GC pressure + improves data locality!
Check out it's usages in aspnet mvc.

An example: it's providing ArrayPool<char> to Newtonsoft.Json through it's own IArrayPool<T> abstraction.

I changed the default value to 512 bytes, but obviously you can decide what you want to use for it -- I don't know what benchmarks to run to find the optimal values, and so on, so I'll just defer to your judgment. 🙂

antonfirsov

Not yet a full review, tried to focus on the most important conceptual stuff for now. My main points:

We need to agree on the minSize / maxSize issues. I strongly believe that by tuning maxSizeInBytes for ArrayPoolMemoryManager, all users should be able to avoid OutOfMemoryException-s & optimize their application. You won't need NullMemoryManager for that, believe me! :) My biggest mistake was to set the maximum to a crazy value, now I regret it very much :( You are already using ArrayPool by using ASP.NET MVC! (and they have a sane maximum limit I suppose)
Buffer<T>.Array is a timebomb/minefield in it's current form! We should deal with it ASAP even if not in this PR. (Should be done before releasing our next beta.)
Avoiding the usage of MemoryManager.Current is absolutely necessary, but maybe we can leave it for now on less significant points of the code. I know, it's a lot of chore work, but I can join pushing to your branch, if you want!
We should provide test coverage for MemoryManager & ArrayPoolMemoryManager. I can do this.

@JimBobSquarePants @tocsoft thoughts?

antonfirsov · 2018-01-13T16:04:26Z

src/ImageSharp/Memory/ArrayPoolMemoryManager.cs

+    /// </summary>
+    public class ArrayPoolMemoryManager : MemoryManager
+    {
+        private readonly int minSizeBytes;


Not sure if we need minSizeBytes at all. The correct value should be found by benchmarking. I'd say it's an early optimization at this point, and would prefer to drop it for simplicity.

Also see my comment on Configuration.MemoryManager for an explanation about why is small-to-mid array pooling benefitical.

antonfirsov · 2018-01-13T16:16:47Z

src/ImageSharp/Memory/ArrayPoolMemoryManager.cs

+        {
+            this.minSizeBytes = minSizeBytes;
+
+            this.pool = ArrayPool<byte>.Create(CalculateMaxArrayLength(), 50);


The maximum pool size should be configurable from the outside! (maximumPoolSizeInBytes)

I pretty sure, that lowering the default maximum to more sane value alone would solve most of the Out Of Memory errors users have!

antonfirsov · 2018-01-13T16:19:05Z

src/ImageSharp/Memory/ArrayPoolMemoryManager.cs

+        /// Heuristically calculates a reasonable maxArrayLength value for the backing <see cref="ArrayPool{T}"/>.
+        /// </summary>
+        /// <returns>The maxArrayLength value</returns>
+        internal static int CalculateMaxArrayLength()


In our new design (backing everyting with byte[] arrays), this should be actually a constant and the default value for the maximumPoolSizeInBytes parameter. (See my previous comment.)

antonfirsov · 2018-01-13T16:23:04Z

src/ImageSharp/Memory/ArrayPoolMemoryManager.cs

+        /// <returns>The maxArrayLength value</returns>
+        internal static int CalculateMaxArrayLength()
+        {
+            const int MaximumExpectedImageSize = 16384 * 16384;


My biggest mistake was defining this value that high. It probably costs 100-s of €-s for Azure users :)

I guess a value like 4096 * 4096 would do for now. Should be determined by proper load testing benchmarks later.

antonfirsov · 2018-01-13T16:28:05Z

src/ImageSharp/Common/Extensions/StreamExtensions.cs

            {
-                byte[] foo = ArrayPool<byte>.Shared.Rent(count);
-                try
+                byte[] foo = new byte[count];


This is a typical case, where using ArrayPool is better than newing up an array! See my comment on Configuration.MemoryManager.

antonfirsov · 2018-01-13T16:52:16Z

src/ImageSharp/Image/PixelAccessor{TPixel}.cs

        /// <param name="height">The height of the image represented by the pixel buffer.</param>
        public PixelAccessor(int width, int height)
-            : this(width, height, Buffer2D<TPixel>.CreateClean(width, height), true)
+            : this(width, height, Configuration.Default.MemoryManager.Allocate2D<TPixel>(width, height, true), true)


Unfortunately we need to propagate that parameter here :(
It's possible to do this incrementally by defining a temporal construct with an optional memoryManager = null parameter and calling memoryManager ?? Configuration.Default.MemoryManager.Allocate2D<TPixel>(width, height, true).

We should get rid of PixelAccessor anyways, so leave it for now if you want

antonfirsov · 2018-01-13T16:54:40Z

src/ImageSharp/Image/PixelArea{TPixel}.cs

            this.Length = this.RowStride * height;

-            this.byteBuffer = MemoryManager.Current.Allocate<byte>(this.Length, true);
+            this.byteBuffer = Configuration.Default.MemoryManager.Allocate<byte>(this.Length, true);


If you feel the chore work is just too much, leave it then :)

We should get rid of both PixelAccessor<T> and PixelArea<T> anyways. They are legacy constructs.

antonfirsov · 2018-01-13T16:57:52Z

src/ImageSharp/PixelFormats/PixelBlenders/DefaultPixelBlenders.Generated.tt

                    Guard.MustBeGreaterThanOrEqualTo(amount.Length, destination.Length, nameof(amount.Length));

-                    using (Buffer<Vector4> buffer = MemoryManager.Current.Allocate<Vector4>(destination.Length * 3))
+                    using (Buffer<Vector4> buffer = Configuration.Default.MemoryManager.Allocate<Vector4>(destination.Length * 3))


Yeah this is actully forcing us to make API design decisions. I think it should be a parameter of Blend(). @tocsoft thoughts?

antonfirsov · 2018-01-13T17:04:43Z

src/ImageSharp/Formats/Jpeg/GolangPort/Components/Decoder/OrigHuffmanTree.cs

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public int GetMaxCode(int index)
+        {
+            fixed (int* maxCodesPtr = this.MaxCodes)


GetMaxCode() gonna pin/unpin this.MaxCodes several times inside a loop. We need to find a better way. Pinning outside the loop, or applying ref arithmetics with SRCS.Unsafe could do the job.

Do you want me to deal with this?

antonfirsov · 2018-01-13T17:09:44Z

src/ImageSharp/Memory/NullMemoryManager.cs

+    /// <summary>
+    /// Implements <see cref="MemoryManager"/> by allocating new buffers on every call.
+    /// </summary>
+    public class NullMemoryManager : MemoryManager


I think we need to find a better name for this :) NullMemoryManager suggests me, that it's always returning null instead of a memory block with a valid memory address.

@JimBobSquarePants @tocsoft suggestions?

SimpleMemoryManager maybe?

What about ManagedMemoryFactory and PooledMemoryFactory ?

vpenades · 2018-01-25T12:23:26Z

As far as I understand how this configurable memory manager works, it is designed to set a global memory manager before any other call, and then it uses that memory manager on everything; from requesting memory for image buffers to small streaming arrays.

My questions are:

What happens if you switch memory manager on the fly, when you've already allocated images, or worse, when there's some asynchronous background image processing running.
It is possible to do this?: To use a "Null" memory manager to hold Image's buffers, but to use ArrayPools locally bounded to the context of encoders/decoders and processors.

If given the choice, I would like to configure ImageSharp like this:

Images will always use GC managed memory, even for small images.
Encoders, Decoders and processos would use ArrayPools associated to their lifetime, for example, a JpegEncoder would create an internal ArrayPool to reuse arrays within the context of encoding a given image, but when it's done, the ArrayPool is GC'ed along with the encoder.

Example:

var image = new Image<Rgba32>(512,512); // memory held by standard GC'ed array
image.Mutate( dc => dc.Resize(256,256).Crop(48,48).GaussianBlur(2) ); // dc using local ArrayPools, GC'ed at the end of the mutation
image.Save("test.png"); // png encoder using ArrayPools internally, again, GC'ed at the end of encoding
// no need to dispose image since it's buffers are GC managed.

In this case, the image's internal buffer would be GC managed, but IImageProcessingContext would have a non static PooledMemoryManager used by all the processors, but GC'ed when the "mutation" is completed.

For me this is the ideal behaviour, because in my case it's a combination of performance, ease of use and guaranteeing when I've finished, all memory used by ImageSharp will be GC'ed

rytmis · 2018-01-25T12:41:09Z

@vpenades, the long-term idea as @tocsoft outlined it is to have "usage hints" at the call sites. This way, you could configure a single composite memory manager that would work the way you describe, since the usage hints for encoders would be different than those used for images.

antonfirsov · 2018-01-25T12:59:33Z

@vpenades

re: different memory usage scenarios

The hard part is the context-bound automatical release of pools after each operation, but maybe we can figure this out. It's very easy however to provide a safe API for manual release of these resources:
When you are finished with all the image manipulation work, you can just call MemoryManager.Default.ReleaseRetainedResources(). (It should not affect images/buffers being in use.)

re: MemoryManager consistence:

What happens if you switch memory manager on the fly, when you've already allocated images, or worse, when there's some asynchronous background image processing running.

My suggestion (could work with both Configuration.Default and user-managed Configuration-s:

public class Configuration
{
    ...
    
    public IMemoryManager MemoryManager 
    {
    	get { return this.memoryManager; } 
    	set 
    	{ 
    		if (this.memoryManager.IsTouched) throw new InvalidOperationException();
    		this.memoryManager = value;
    	}
    }
}

vpenades · 2018-01-25T15:08:53Z

@antonfirsov my concern with static configurations is that I'm working with a plugin based system (I've pointed you to it before) in which I can have more than one ImageSharp based plugin loaded at the same time. (In the scenarios I have in mind, there will be lots!)

So I have these conflicting scenarios when I have two independently developed plugins being loaded at the same time:

Both plugins trying to configure the memory manager independently: the second setup attempt crashes.
Both leaving the configuration setup to the host application. In this case, I have to make the host application "ImageSharp aware" which defeats the purpose of being plugin based.

This can also happen with third party libraries extending or wrapping imagesharp, and willing to use it with a specific configuration; whenever you try to use libraries from different sources, we can end having conflicting configurations.

The only way I can think to solve this is to break the APIs and create objects through non static factory instances, but I guess it's a change too big to be considered.

antonfirsov · 2018-01-25T15:19:18Z

@vpenades your scenario is exactly the reason why usin Configuration.Default is not mandatory at all! You can have as many instances of the Configuration object, thus MemoryManager as you want. Just make sure, you use the proper Image.Load(configuration, etc ...) overloads.
If it was only up to me to decide, I would maybe even drop the singleton, because I consider it an antipattern, but it would complicate the API, thus raise the barrier of entry for new users.

antonfirsov · 2018-01-25T15:25:04Z

Calling Configuration.Default.MemoryManager.Anything() was only demonstrational in my examples. You can operate on the Configuration/MemoryManager instance of your choice.

vpenades · 2018-01-25T15:35:20Z

@antonfirsov maybe the singleton can be dropped if we rename things: instead of calling it "Configuration" let's called "Factory", so the new API would look like this:

var image1 = PooledFactory.Default.Load("test.png");
var image2 = GCManagedFactory.Default.CreateImage<Rgba32>(512,512);

I think it's quite a huge change, but it's not that ugly. In fact, having the images created from factories would help future works when we require "interop" or file mapped images, which could be supported transparently.

antonfirsov · 2018-02-17T02:48:13Z

Hey @rytmis! Do you have interest/time to continue working on this?
If you can't make it, I'd prefer pulling your work into a separate branch, finish it there, and open a new PR in the end.

Gonna have time for this in the middle of the next week.

rytmis · 2018-02-17T06:12:37Z

Interest? Sure. Time? Not so sure. Skills? Probably not. :)

If you need someone to do boring grunt work, then I'm your guy, obviously -- but I don't think pulling this to a separate branch is necessarily a problem for that, so do whatever is easiest for you. I'm just glad to have helped at least a little bit.

antonfirsov · 2018-02-17T14:35:11Z

You already helped a lot, thanks!

I have some time, so I'll grab it from now then. In order to stick to my personal plans I need to finish it before the end of the next week. Probably gonna ask for some input.

Don't talk nonsense about your skills, you're doing cool stuff according to your blog and GH contribution history! 😄

antonfirsov · 2018-02-26T19:44:32Z

Closed this in favor of #475

rytmis added 6 commits January 11, 2018 19:31

- Make Buffer2D wrap Buffer

72772ff

- Allocate Buffers from memory manager

607e452

- Allocate Buffer2Ds from memory manager

c5eb2cf

- Add a minimum size threshold for array pool usage

a73283f

- Add a null memory manager that doesn't do any actual memory management

- Removed a test that doesn't actually test anything any more

9e4e5ab

- Removed PixelDataPool

b1a5c71

rytmis added 4 commits January 12, 2018 15:03

- Oops. Note to self: don't make changes to unsafe

333ce3b

code just before going to sleep

- Explicitly pass MemoryManager to the places that

f675f5c

need it (aside from a few exceptions)

- Use Configuration.Default.MemoryManager in tests

f1412d3

- Code style fixes

2451168

rytmis mentioned this pull request Jan 12, 2018

Implement pluggable memory management #225

Closed

15 tasks

rytmis added 3 commits January 12, 2018 19:07

- Removing more usages of ArrayPool

58330a1

- Remove ArrayPool from Huffman tree

58ab4e6

- Use fixed buffers in Huffman table

9290536

JimBobSquarePants reviewed Jan 13, 2018

View reviewed changes

rytmis commented Jan 13, 2018

View reviewed changes

rytmis added 5 commits January 13, 2018 14:32

- Use memory manager in Bytes

2615556

- Removed ArrayPool from WuQuantizer

4e4cdd5

- Optional param -> second method

cf13ebf

- Whitespace fix

76dc5c2

- Reduced the threshold for ArrayPoolMemoryManager to 512 bytes

54313e8

antonfirsov requested changes Jan 13, 2018

View reviewed changes

rytmis and others added 4 commits January 13, 2018 22:04

- Oops, neglected to fix this test

b23b137

- Removed Configuration.Default.MemoryManager from PixelAccessor

750af5a

dropping minSizeBytes + fixing tests

7e3cb28

maxPoolSizeInBytes parameter + safer indexer for Buffer<T>

22206f1

antonfirsov mentioned this pull request Jan 15, 2018

PixelDataPool<T>: reduce maximum pooled array size #436

Merged

4 tasks

JimBobSquarePants mentioned this pull request Feb 9, 2018

OutOfMemoryException when loading png image #248

Closed

antonfirsov mentioned this pull request Feb 25, 2018

Configurable & optimized memory management #475

Merged

4 tasks

antonfirsov closed this Feb 26, 2018

Uh oh!

WIP: swappable memory pools #431

WIP: swappable memory pools #431

Uh oh!

Conversation

rytmis commented Jan 11, 2018

Prerequisites

Description

Uh oh!

CLAassistant commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov commented Jan 12, 2018

Uh oh!

JimBobSquarePants commented Jan 12, 2018

Uh oh!

codecov bot commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpenades commented Jan 25, 2018

CLAassistant commented Jan 11, 2018 •

edited

Loading

codecov bot commented Jan 12, 2018 •

edited

Loading

antonfirsov Jan 13, 2018 •

edited

Loading

antonfirsov commented Jan 25, 2018 •

edited

Loading

vpenades commented Jan 25, 2018 •

edited

Loading