Remove Closure Allocations when Iterating Rows in Parallel #1107

JimBobSquarePants · 2020-02-06T12:29:34Z

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

The way ImageSharp currently handles work-to-be-executed in parallel, at the simplest level, as a Action<RowInterval>. Any state necessary for the operation must therefore be provided as closures, incurring heap allocations.

By utilizing a trick to exploit the reification in C# generics it's possible completely get rid of callvirt instructions when invoking a delegate passed as argument which, in turn, allows the compiler to inline the input delegate. This removes the closure allocations and importantly removes the risk of developers inadvertently passing this as an allocation.

We can perform this optimization by updating the current ParallelHelper.IterateRows methods and variants to accept a readonly struct implementing an interface instead the current action. This optimization technique is already utilized within the IImageProcessor<TPixel> API.

The interface pattern is as follows.

/// <summary>
/// Defines the contract for an action that operates on a row interval.
/// </summary>
public interface IRowIntervalAction
{
    /// <summary>
    /// Invokes the method passing the row interval.
    /// </summary>
    /// <param name="rows">The row interval.</param>
    void Invoke(in RowInterval rows);
}

/// <summary>
/// Defines the contract for an action that operates on a row interval with a temporary buffer.
/// </summary>
/// <typeparam name="TBuffer">The type of buffer elements.</typeparam>
public interface IRowIntervalAction<TBuffer>
    where TBuffer : unmanaged
{
    /// <summary>
    /// Invokes the method passing the row interval and a buffer.
    /// </summary>
    /// <param name="rows">The row interval.</param>
    /// <param name="memory">The contiguous region of memory.</param>
    void Invoke(in RowInterval rows, Memory<TBuffer> memory);
}

And the methods as follows.

/// <summary>
/// Iterate through the rows of a rectangle in optimized batches defined by <see cref="RowInterval"/>-s.
/// </summary>
/// <typeparam name="T">The type of row action to perform.</typeparam>
/// <param name="rectangle">The <see cref="Rectangle"/>.</param>
/// <param name="parallelSettings">The <see cref="ParallelExecutionSettings"/>.</param>
/// <param name="body">The method body defining the iteration logic on a single <see cref="RowInterval"/>.</param>
public static void IterateRows<T>(
    Rectangle rectangle,
    in ParallelExecutionSettings parallelSettings,
    in T body)
    where T : struct, IRowIntervalAction

/// <summary>
/// Iterate through the rows of a rectangle in optimized batches defined by <see cref="RowInterval"/>-s
/// instantiating a temporary buffer for each <paramref name="body"/> invocation.
/// </summary>
internal static void IterateRows<T, TBuffer>(
    Rectangle rectangle,
    in ParallelExecutionSettings parallelSettings,
    in T body)
    where T : struct, IRowIntervalAction<TBuffer>
    where TBuffer : unmanaged

TODO

Define new interfaces and methods
Refactor existing processor to use new methods
Remove old methods and update tests

codecov · 2020-02-06T13:00:41Z

Codecov Report

Merging #1107 into master will decrease coverage by 0.18%.
The diff coverage is 94.21%.

@@            Coverage Diff             @@
##           master    #1107      +/-   ##
==========================================
- Coverage   82.23%   82.04%   -0.19%     
==========================================
  Files         702      700       -2     
  Lines       29163    28883     -280     
  Branches     3297     3286      -11     
==========================================
- Hits        23981    23698     -283     
- Misses       4491     4497       +6     
+ Partials      691      688       -3

Flag	Coverage Δ
#unittests	`82.04% <94.21%> (-0.19%)`	⬇️

