Configurable & optimized memory management #475

antonfirsov · 2018-02-25T20:44: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)

PR summary

Finished the changes started by @rytmis on WIP: swappable memory pools #431
Enables configurable memory management as described in Implement pluggable memory management #225
With the default pooling settings, ImageSharp memory usage is approximately halved.

Changes

Implemented all the beta-3 points of Implement pluggable memory management #225
- There is no MemoryManager.Return() in the final design, because the buffer implementations are now bound to their MemoryManager-s.
A workaround for coreclr/#16443, hope it will fix travis builds
A few regression tests for drawing methods. (Needed it to allow changes to be made safely.)

Benchmarks

Load testing benchmarks have been implemented on the load-test branch. (The project should be moved into it's own SixLabors repository later). Link to results

TLDR

This is how (managed/GC) memory consumption changes for 10,000 resize requests of ~3200p Jpegs, new defaults VS pre-MemoryManager (old):

The peaks are measured around requests with big outlier images.

Throughput

The throughput visualized for different scenarios & pool settings (measured in MegaPixel/sec):

Memory footprint

Avarage managed memory footprint while serving ~600 resize requests (different scenarios & pool settings):

Conclusions

With the default pooling settings, ImageSharp memory usage is approximately halved. I hope these settings are now good enough for most users.
If your environment is memory-constrained:
- Use ArrayPoolMemoryManager.CreateWithModeratePooling() or .CreateWithMinimalPooling()! For these two: the difference in memory consumption is not as big as the difference in throughput.
- Do not use SimpleGCMemoryManager ("NullMemoryManager"), it only makes things worse!
For servers under high load ArrayPoolMemoryManager.CreateWithAggressivePooling() can increase the throughput with the cost of a higher memory footprint
The memory usage is always being stabilized after a warm-up period.
The throughput of the pre-MemoryManager ("Old") version is significantly higher. Not sure if the performance regression is caused directly by changes in memory management. We need to investigate this.

/cc @vpenades @GeorgePlotnikov @denisivan0v

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

code just before going to sleep

need it (aside from a few exceptions)

# Conflicts: # src/ImageSharp/Formats/Gif/GifDecoderCore.cs # src/ImageSharp/Formats/Png/PngEncoderCore.cs # src/ImageSharp/Image/ImageFrame{TPixel}.cs # src/ImageSharp/Memory/Buffer{T}.cs # src/ImageSharp/Memory/PixelDataPool{T}.cs # src/ImageSharp/Processing/Processors/Filters/LomographProcessor.cs # src/ImageSharp/Processing/Processors/Filters/PolaroidProcessor.cs # src/ImageSharp/Processing/Processors/Transforms/ResamplingWeightedProcessor.Weights.cs # src/ImageSharp/Processing/Processors/Transforms/ResizeProcessor.cs # src/ImageSharp/Processing/Processors/Transforms/RotateProcessor.cs # src/ImageSharp/Processing/Processors/Transforms/SkewProcessor.cs # tests/ImageSharp.Tests/Memory/PixelDataPoolTests.cs # tests/ImageSharp.Tests/Processing/Processors/Transforms/ResizeProfilingBenchmarks.cs

…to feature/memory-manager # Conflicts: # tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs

PngEncoder, WuQuantizer, ShapeRegion, ShapePath

antonfirsov · 2018-02-25T23:55:29Z

Run some resizes, like with the current stress tests.
Stop using ImageSharp, and run a full GC collection, one that also tries to free any large object.
Test result: how much memory was actually freed, and how much is still locked by the array pools?

@vpenades as you wish!
On this chart you can see the effect of calling MemoryManager.ReleaseRetainedResources() + GC.Collect() after ~200 requests:

So, to use an alternative memory manager, there's two choices: [...]

Yes, you always need to explicitly pass a memory manager to individual operations. I suggest you to implement an ImagingContext class which owns a ~~MemoryManager~~ Configuraition+MemoryManager for you and encapsulates your commonly used Image factory methods & maybe some operations. As @JimBobSquarePants explained, we did not want such a thing to be part of our core API, because it complicates things, and hardens the learning curve of the library, while not needed by the 90% of our users.

vpenades · 2018-02-26T11:14:03Z

@antonfirsov Upon a second review, I've realized the Configuration class essentially handles memory managers and image formats, but Image Formats are handled directly by the configuration class.

Maybe it could be cleaner to have a image formats decoupled from Configuration into it's own class ImageFormatsCollection that would keeps all the functionality related to image formats management, and would let Configuration to be greatly simplified.

That would make life easier when creating multiple configurations while keeping a shared collection of image formats. with the current configuration other than the default, you need to manually scan for all the formats in the default and copy them into your own configuration. If at some point during execution a new format is added to the default configuration, the manually created configurations will not be aware of the change.

Something like this:

class Configuration
{
    public ParallelOptions ParallelOptions {get;set;
    public MemoryManager Manager {get; set;}
    public FormatsCollection Formats {get; set;}
}

This would allow having manually created configurations with their own formats, OR sharing the formats with the default configuration.

JimBobSquarePants

One hell of a job! 🎉

Only thing blocking a merge that I can see is the naming of GetReferenceToOrigo. Despite it being internal, I'd rather fix it before so we don't forget.

JimBobSquarePants · 2018-02-26T01:40:40Z

src/ImageSharp.Drawing/Paths/ShapeRegion.cs

-                    buffer[i + offset] = array[i].X;
-                }
+            // TODO: This is a temporal workaround because of the lack of Span<T> API-s on IPath. We should use MemoryManager.Allocate() here!
+            PointF[] innerBuffer = new PointF[buffer.Length];


Let's get a PR in place against Shapes to add those API's.

Should we do it now, or in a follow up PR pair?

Let's do a follow up once we've done Shapes.

JimBobSquarePants · 2018-02-26T01:42:00Z

src/ImageSharp.Drawing/Brushes/SolidBrush{TPixel}.cs

                    }
                }
                catch (Exception)
                {
+                    // TODO: Why are we catching exceptions here silently ???


This was probably a hangover from the old try..catch..finally pattern from when we were directly using ArrayPool

JimBobSquarePants · 2018-02-26T05:45:54Z

src/ImageSharp/Formats/Jpeg/PdfJsPort/Components/PdfJsHuffmanTable.cs

-            this.valOffset = Buffer<short>.CreateClean(18);
-            this.maxcode = Buffer<long>.CreateClean(18);
+            // TODO: Replace FakeBuffer<T> usages with standard or array orfixed-sized arrays
+            this.lookahead = memoryManager.AllocateFake<short>(256);


Are we leaving this TODO for when we PR Shapes?

Yeah, that would allow us to get rid of the AllocateFake<T> workaround entirely.

JimBobSquarePants · 2018-02-26T05:52:16Z

src/ImageSharp.Drawing/Brushes/SolidBrush{TPixel}.cs

-                    this.Colors[i] = color;
-                }
+                this.Colors = source.MemoryManager.Allocate<TPixel>(source.Width);
+                this.Colors.Span.Fill(color);


