Merge pull request #3113 from SixLabors/js/parallel-cleanup

Replace parallel row iteration with sequential loops in Crop/Convolution Processors

Author: JimBobSquarePants

src/ImageSharp/Advanced/ParallelExecutionSettings.cs

@@ -20,7 +20,7 @@ public readonly struct ParallelExecutionSettings
     /// </summary>
     /// <param name="maxDegreeOfParallelism">
     /// The value used for initializing <see cref="ParallelOptions.MaxDegreeOfParallelism"/> when using TPL.
-    /// Set to <c>-1</c> to leave the degree of parallelism unbounded.
+    /// If set to <c>-1</c>, there is no limit on the number of concurrently running operations.
     /// </param>
     /// <param name="minimumPixelsProcessedPerTask">The value for <see cref="MinimumPixelsProcessedPerTask"/>.</param>
     /// <param name="memoryAllocator">The <see cref="MemoryAllocator"/>.</param>
@@ -31,7 +31,7 @@ public ParallelExecutionSettings(
     {
         // Shall be compatible with ParallelOptions.MaxDegreeOfParallelism:
         // https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.paralleloptions.maxdegreeofparallelism
-        if (maxDegreeOfParallelism == 0 || maxDegreeOfParallelism < -1)
+        if (maxDegreeOfParallelism is 0 or < -1)
         {
             throw new ArgumentOutOfRangeException(nameof(maxDegreeOfParallelism));
         }
@@ -49,7 +49,7 @@ public ParallelExecutionSettings(
     /// </summary>
     /// <param name="maxDegreeOfParallelism">
     /// The value used for initializing <see cref="ParallelOptions.MaxDegreeOfParallelism"/> when using TPL.
-    /// Set to <c>-1</c> to leave the degree of parallelism unbounded.
+    /// If set to <c>-1</c>, there is no limit on the number of concurrently running operations.
     /// </param>
     /// <param name="memoryAllocator">The <see cref="MemoryAllocator"/>.</param>
     public ParallelExecutionSettings(int maxDegreeOfParallelism, MemoryAllocator memoryAllocator)
@@ -64,7 +64,7 @@ public ParallelExecutionSettings(int maxDegreeOfParallelism, MemoryAllocator mem
 
     /// <summary>
     /// Gets the value used for initializing <see cref="ParallelOptions.MaxDegreeOfParallelism"/> when using TPL.
-    /// A value of <c>-1</c> leaves the degree of parallelism unbounded.
+    /// A value of <c>-1</c> means there is no limit on the number of concurrently running operations.
     /// </summary>
     public int MaxDegreeOfParallelism { get; }
 
@@ -93,12 +93,10 @@ public ParallelExecutionSettings MultiplyMinimumPixelsPerTask(int multiplier)
     }
 
     /// <summary>
-    /// Get the default <see cref="SixLabors.ImageSharp.Advanced.ParallelExecutionSettings"/> for a <see cref="SixLabors.ImageSharp.Configuration"/>
+    /// Get the default <see cref="ParallelExecutionSettings"/> for a <see cref="Configuration"/>
     /// </summary>
     /// <param name="configuration">The <see cref="Configuration"/>.</param>
     /// <returns>The <see cref="ParallelExecutionSettings"/>.</returns>
     public static ParallelExecutionSettings FromConfiguration(Configuration configuration)
-    {
-        return new ParallelExecutionSettings(configuration.MaxDegreeOfParallelism, configuration.MemoryAllocator);
-    }
+        => new(configuration.MaxDegreeOfParallelism, configuration.MemoryAllocator);
 }

src/ImageSharp/Advanced/ParallelRowIterator.cs

@@ -44,7 +44,6 @@ public static void IterateRows<T>(
         where T : struct, IRowOperation
     {
         ValidateRectangle(rectangle);
-        ValidateSettings(parallelSettings);
 
         int top = rectangle.Top;
         int bottom = rectangle.Bottom;
@@ -109,7 +108,6 @@ public static void IterateRows<T, TBuffer>(
         where TBuffer : unmanaged
     {
         ValidateRectangle(rectangle);
-        ValidateSettings(parallelSettings);
 
         int top = rectangle.Top;
         int bottom = rectangle.Bottom;
@@ -174,7 +172,6 @@ public static void IterateRowIntervals<T>(
         where T : struct, IRowIntervalOperation
     {
         ValidateRectangle(rectangle);
-        ValidateSettings(parallelSettings);
 
         int top = rectangle.Top;
         int bottom = rectangle.Bottom;
@@ -236,7 +233,6 @@ public static void IterateRowIntervals<T, TBuffer>(
         where TBuffer : unmanaged
     {
         ValidateRectangle(rectangle);
-        ValidateSettings(parallelSettings);
 
         int top = rectangle.Top;
         int bottom = rectangle.Bottom;
@@ -315,35 +311,4 @@ private static void ValidateRectangle(Rectangle rectangle)
             0,
             $"{nameof(rectangle)}.{nameof(rectangle.Height)}");
     }
-
-    /// <summary>
-    /// Validates the supplied <see cref="ParallelExecutionSettings"/>.
-    /// </summary>
-    /// <param name="parallelSettings">The execution settings.</param>
-    /// <exception cref="ArgumentOutOfRangeException">
-    /// Thrown when <see cref="ParallelExecutionSettings.MaxDegreeOfParallelism"/> or
-    /// <see cref="ParallelExecutionSettings.MinimumPixelsProcessedPerTask"/> is invalid.
-    /// </exception>
-    /// <exception cref="ArgumentNullException">
-    /// Thrown when <see cref="ParallelExecutionSettings.MemoryAllocator"/> is null.
-    /// This also guards the public <see cref="ParallelExecutionSettings"/> default value, which bypasses constructor validation.
-    /// </exception>
-    private static void ValidateSettings(in ParallelExecutionSettings parallelSettings)
-    {
-        // ParallelExecutionSettings is a public struct, so callers can pass default and bypass constructor validation.
-        if (parallelSettings.MaxDegreeOfParallelism is 0 or < -1)
-        {
-            throw new ArgumentOutOfRangeException(
-                $"{nameof(parallelSettings)}.{nameof(ParallelExecutionSettings.MaxDegreeOfParallelism)}");
-        }
-
-        Guard.MustBeGreaterThan(
-            parallelSettings.MinimumPixelsProcessedPerTask,
-            0,
-            $"{nameof(parallelSettings)}.{nameof(ParallelExecutionSettings.MinimumPixelsProcessedPerTask)}");
-
-        Guard.NotNull(
-            parallelSettings.MemoryAllocator,
-            $"{nameof(parallelSettings)}.{nameof(ParallelExecutionSettings.MemoryAllocator)}");
-    }
 }

src/ImageSharp/Configuration.cs

@@ -65,8 +65,9 @@ public Configuration(params IImageFormatConfigurationModule[] configurationModul
     /// <summary>
     /// Gets or sets the maximum number of concurrent tasks enabled in ImageSharp algorithms
     /// configured with this <see cref="Configuration"/> instance.
-    /// Set to <c>-1</c> to leave the degree of parallelism unbounded.
-    /// Initialized with <see cref="Environment.ProcessorCount"/> by default.
+    /// A positive value limits the number of concurrent operations to the set value.
+    /// If set to <c>-1</c>, there is no limit on the number of concurrently running operations.
+    /// Defaults to <see cref="Environment.ProcessorCount"/>.
     /// </summary>
     public int MaxDegreeOfParallelism
     {

src/ImageSharp/Processing/Processors/Convolution/BokehBlurProcessor{TPixel}.cs

@@ -5,6 +5,7 @@
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using SixLabors.ImageSharp.Advanced;
+using SixLabors.ImageSharp.ColorProfiles.Companding;
 using SixLabors.ImageSharp.Memory;
 using SixLabors.ImageSharp.PixelFormats;
 using SixLabors.ImageSharp.Processing.Processors.Convolution.Parameters;
@@ -112,7 +113,7 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
         }
         else
         {
-            ApplyInverseGammaExposureRowOperation operation = new(sourceRectangle, source.PixelBuffer, processingBuffer, this.Configuration, 1 / this.gamma);
+            ApplyInverseGammaExposureRowOperation operation = new(sourceRectangle, source.PixelBuffer, processingBuffer, this.Configuration, this.gamma);
             ParallelRowIterator.IterateRows(
                 this.Configuration,
                 sourceRectangle,
@@ -305,15 +306,9 @@ public void Invoke(int y, Span<Vector4> span)
         {
             Span<TPixel> targetRowSpan = this.targetPixels.DangerousGetRowSpan(y)[this.bounds.X..];
             PixelOperations<TPixel>.Instance.ToVector4(this.configuration, targetRowSpan[..span.Length], span, PixelConversionModifiers.Premultiply);
-            ref Vector4 baseRef = ref MemoryMarshal.GetReference(span);
 
-            for (int x = 0; x < this.bounds.Width; x++)
-            {
-                ref Vector4 v = ref Unsafe.Add(ref baseRef, (uint)x);
-                v.X = MathF.Pow(v.X, this.gamma);
-                v.Y = MathF.Pow(v.Y, this.gamma);
-                v.Z = MathF.Pow(v.Z, this.gamma);
-            }
+            // Input is premultiplied [0,1] so the LUT is safe here.
+            GammaCompanding.Expand(span[..this.bounds.Width], this.gamma);
 
             PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, span, targetRowSpan);
         }
@@ -367,44 +362,34 @@ public void Invoke(int y, Span<Vector4> span)
         private readonly Buffer2D<TPixel> targetPixels;
         private readonly Buffer2D<Vector4> sourceValues;
         private readonly Configuration configuration;
-        private readonly float inverseGamma;
+        private readonly float gamma;
 
         [MethodImpl(InliningOptions.ShortMethod)]
         public ApplyInverseGammaExposureRowOperation(
             Rectangle bounds,
             Buffer2D<TPixel> targetPixels,
             Buffer2D<Vector4> sourceValues,
             Configuration configuration,
-            float inverseGamma)
+            float gamma)
         {
             this.bounds = bounds;
             this.targetPixels = targetPixels;
             this.sourceValues = sourceValues;
             this.configuration = configuration;
-            this.inverseGamma = inverseGamma;
+            this.gamma = gamma;
         }
 
         /// <inheritdoc/>
         [MethodImpl(InliningOptions.ShortMethod)]
         public void Invoke(int y)
         {
-            Vector4 low = Vector4.Zero;
-            Vector4 high = new(float.PositiveInfinity, float.PositiveInfinity, float.PositiveInfinity, float.PositiveInfinity);
-
             Span<TPixel> targetPixelSpan = this.targetPixels.DangerousGetRowSpan(y)[this.bounds.X..];
-            Span<Vector4> sourceRowSpan = this.sourceValues.DangerousGetRowSpan(y)[this.bounds.X..];
-            ref Vector4 sourceRef = ref MemoryMarshal.GetReference(sourceRowSpan);
+            Span<Vector4> sourceRowSpan = this.sourceValues.DangerousGetRowSpan(y).Slice(this.bounds.X, this.bounds.Width);
 
-            for (int x = 0; x < this.bounds.Width; x++)
-            {
-                ref Vector4 v = ref Unsafe.Add(ref sourceRef, (uint)x);
-                Vector4 clamp = Numerics.Clamp(v, low, high);
-                v.X = MathF.Pow(clamp.X, this.inverseGamma);
-                v.Y = MathF.Pow(clamp.Y, this.inverseGamma);
-                v.Z = MathF.Pow(clamp.Z, this.inverseGamma);
-            }
+            Numerics.Clamp(MemoryMarshal.Cast<Vector4, float>(sourceRowSpan), 0, 1F);
+            GammaCompanding.Compress(sourceRowSpan, this.gamma);
 
-            PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, sourceRowSpan[..this.bounds.Width], targetPixelSpan, PixelConversionModifiers.Premultiply);
+            PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, sourceRowSpan, targetPixelSpan, PixelConversionModifiers.Premultiply);
         }
     }
 
@@ -433,17 +418,16 @@ public ApplyInverseGamma3ExposureRowOperation(
 
         /// <inheritdoc/>
         [MethodImpl(InliningOptions.ShortMethod)]
-        public unsafe void Invoke(int y)
+        public void Invoke(int y)
         {
             Span<Vector4> sourceRowSpan = this.sourceValues.DangerousGetRowSpan(y).Slice(this.bounds.X, this.bounds.Width);
-            ref Vector4 sourceRef = ref MemoryMarshal.GetReference(sourceRowSpan);
 
-            Numerics.Clamp(MemoryMarshal.Cast<Vector4, float>(sourceRowSpan), 0, float.PositiveInfinity);
+            Numerics.Clamp(MemoryMarshal.Cast<Vector4, float>(sourceRowSpan), 0, 1F);
             Numerics.CubeRootOnXYZ(sourceRowSpan);
 
             Span<TPixel> targetPixelSpan = this.targetPixels.DangerousGetRowSpan(y)[this.bounds.X..];
 
-            PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, sourceRowSpan[..this.bounds.Width], targetPixelSpan, PixelConversionModifiers.Premultiply);
+            PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, sourceRowSpan, targetPixelSpan, PixelConversionModifiers.Premultiply);
         }
     }
 }

src/ImageSharp/Processing/Processors/Convolution/Convolution2DProcessor{TPixel}.cs

@@ -1,8 +1,8 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
+using System.Buffers;
 using System.Numerics;
-using SixLabors.ImageSharp.Advanced;
 using SixLabors.ImageSharp.Memory;
 using SixLabors.ImageSharp.PixelFormats;
 
@@ -79,10 +79,16 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
                 this.Configuration,
                 this.PreserveAlpha);
 
-            ParallelRowIterator.IterateRows<Convolution2DRowOperation<TPixel>, Vector4>(
-                this.Configuration,
-                interest,
-                in operation);
+            // Convolution is memory-bandwidth-bound with low arithmetic intensity.
+            // Parallelization degrades performance due to cache line contention from
+            // overlapping source row reads. See #3111.
+            using IMemoryOwner<Vector4> buffer = allocator.Allocate<Vector4>(operation.GetRequiredBufferLength(interest));
+            Span<Vector4> span = buffer.Memory.Span;
+
+            for (int y = interest.Top; y < interest.Bottom; y++)
+            {
+                operation.Invoke(y, span);
+            }
         }
 
         Buffer2D<TPixel>.SwapOrCopyContent(source.PixelBuffer, targetPixels);