Impacted Files	Coverage Δ
...lFormats/Utils/Vector4Converters.RgbaCompatible.cs	`91.42% <ø> (ø)`	⬆️
src/ImageSharp/Primitives/Rectangle.cs	`95% <ø> (ø)`	⬆️
...c/ImageSharp/Advanced/ParallelExecutionSettings.cs	`100% <ø> (ø)`
...ageSharp/Common/Extensions/EnumerableExtensions.cs	`83.33% <ø> (ø)`	⬆️
...ng/Processors/Transforms/Resize/ResizeKernelMap.cs	`95.78% <ø> (ø)`	⬆️
...ssing/Processors/Transforms/Resize/ResizeWorker.cs	`98.7% <ø> (ø)`	⬆️
...ocessors/Quantization/QuantizeProcessor{TPixel}.cs	`0% <0%> (ø)`	⬆️
...ing/Processors/Transforms/CropProcessor{TPixel}.cs	`100% <100%> (ø)`	⬆️
...ors/Convolution/EdgeDetector2DProcessor{TPixel}.cs	`100% <100%> (ø)`	⬆️
...ssors/Overlays/BackgroundColorProcessor{TPixel}.cs	`100% <100%> (ø)`	⬆️
... and 32 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 aaa5bb7...c66dd0c. Read the comment docs.

…delegate WIP: single row value delegates for parallel iterator

…l-value-delegate" This reverts commit 1835475, reversing changes made to dd7bd7b.

JimBobSquarePants · 2020-02-11T23:19:19Z

3... 2... 1... 😉

antonfirsov

Just a few wishes regarding naming of internals I find misleading at the moment, otherwise LGTM.

antonfirsov · 2020-02-11T23:43:37Z

src/ImageSharp/Advanced/ParallelRowIterator.Wrappers.cs

+            }
+        }
+
+        private readonly struct WrappingRowIntervalOperation<T>


Shouldn't this be RowIntervalOperationWrapper<T> instead? Took me a a while to figure out it is exactly the opposite as it's name :)

Haha, yeah no problem.

antonfirsov · 2020-02-11T23:49:06Z

src/ImageSharp/Advanced/ParallelRowIterator.Wrappers.cs

