Reduced intermediate allocations #2415

gfoidl · 2023-03-25T21:41:19Z

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 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

intermediate allocations are reduced -- mostly by using stackallocated memory
scratch buffers, that were fields of a class, are either removed or substituted by fixed sized buffers, thus reducing the amount of bytes allocated a bit (no additional object headers for the array), and reducing the total number of object the GC needs to track

Tests are run locally also with only 256 kB of stack-size (like (older) IIS) has -- via DOTNET_DefaultStackSize=0x3e800.

gfoidl

Some notes for review.

gfoidl · 2023-03-25T21:42:02Z

src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs

@@ -1470,7 +1474,7 @@ private int ReadImageHeaders(BufferedReadStream stream, out bool inverted, out b
        {
            // Usually the color palette is 1024 byte (256 colors * 4), but the documentation does not mention a size limit.
            // Make sure, that we will not read pass the bitmap offset (starting position of image data).
-            if ((stream.Position + colorMapSizeBytes) > this.fileHeader.Offset)
+            if (stream.Position > this.fileHeader.Offset - colorMapSizeBytes)


This is just because I'm paranoid of overflow 😉.

gfoidl · 2023-03-25T21:43:22Z

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

@@ -22,7 +22,7 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
    /// <summary>
    /// The temp buffer used to reduce allocations.
    /// </summary>
-    private readonly byte[] buffer = new byte[16];
+    private ScratchBuffer buffer;   // mutable struct, don't make readonly


This ScratchBuffer also has 16 bytes and is embedded into this class. It saves the additional object the GC needs to track + the overhead of object / array.

I think we use scratch buffers in a few places.

gfoidl · 2023-03-25T21:46:24Z

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

+    private unsafe struct ScratchBuffer
+    {
+        private fixed byte scratch[16];
+
+        public Span<byte> Span => MemoryMarshal.CreateSpan(ref this.scratch[0], 16);
+    }


Here you can see what the C# compiler generates.

ref this.scratch[0] gets translated to ref scratch.FixedElementField so a normal reference to a field.

gfoidl · 2023-03-25T21:47:04Z

src/ImageSharp/Formats/Gif/GifEncoderCore.cs

+            buffer[1] = GifConstants.CommentLabel;
+            buffer[0] = GifConstants.ExtensionIntroducer;


I'll write below a comment on why these are flipped.

gfoidl · 2023-03-25T21:49:38Z

src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs

+
+            this.outputStream.Write(buffer, 0, 4);
+
+            // We write the highest index first, to have only one bound check.


This sharplab showcases the difference between "normal" (0-13) or the order used here.

Note the bound check before every access in Foo, and only one bound check in Bar.

Yeah, there's a few places we've done that. 👍

gfoidl · 2023-03-25T21:52:39Z

src/ImageSharp/Formats/Tiff/PhotometricInterpretation/RgbFloat323232TiffColor{TPixel}.cs

@@ -61,16 +61,13 @@ public override void Decode(ReadOnlySpan<byte> data, Buffer2D<TPixel> pixels, in
            {
                for (int x = 0; x < pixelRow.Length; x++)
                {
-                    data.Slice(offset, 4).CopyTo(buffer);
-                    float r = BitConverter.ToSingle(buffer, 0);
+                    float r = BitConverter.ToSingle(data.Slice(offset, 4));


The intermediate copy into the buffer can be avoided here too.

Oh gosh yeah! That was a bad one.

gfoidl · 2023-03-25T21:54:05Z

src/ImageSharp/Formats/Webp/BitWriter/BitWriterBase.cs

@@ -23,7 +24,7 @@ internal abstract class BitWriterBase
    /// <summary>
    /// A scratch buffer to reduce allocations.
    /// </summary>
-    private readonly byte[] scratchBuffer = new byte[4];
+    private ScratchBuffer scratchBuffer;  // mutable struct, don't make readonly


I tried hard to avoid these fields, and use solely stackalloc.
But sometimes it's very hard, and passing the buffer into every method, potentially across types, is cumbersome. Thus these ScratchBuffer-types are used.

gfoidl · 2023-03-25T21:55:40Z

src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs

-            huffTree[i] = default;
-        }
+        Span<ushort> histogramSymbols = histogramImageXySize <= 64 ? stackalloc ushort[histogramImageXySize] : new ushort[histogramImageXySize];
+        Span<HuffmanTree> huffTree = stackalloc HuffmanTree[3 * WebpConstants.CodeLengthCodes];


This is a rather big stack allocation, but it should be fine.
That's why the unit-tests were run with only 256 kB stack size locally.

gfoidl · 2023-03-25T21:57:42Z

src/ImageSharp/Formats/Webp/Lossless/WebpLosslessDecoder.cs

-        {
-            codeLengths[i] = 0;
-        }
+        codeLengths.AsSpan(0, alphabetSize).Clear();


Span<T>.Clear is a method call, so I wasn't sure if this is a win over the manual (inline) loop.
Thus a benchmark run with .NET 6 yields:

| Method | AlphabetSize | Mean | Error | StdDev | Ratio | RatioSD | |----------- |------------- |----------:|---------:|---------:|------:|--------:| | Default | 40 | 21.36 ns | 0.457 ns | 0.764 ns | 1.00 | 0.00 | | Span_Clear | 40 | 15.73 ns | 0.338 ns | 0.474 ns | 0.73 | 0.03 | | | | | | | | | | Default | 256 | 118.73 ns | 2.409 ns | 4.642 ns | 1.00 | 0.00 | | Span_Clear | 256 | 26.19 ns | 0.532 ns | 0.711 ns | 0.22 | 0.01 |

Benchmark code

using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Running; BenchmarkRunner.Run<Bench>(); public class Bench { private readonly int[] _codeLengths = new int[296]; [Params(40, 256)] public int AlphabetSize { get; set; } [Benchmark(Baseline = true)] public void Default() { int alphabetSize = this.AlphabetSize; int[] codeLengths = _codeLengths; for (int i = 0; i < alphabetSize; ++i) { codeLengths[i] = 0; } } [Benchmark] public void Span_Clear() { _codeLengths.AsSpan(0, this.AlphabetSize).Clear(); } }

gfoidl · 2023-03-25T21:59:39Z

src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs

-    protected ulong ReadUInt64() =>
-        this.TryReadSpan(this.buf8)
-            ? this.ConvertToUInt64(this.buf8)
+    protected ulong ReadUInt64()


This method won't inline, so no problem to have the stackalloc here.
In recent JITs sometimes stackalloc won't hinder inlining anyway.

gfoidl · 2023-03-26T09:40:43Z

Please wait a moment with review -- I'd like to validate / improve something.

It's only one value for the fixed buffer size and the creation of the span -- so less error prone once the value needs to be updated.

gfoidl · 2023-03-26T11:50:41Z

to validate

Everything is fine. My fear was that the scratch-buffer, backed by the fixed buffer, e.g.

ImageSharp/src/ImageSharp/Formats/Gif/GifDecoderCore.cs

Line 25 in 7b2923d

private ScratchBuffer buffer; // mutable struct, don't make readonly

gets passed into a method directly. As it's a struct, so copied by value, there could be a perf-penalty due excessive copying. This example showcases what I mean. Method TestA passed the _buffer as value, so copying needs to be done (in the dasm the first call is the call to "memory copy" native routine). If the buffer needs to passed around, then it should be done via ref as shown in TestB.

Both don't apply here (also checked other uses of fixed buffers across the code-base).
Here we only pass the span around (as in TestC), so no problem.

JimBobSquarePants

A lot of learnings here.

It's really great to see in many instances a modernization of the codebase.

gfoidl added 8 commits March 25, 2023 22:35

Reduced intermediate allocations: Tiff

b099bda

Reduced intermediate allocations: Webp

d9e8d79

Reduced intermediate allocations: Png

31424c0

Reduced intermediate allocations: Jpeg

858a848

Reduced intermediate allocations: Gif

1d3ae0e

Reduced intermediate allocations: Tga

3c3479e

Reduced intermediate allocations: Bmp

b9b6f72

Reduced intermediate allocations: Profiles

5d65ef0

gfoidl commented Mar 25, 2023

View reviewed changes

Fixed build warnings / errors

57d0793

Use constant to specify the size of the buffer

7b2923d

It's only one value for the fixed buffer size and the creation of the span -- so less error prone once the value needs to be updated.

Merge branch 'main' into allocations

1e96db9

JimBobSquarePants added this to the v3.1.0 milestone Mar 27, 2023

Merge branch 'main' into allocations

53fa6da

JimBobSquarePants approved these changes Mar 29, 2023

View reviewed changes

JimBobSquarePants merged commit 3d40c91 into SixLabors:main Mar 29, 2023

gfoidl deleted the allocations branch March 29, 2023 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduced intermediate allocations #2415

Reduced intermediate allocations #2415

gfoidl commented Mar 25, 2023

gfoidl left a comment

gfoidl Mar 25, 2023

gfoidl Mar 25, 2023

JimBobSquarePants Mar 28, 2023

gfoidl Mar 25, 2023

gfoidl Mar 25, 2023

gfoidl Mar 25, 2023

JimBobSquarePants Mar 28, 2023

gfoidl Mar 25, 2023

JimBobSquarePants Mar 28, 2023

gfoidl Mar 25, 2023

gfoidl Mar 25, 2023

gfoidl Mar 25, 2023

gfoidl Mar 25, 2023

gfoidl commented Mar 26, 2023

gfoidl commented Mar 26, 2023

JimBobSquarePants left a comment

		buffer[1] = GifConstants.CommentLabel;
		buffer[0] = GifConstants.ExtensionIntroducer;


		this.outputStream.Write(buffer, 0, 4);

		// We write the highest index first, to have only one bound check.

Reduced intermediate allocations #2415

Reduced intermediate allocations #2415

Conversation

gfoidl commented Mar 25, 2023

Prerequisites

Description

gfoidl 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

gfoidl commented Mar 26, 2023

gfoidl commented Mar 26, 2023

JimBobSquarePants left a comment

Choose a reason for hiding this comment