src/ImageSharp/Processing/Processors/Transforms/CropProcessor{TPixel}.cs

@@ -59,16 +59,14 @@ protected override void OnFrameApply(ImageFrame<TPixel> source, ImageFrame<TPixe
 
         Rectangle bounds = this.cropRectangle;
 
-        // Copying is cheap, we should process more pixels per task:
-        ParallelExecutionSettings parallelSettings =
-            ParallelExecutionSettings.FromConfiguration(this.Configuration).MultiplyMinimumPixelsPerTask(4);
-
+        // Copying is too cheap to benefit from parallelization;
+        // the overhead exceeds the work per task. See #3111.
         RowOperation operation = new(bounds, source.PixelBuffer, destination.PixelBuffer);
 
-        ParallelRowIterator.IterateRows(
-            bounds,
-            in parallelSettings,
-            in operation);
+        for (int y = bounds.Top; y < bounds.Bottom; y++)
+        {
+            operation.Invoke(y);
+        }
     }
 
     /// <summary>

tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs

@@ -224,24 +224,6 @@ public void IterateRows_MaxDegreeOfParallelismMinusOne_ShouldVisitAllRows()
         Assert.Equal(Enumerable.Repeat(1, rectangle.Height), actualData);
     }
 
-    [Fact]
-    public void IterateRowsWithTempBuffer_DefaultSettingsRequireInitialization()
-    {
-        ParallelExecutionSettings parallelSettings = default;
-        Rectangle rect = new(0, 0, 10, 10);
-
-        void RowAction(int y, Span<Rgba32> memory)
-        {
-        }
-
-        TestRowOperation<Rgba32> operation = new(RowAction);
-
-        ArgumentOutOfRangeException ex = Assert.Throws<ArgumentOutOfRangeException>(
-            () => ParallelRowIterator.IterateRows<TestRowOperation<Rgba32>, Rgba32>(rect, in parallelSettings, in operation));
-
-        Assert.Contains(nameof(ParallelExecutionSettings.MaxDegreeOfParallelism), ex.Message);
-    }
-
     public static TheoryData<int, int, int, int, int, int, int> IterateRows_WithEffectiveMinimumPixelsLimit_Data =
         new()
         {
@@ -367,24 +349,6 @@ void RowAction(RowInterval rows, Span<Vector4> buffer)
         Assert.Equal(Enumerable.Repeat(1, rectangle.Height), actualData);
     }
 
-    [Fact]
-    public void IterateRows_DefaultSettingsRequireInitialization()
-    {
-        ParallelExecutionSettings parallelSettings = default;
-        Rectangle rect = new(0, 0, 10, 10);
-
-        void RowAction(int y)
-        {
-        }
-
-        TestRowActionOperation operation = new(RowAction);
-
-        ArgumentOutOfRangeException ex = Assert.Throws<ArgumentOutOfRangeException>(
-            () => ParallelRowIterator.IterateRows(rect, in parallelSettings, in operation));
-
-        Assert.Contains(nameof(ParallelExecutionSettings.MaxDegreeOfParallelism), ex.Message);
-    }
-
     public static readonly TheoryData<int, int, int, int, int, int, int> IterateRectangularBuffer_Data =
         new()
         {