+    /// </content>
+    public static partial class ParallelRowIterator
+    {
+        private readonly struct WrappingRowIntervalInfo


This deserves a better name. IterationParameters maybe?

I'm fine with that, will update.

antonfirsov · 2020-02-11T23:50:24Z

src/ImageSharp/Advanced/ParallelRowIterator.Wrappers.cs

+            public readonly int MinY;
+            public readonly int MaxY;
+            public readonly int StepY;
+            public readonly int MaxX;


Shouldn't this be Width instead? First I thought there is a bug, and we are actually allocating a buffer with the size defined by the maximum x coordinate.

Sounds good to me.

JimBobSquarePants added 10 commits February 5, 2020 12:23

Add row action overload

aaa5bb7

Merge branch 'master' into js/parallel-allocation-experiments

b6d8375

Reduce captuing allocation to 16 bytes.

4523366

Add memorydiagnoser

463f0a9

Normalize row action with buffer.

abb064f

Remove final allocations

e3a7bb4

Rename class and add native memory profiler

62890f8

Convert AffineTranformProcessor

7f19fb2

Update ProjectiveTransformProcessor{TPixel}.cs

01f2c05

Internalize, partially optimize and rename Action methods.

e40731d

JimBobSquarePants added API area:performance labels Feb 6, 2020

Fix non-windows build

3b3e6ab

Sergio0694 and others added 15 commits February 6, 2020 17:00

Add XML comments to the CropProcessor<TPixel>

079e2a6

Refactor BokehBlurProcessor<TPixel>

2f83149

Refactor Convolution2PassProcessor<TPixel>

6298c78

Refactor ConvolutionProcessor<TPixel>

c5ed869

Refactor EdgeDetectorCompassProcessor<TPixel>

58418b4

Refactor Convolution2DProcessor<TPixel>

1564149

Add readonly modifiers to APIs in Rectangle

d844f8a

Removed unnecessary XML comments

2bc5d82

Refactor DrawImageProcessor<TPixelBg, TPixelFg>

b77a27c

Refactor OilPaintingProcessor<TPixel>

d5d50e9

Refactor PixelRowDelegateProcessorBase<TPixel>

f49d1b6

Major refactor to the pixel row delegate processors

6d4d9a6

Refactor FilterProcessor

d39a057

Minor perf update

c6c6b9b

Update ProjectiveTransformProcessor{TPixel}.cs

e9e461e

Refactor single row APIs

d6fb30e

Sergio0694 mentioned this pull request Feb 7, 2020

Single row value delegates for parallel iterator #1108

Merged

4 tasks

Sergio0694 and others added 12 commits February 7, 2020 17:32

Add single row value delegate with buffer

dd4285d

Refactor processors to single row with buffer APIs

3da200d

Rename APIs

b545ea0

Merge pull request #1108 from SixLabors/sp/single-row-parallel-value-…

1835475

…delegate WIP: single row value delegates for parallel iterator

Update pixelate processor

fcf7c44

Revert "Merge pull request #1108 from SixLabors/sp/single-row-paralle…

8540f83

…l-value-delegate" This reverts commit 1835475, reversing changes made to dd7bd7b.

Update PixelateProcessor{TPixel}.cs

89e3898

Update AdaptiveHistogramEqualizationProcessor and rename interfaces

2b00433

Update AdaptiveHistogramEqualizationProcessor{TPixel}.cs

7a8edff

Update AdaptiveHistogramEqualizationSlidingWindowProcessor{TPixel}.cs

7609a53

Fix arguments order and tests

c946849

Use Span<T> as buffer param

b247bac

JimBobSquarePants changed the title ~~WIP: Remove Closure Allocations when Iterating Rows in Parallel~~ Remove Closure Allocations when Iterating Rows in Parallel Feb 9, 2020

JimBobSquarePants requested a review from a team February 9, 2020 13:22

JimBobSquarePants added this to the 1.0.0-rc1 milestone Feb 9, 2020

JimBobSquarePants added 4 commits February 10, 2020 13:55

Update ProjectiveTransformProcessor{TPixel}.cs

c5166c9

Update EdgeDetectorCompassProcessor{TPixel}.cs

3cfe575

Update BinaryThresholdProcessor{TPixel}.cs

49d0215

Cleanup

eefa57e

antonfirsov approved these changes Feb 11, 2020

View reviewed changes

JimBobSquarePants added 3 commits February 12, 2020 11:03

Undo unrelated changes and clean up.

4b7718e

Update QuantizeProcessor{TPixel}.cs

b5d055f

Rename things

c66dd0c

Sergio0694 mentioned this pull request Feb 12, 2020

Update type constraints and other optimizations #1111

Closed

2 tasks

JimBobSquarePants merged commit 1a1c319 into master Feb 12, 2020

JimBobSquarePants deleted the js/parallel-allocation-experiments branch February 12, 2020 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Closure Allocations when Iterating Rows in Parallel #1107

Remove Closure Allocations when Iterating Rows in Parallel #1107

JimBobSquarePants commented Feb 6, 2020 •

edited

Loading

codecov bot commented Feb 6, 2020 •

edited

Loading

JimBobSquarePants commented Feb 11, 2020

antonfirsov left a comment

antonfirsov Feb 11, 2020 •

edited

Loading

JimBobSquarePants Feb 12, 2020

antonfirsov Feb 11, 2020

JimBobSquarePants Feb 12, 2020

antonfirsov Feb 11, 2020 •

edited

Loading

JimBobSquarePants Feb 12, 2020

Remove Closure Allocations when Iterating Rows in Parallel #1107

Remove Closure Allocations when Iterating Rows in Parallel #1107

Conversation

JimBobSquarePants commented Feb 6, 2020 • edited Loading

Prerequisites

Description

TODO

codecov bot commented Feb 6, 2020 • edited Loading

Codecov Report

JimBobSquarePants commented Feb 11, 2020

antonfirsov left a comment

Choose a reason for hiding this comment

antonfirsov Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants Feb 12, 2020

Choose a reason for hiding this comment

antonfirsov Feb 11, 2020

Choose a reason for hiding this comment

JimBobSquarePants Feb 12, 2020

Choose a reason for hiding this comment

antonfirsov Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants Feb 12, 2020

Choose a reason for hiding this comment

JimBobSquarePants commented Feb 6, 2020 •

edited

Loading

codecov bot commented Feb 6, 2020 •

edited

Loading

antonfirsov Feb 11, 2020 •

edited

Loading

antonfirsov Feb 11, 2020 •

edited

Loading