Skip to content
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.

Sign up for GitHub

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

Optimize Convolution #1465

Merged
merged 13 commits into from
Dec 10, 2020
Merged

Optimize Convolution #1465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2eb09c3
First working no-clamp version for 2 pass convolution
JimBobSquarePants Dec 4, 2020
a071d7e
Naming tweaks
JimBobSquarePants Dec 4, 2020
170d220
All tests pass
JimBobSquarePants Dec 4, 2020
c6c867c
Refactor 2D and cleanup
JimBobSquarePants Dec 5, 2020
81f21e5
Update KernelSamplingMap.cs
JimBobSquarePants Dec 5, 2020
6e76887
Explicit in
JimBobSquarePants Dec 5, 2020
811600a
Use faster GetSpan()
JimBobSquarePants Dec 6, 2020
656ca73
Avoid per-index multiply.
JimBobSquarePants Dec 6, 2020
e89f967
Merge branch 'master' into js/convolution-experiments
JimBobSquarePants Dec 6, 2020
0be08a2
Working version per-row
JimBobSquarePants Dec 7, 2020
fd2ece0
Cleanup
JimBobSquarePants Dec 7, 2020
9e3dd8a
Merge branch 'master' into js/convolution-experiments
JimBobSquarePants Dec 8, 2020
863bddb
Merge branch 'master' into js/convolution-experiments
JimBobSquarePants Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,11 @@
*.pvr binary
*.snk binary
*.tga binary
*.tif binary
*.tiff binary
*.ttc binary
*.ttf binary
*.wbmp binary
*.webp binary
*.woff binary
*.woff2 binary
Expand Down
279 changes: 0 additions & 279 deletions src/ImageSharp/Common/Helpers/DenseMatrixUtils.cs
View file
Open in desktop

This file was deleted.

7 changes: 6 additions & 1 deletion src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.Buffer{T}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,13 @@ public override Span<T> GetSpan()
{
ThrowObjectDisposedException();
}

#if SUPPORTS_CREATESPAN
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came up during profiling. On NET Core 3.1 We can use a shortcut since we know the length.

ref byte r0 = ref MemoryMarshal.GetReference<byte>(this.Data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small note - this is implicitly creating a Span<byte> from the array. When we add .NET 5 support we can optimize this even further by using MemoryMarshal.GetArrayDataReference instead and completely bypass all checks 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I was looking at that. I still don't know what to do re .NET 5 though. It's be our first target framework which isn't LTS.

Copy link
Member

@Sergio0694 Sergio0694 Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. The way I see it, we could maybe do:

  • Add a .NET 5 target to start preparing the codebase for .NET 6. When .NET 6 lands, immediately switch to it and drop .NET 5, since support for it would end 3 months after .NET 6 is released anyways.
  • Just hold our breath until .NET 6 lands and stick to .NET Core 3.1 as max until then.

I'd say it might be worth doing an initial investigation for starters, to see how many places in the lib could actually benefit from .NET 5 exclusive APIs that are not on .NET Core 3.1? Then we could decide based on that 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARM intrinsics is a big one.

Copy link
Member

@Sergio0694 Sergio0694 Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, right. Yeah that alone might be worth it. Properly supporting ARM64 gives a ton of visibility right now, plus being ready for .NET 6 would be very nice. And if we are to work on ARM64 right now anyway, I don't see the harm in publishing .NET 5 packages already, assuming we'll get a working build soon. After all, companies would likely remain on .NET Core 3.1 until .NET 6 is out, so those wouldn't be affected. And devs on .NET 5 now would be fine with having to jump to .NET 6 immediately anyway, after all they jsut did the same with .NET 5 right now. I'd say if you have time you could look into this and then when you have a branch that shows a nice boost on ARM64, consider shipping if it's really good (and if it's not like mid 2021 already by the time that's done)? 😄

return MemoryMarshal.CreateSpan(ref Unsafe.As<byte, T>(ref r0), this.length);
#else
return MemoryMarshal.Cast<byte, T>(this.Data.AsSpan()).Slice(0, this.length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory here we know that the byte size is an exact multiple of our target length for the T span, so I was thinking we could be able to remove that final Slice call by pre-slicing the Data array. Something like:

return MemoryMarshal.Cast<byte, T>(this.Data.AsSpan(0, this.length * sizeof(T)));

Tried out on sharplab (here) but apparently the codegen is worse for some reason. So yeah I'm not really seeing a way to improve this without having access to the .NET 5 APIs 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep .NET 5 makes this easier.

#endif

}

/// <inheritdoc />
Expand Down
16 changes: 8 additions & 8 deletions src/ImageSharp/Primitives/DenseMatrix{T}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public DenseMatrix(T[,] data)
/// <returns>The <see typeparam="T"/> at the specified position.</returns>
public ref T this[int row, int column]
{
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it AggressiveInlining here and not the internal ShortMethod? Was this intentional or did you just forget to change this back after your initial experiments?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional. ShortMethod was initially designed for certain jpeg profiling but it kinda spread. I don't think we need it any more wit the tooling we have available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. Well we might want to revert that in another PR then 😄

get
{
this.CheckCoordinates(row, column);
Expand All @@ -124,7 +124,7 @@ public DenseMatrix(T[,] data)
/// <returns>
/// The <see cref="DenseMatrix{T}"/> representation on the source data.
/// </returns>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator DenseMatrix<T>(T[,] data) => new DenseMatrix<T>(data);

/// <summary>
Expand All @@ -134,7 +134,7 @@ public DenseMatrix(T[,] data)
/// <returns>
/// The <see cref="T:T[,]"/> representation on the source data.
/// </returns>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
#pragma warning disable SA1008 // Opening parenthesis should be spaced correctly
public static implicit operator T[,](in DenseMatrix<T> data)
#pragma warning restore SA1008 // Opening parenthesis should be spaced correctly
Expand Down Expand Up @@ -175,7 +175,7 @@ public DenseMatrix(T[,] data)
/// Transposes the rows and columns of the dense matrix.
/// </summary>
/// <returns>The <see cref="DenseMatrix{T}"/>.</returns>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public DenseMatrix<T> Transpose()
{
var result = new DenseMatrix<T>(this.Rows, this.Columns);
Expand All @@ -196,13 +196,13 @@ public DenseMatrix<T> Transpose()
/// Fills the matrix with the given value
/// </summary>
/// <param name="value">The value to fill each item with</param>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void Fill(T value) => this.Span.Fill(value);

/// <summary>
/// Clears the matrix setting each value to the default value for the element type
/// </summary>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void Clear() => this.Span.Clear();

/// <summary>
Expand Down Expand Up @@ -232,14 +232,14 @@ public override bool Equals(object obj)
=> obj is DenseMatrix<T> other && this.Equals(other);

/// <inheritdoc/>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool Equals(DenseMatrix<T> other) =>
this.Columns == other.Columns
&& this.Rows == other.Rows
&& this.Span.SequenceEqual(other.Span);

/// <inheritdoc/>
[MethodImpl(InliningOptions.ShortMethod)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override int GetHashCode()
{
HashCode code = default;
Expand Down
Loading