Didn't know Span had a Fill method. Neat!

JimBobSquarePants · 2018-02-26T05:54:37Z

src/ImageSharp/Advanced/AdvancedImageExtensions.cs

@@ -88,6 +88,14 @@ internal static Span<TPixel> GetPixelRowSpan<TPixel>(this Image<TPixel> source,
            where TPixel : struct, IPixel<TPixel>
            => source.Frames.RootFrame.GetPixelRowSpan(row);

+        /// <summary>


When shall we make this and others public?

We need Memory<T> for the memory interop stuff, for the rest we need a proper API review. Not sure if stuff like .GetMemoryManager() should be public.

Cool, I'm looking forward to MS getting that released.

JimBobSquarePants · 2018-02-26T11:16:13Z

src/ImageSharp/Memory/BufferArea{T}.cs

+        /// Gets a value indicating whether the area refers to the entire <see cref="DestinationBuffer"/>
+        /// </summary>
+        public bool IsFullBufferArea => this.Size == this.DestinationBuffer.Size();
+
        /// <summary>
        /// Gets or sets a value at the given index.
        /// </summary>


Diff wouldn't let me highlight where I wanted to

GetReferenceToOrigo I'm assuming this is a typo? GetReferenceToOrigin perhaps?

It's just my Hunglish :D

😄 Haha... Far better than any language skills I've accumulated.

JimBobSquarePants · 2018-02-26T11:22:36Z

src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs

@@ -221,9 +224,9 @@ private void ReadRle8<TPixel>(PixelAccessor<TPixel> pixels, byte[] colors, int w
            var color = default(TPixel);
            var rgba = new Rgba32(0, 0, 0, 255);

-            using (var buffer = Buffer2D<byte>.CreateClean(width, height))
+            using (var buffer = this.configuration.MemoryManager.Allocate2D<byte>(width, height, true))


AllocateClean2D?

JimBobSquarePants · 2018-02-26T11:42:30Z

Maybe it could be cleaner to have a image formats decoupled from Configurationinto it's own class ImageFormatsCollection that would keeps all the functionality related to image formats management, and would let Configuration to be greatly simplified.

@vpenades Looking at Configuration class I'm inclined to agree. A separate PR though I think.

@antonfirsov Re throughput performance? Perhaps it's as simple as the abstraction costs due to using interfaces? I can't see anything that stands out.

antonfirsov · 2018-02-26T22:38:47Z

@JimBobSquarePants a quick initial investigation regarding the regression in speed:
Looks like there are several factors behind that, scattered through the whole codebase.
The most significant is probably Span<T> indexer, and the use of IBuffer<T>.Span (accessing elements in a lazy way: buffer.Span[i]). Span is being used now on many places where we've been using locally pooled arrays earlier, it has a high impact therefore.

I've been running my benchmarks on .NET 4.6.1 runtime (was simpler to work with command line), where indexing Span<T> is significantly slower than indexing arrays. The slowdown of the ReadByteStuffedByteUnsafe() is a quite trivial example in the Jpeg decoder:

Like it or not, but if we want the library to be fast on the classic .NET, we have to replace indexers with Unsafe.Add(ref base, i) on hot path.

I will continue my investigation and maybe apply fixes on the weakest points.

…parallel buffer reusal trick

JimBobSquarePants · 2018-02-26T23:43:10Z

@antonfirsov I'm happy for this to be merged now. 👍

Unsafe<T> is fantastically readable really (It's such a great API!) so I have no issue replacing indexer access on hot paths with that. That can be done as small individual PR's though as we benchmark after the beta. (I'm really hoping the final push will be all about optimization with very few bug fixes/changes)

antonfirsov · 2018-02-26T23:45:19Z

@JimBobSquarePants check out my latest optimization commits. Pushed stuff that would normally deserve a separate explanatory PR.

Maybe I'll add a few more of these (only for stuff that's covered!), and we can merge afterwards.

JimBobSquarePants · 2018-02-26T23:59:38Z

WithTemporalBuffer<T> is very nice! I really should get better at reading up on the API's I use, I wasn't aware of the before/after action overloads to Parallel.For

Ok, rock on, I'll leave merging to you.

… pinning from OrigHuffmanTree (cherry picked from commit 81f6a94)

…ngs worse! (cherry picked from commit 0bf64a09598ba988318c170259a7aab4d9391cb5)

antonfirsov · 2018-02-27T02:31:40Z

Okkay, I guess I managed to bring back speed to previous levels with just a few changes, so no one would feel there's a regression.

JimBobSquarePants · 2018-02-27T02:51:14Z

That's a neat trick you used to access the fixed Data field in the unfixed indexer expression in FixedInt32Buffer16 and co. I'll have to remember that one.

JimBobSquarePants · 2018-02-27T02:56:53Z

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

@@ -58,10 +58,12 @@ internal struct Bytes : IDisposable
        /// <returns>The bytes created</returns>
        public static Bytes Create(MemoryManager memoryManager)


No need to pass the parameter here now or to the InputProcessor.

Thanks for fixing it!

antonfirsov · 2018-02-27T15:27:15Z

@JimBobSquarePants
Re Parallel.For parameters:
I also discovered them yesterday! I was googling for that feature (I was quite sure it must exist), and it was right here on the Parallel.For, API we just failed to notice it :)

Re indexable fixed structs:
It's quite an unsafe trick, so one should be very careful using it. Strictly speaking, it should be named something like GetItemUnsafe(idx) instead of simply being an indexer, but that naming is soooo verbose.

…/ImageSharp into feature/memory-manager

vpenades · 2018-02-27T15:57:33Z

@vpenades Looking at Configuration class I'm inclined to agree. A separate PR though I think.

@JimBobSquarePants Yes, please, this PR is a bit useless if creating manual configurations is non trivial.

antonfirsov · 2018-02-27T16:59:31Z

I'm merging this now, further optimizations and API changes will be added as separate PR-s.

Configurable & optimized memory management

rytmis and others added 30 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

- 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

- Removing more usages of ArrayPool

58330a1

- Remove ArrayPool from Huffman tree

58ab4e6

- Use fixed buffers in Huffman table

9290536

- 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

- 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

fix build after merge

af23153

moving common MemoryManager logic into extension methods

7a076de

Merge remote-tracking branch 'origin/antonfirsov/cover-all-codecs' in…

967098b

…to feature/memory-manager # Conflicts: # tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs

dropping MemoryManager ctr. argument:

3819c75

PngEncoder, WuQuantizer, ShapeRegion, ShapePath

introducing FakeBuffer<T> workaround

2ea9e08

IManagedByteBuffer

29483b3

hide Buffer<T>.Array, use IManagedByteBuffer when necessary

cf96e61

SimpleManagedMemoryManager -> SimpleGcMemoryManager

48a3189

JimBobSquarePants requested changes Feb 26, 2018

View reviewed changes

antonfirsov mentioned this pull request Feb 26, 2018

WIP: swappable memory pools #431

Closed

4 tasks

antonfirsov added 2 commits February 26, 2018 21:04

review cleanup

c07cbea

added comments for the WeakReference stuff

4d915b6

JimBobSquarePants approved these changes Feb 26, 2018

View reviewed changes

antonfirsov added 2 commits February 27, 2018 00:36

optimize ResizeProcessor parallel behavior and Span<T> usage

de67364

introducing ParallelFor.WithTemporalBuffer(): common utility for the …

d721138

…parallel buffer reusal trick

antonfirsov added 2 commits February 27, 2018 03:00

removed Span bottleneck from Block8x8F.CopyTo() + removed unnecessary…

1b0a2b4

… pinning from OrigHuffmanTree (cherry picked from commit 81f6a94)

Removing all the buffer magic from the Bytes struct. It only made thi…

7d5cea1

…ngs worse! (cherry picked from commit 0bf64a09598ba988318c170259a7aab4d9391cb5)

JimBobSquarePants reviewed Feb 27, 2018

View reviewed changes

Remove unneeded parameter

c314db6

antonfirsov added 2 commits February 27, 2018 16:44

Temporary Vortex

5b9b27c

Merge branch 'feature/memory-manager' of https://github.com/SixLabors…

55d2184

…/ImageSharp into feature/memory-manager

antonfirsov merged commit 47a8533 into master Feb 27, 2018

denisivan0v mentioned this pull request Apr 18, 2018

Disposing data chunk instead of explicit returning it to ArrayPool in PngDecoderCore.Identify #534

Merged

4 tasks

JimBobSquarePants deleted the feature/memory-manager branch September 3, 2019 11:12

antonfirsov added a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#475 from SixLabors/feature/memory-manager

394bfae

Configurable & optimized memory management

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable & optimized memory management #475

Configurable & optimized memory management #475

antonfirsov commented Feb 25, 2018 •

edited

Loading

antonfirsov commented Feb 25, 2018 •

edited

Loading

vpenades commented Feb 26, 2018 •

edited

Loading

JimBobSquarePants left a comment

JimBobSquarePants Feb 26, 2018

antonfirsov Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants Feb 26, 2018

antonfirsov Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants Feb 26, 2018

antonfirsov Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants Feb 26, 2018

antonfirsov Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants Feb 26, 2018

JimBobSquarePants commented Feb 26, 2018

antonfirsov commented Feb 26, 2018

JimBobSquarePants commented Feb 26, 2018

antonfirsov commented Feb 26, 2018

JimBobSquarePants commented Feb 26, 2018

antonfirsov commented Feb 27, 2018

JimBobSquarePants commented Feb 27, 2018 •

edited

Loading

JimBobSquarePants Feb 27, 2018

antonfirsov Feb 27, 2018

antonfirsov commented Feb 27, 2018 •

edited

Loading

vpenades commented Feb 27, 2018

antonfirsov commented Feb 27, 2018

		@@ -58,10 +58,12 @@ internal struct Bytes : IDisposable
		/// <returns>The bytes created</returns>
		public static Bytes Create(MemoryManager memoryManager)

Configurable & optimized memory management #475

Configurable & optimized memory management #475

Conversation

antonfirsov commented Feb 25, 2018 • edited Loading

Prerequisites

PR summary

Changes

Benchmarks

TLDR

Throughput

Memory footprint

Conclusions

antonfirsov commented Feb 25, 2018 • edited Loading

vpenades commented Feb 26, 2018 • edited Loading

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants commented Feb 26, 2018

antonfirsov commented Feb 26, 2018

JimBobSquarePants commented Feb 26, 2018

antonfirsov commented Feb 26, 2018

JimBobSquarePants commented Feb 26, 2018

antonfirsov commented Feb 27, 2018

JimBobSquarePants commented Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Feb 27, 2018 • edited Loading

vpenades commented Feb 27, 2018

antonfirsov commented Feb 27, 2018

antonfirsov commented Feb 25, 2018 •

edited

Loading

antonfirsov commented Feb 25, 2018 •

edited

Loading

vpenades commented Feb 26, 2018 •

edited

Loading

JimBobSquarePants commented Feb 27, 2018 •

edited

Loading

antonfirsov commented Feb 27, 2018 •

edited

Loading