-
-
Notifications
You must be signed in to change notification settings - Fork 893
Replace parallel row iteration with sequential loops in Crop/Convolution Processors #3113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
a9c43dc
abafe10
0220108
e405df5
9438c8b
656f53d
03670ab
3a87933
418f9c3
4d844ba
6998d43
5166bed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||
| // Copyright (c) Six Labors. | ||||
| // Licensed under the Six Labors Split License. | ||||
|
|
||||
| using System.Buffers; | ||||
| using System.Numerics; | ||||
| using System.Runtime.CompilerServices; | ||||
| using System.Runtime.InteropServices; | ||||
|
|
@@ -106,6 +107,12 @@ protected override void OnFrameApply(ImageFrame<TPixel> source) | |||
|
|
||||
| mapXY.BuildSamplingOffsetMap(this.KernelX.Length, this.KernelX.Length, interest, this.BorderWrapModeX, this.BorderWrapModeY); | ||||
|
|
||||
| MemoryAllocator allocator = this.Configuration.MemoryAllocator; | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to enable warnings that help detecting unused locals or dead code?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was weird I missed that. I'll see what I can enable. Fixed anyway. I didn't use your suggestion as there was an addition rogue |
||||
| // Convolution is memory-bandwidth-bound with low arithmetic intensity. | ||||
| // Parallelization degrades performance due to cache line contention from | ||||
| // overlapping source row reads. See #3111. | ||||
|
|
||||
| // Horizontal convolution | ||||
| HorizontalConvolutionRowOperation horizontalOperation = new( | ||||
| interest, | ||||
|
|
@@ -116,10 +123,15 @@ protected override void OnFrameApply(ImageFrame<TPixel> source) | |||
| this.Configuration, | ||||
| this.PreserveAlpha); | ||||
|
|
||||
| ParallelRowIterator.IterateRows<HorizontalConvolutionRowOperation, Vector4>( | ||||
| this.Configuration, | ||||
| interest, | ||||
| in horizontalOperation); | ||||
| using (IMemoryOwner<Vector4> hBuffer = allocator.Allocate<Vector4>(horizontalOperation.GetRequiredBufferLength(interest))) | ||||
| { | ||||
| Span<Vector4> hSpan = hBuffer.Memory.Span; | ||||
|
|
||||
| for (int y = interest.Top; y < interest.Bottom; y++) | ||||
| { | ||||
| horizontalOperation.Invoke(y, hSpan); | ||||
| } | ||||
| } | ||||
|
|
||||
| // Vertical convolution | ||||
| VerticalConvolutionRowOperation verticalOperation = new( | ||||
|
|
@@ -131,10 +143,15 @@ protected override void OnFrameApply(ImageFrame<TPixel> source) | |||
| this.Configuration, | ||||
| this.PreserveAlpha); | ||||
|
|
||||
| ParallelRowIterator.IterateRows<VerticalConvolutionRowOperation, Vector4>( | ||||
| this.Configuration, | ||||
| interest, | ||||
| in verticalOperation); | ||||
| using (IMemoryOwner<Vector4> vBuffer = allocator.Allocate<Vector4>(verticalOperation.GetRequiredBufferLength(interest))) | ||||
|
antonfirsov marked this conversation as resolved.
Outdated
|
||||
| { | ||||
| Span<Vector4> vSpan = vBuffer.Memory.Span; | ||||
|
|
||||
| for (int y = interest.Top; y < interest.Bottom; y++) | ||||
| { | ||||
| verticalOperation.Invoke(y, vSpan); | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| /// <summary> | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The operation type is clearly unneeded here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wanted to do the minimal amount of work required to fix the issues for now. |
||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for
Convolution2DRowOperationisolation? IMO removing it/moving it to a private method would simplify code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None other than I want to make minimal changes.