Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[WIP] Add Sort(...) extension methods for Span<T> #26859

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
92702fe
add function signatures and minimal xml doc
nietras Dec 23, 2017
0c99da2
add helper stuff and scaffolding
nietras Dec 23, 2017
48f655d
copy code from ArraySortHelper in coreclr
nietras Dec 23, 2017
57eb72f
more porting work
nietras Dec 23, 2017
2b8b08f
compiles
nietras Dec 23, 2017
4272823
comparablespansorthelper
nietras Dec 23, 2017
25fc3c6
fix make generic sorter
nietras Dec 25, 2017
22d819b
fix porting bug
nietras Dec 25, 2017
ba1da6a
remove unnecessary swap checks, add reverse test
nietras Dec 29, 2017
ec77c64
fast middle via uint
nietras Dec 29, 2017
4f500a3
cleanup and a few comments
nietras Dec 29, 2017
4f2f822
optimize DownHeap
nietras Dec 29, 2017
758762b
add simple perf tests, with issues...
nietras Dec 29, 2017
fcb63b5
remove span sort perf for loop
nietras Dec 29, 2017
eff4ab4
show type specialization can make perf as good or better as array sort.
nietras Dec 30, 2017
0d99df1
make default sort helper static readonly
nietras Dec 30, 2017
03b1bef
expand perf tests
nietras Dec 30, 2017
84691a2
add new no sorting needed perf tests
nietras Dec 30, 2017
c8dff35
fix sort perf test
nietras Jan 1, 2018
5352890
Add implementation code from bb5c9f89 in DotNetCross.Sorting "span-so…
nietras Jan 28, 2018
c109f6a
Add KeysAndOrValues code from 4942ee74 in DotNetCross.Sorting "span-…
nietras Jan 29, 2018
48be72d
Update to a71460be with Sort tests including possible overflow test.
nietras Jan 29, 2018
7468485
Update to fdbb7bcf from DotNetCross/Sorting
nietras Feb 5, 2018
78aa18c
Add bogus comparer/comparable detection and exception throwing. Fix f…
nietras Feb 11, 2018
07b0105
Add TComparer, Comparison variants, remove bounds checks for TDirectC…
nietras Mar 13, 2018
04c5c0b
cleanup MemoryExtensions
nietras Mar 18, 2018
ced09bd
address ahsonkhan feedback, before sort test split up
nietras Mar 18, 2018
4259560
Split Sort test code into three files.
nietras Mar 18, 2018
9527a49
Remove uncommented Sort3 code
nietras Mar 18, 2018
146029f
Add below lines to comments
nietras Mar 18, 2018
e44b8b6
Remove AggressiveInlining attributes
nietras Mar 18, 2018
596df18
Add license to Common
nietras Mar 19, 2018
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
7 changes: 7 additions & 0 deletions src/System.Memory/ref/System.Memory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@ public static class MemoryExtensions
public static int BinarySearch<T>(this Span<T> span, IComparable<T> comparable) { throw null; }
public static int BinarySearch<T, TComparable>(this Span<T> span, TComparable comparable) where TComparable : IComparable<T> { throw null; }
public static int BinarySearch<T, TComparer>(this Span<T> span, T value, TComparer comparer) where TComparer : IComparer<T> { throw null; }

public static void Sort<T>(this Span<T> span) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

nice: new line.

Generate the reference using our tool.
Run msbuild /t:GenerateReferenceSource from the S.Memory source directory.

cc @weshaggard if you have any questions.

public static void Sort<T, TComparer>(this Span<T> span, TComparer comparer) where TComparer : IComparer<T> { throw null; }
public static void Sort<T>(this Span<T> span, Comparison<T> comparison) { throw null; }
public static void Sort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items) { throw null; }
public static void Sort<TKey, TValue, TComparer>(this Span<TKey> keys, Span<TValue> items, TComparer comparer) where TComparer : IComparer<TKey> { throw null; }
public static void Sort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items, Comparison<TKey> comparison) { throw null; }
}

public readonly struct ReadOnlyMemory<T>
Expand Down
10 changes: 10 additions & 0 deletions src/System.Memory/src/System.Memory.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@
<Compile Include="System\SpanHelpers.BinarySearch.cs" />
<Compile Include="System\SpanHelpers.T.cs" />
<Compile Include="System\SpanHelpers.byte.cs" />
<Compile Include="System\SpanSortHelpers.Common.cs" />
<Compile Include="System\SpanSortHelpers.Keys.cs" />
<Compile Include="System\SpanSortHelpers.Keys.IComparable.cs" />
<Compile Include="System\SpanSortHelpers.Keys.Specialized.cs" />
<Compile Include="System\SpanSortHelpers.Keys.TComparer.cs" />
<Compile Include="System\SpanSortHelpers.KeysAndOrValues.cs" />
<Compile Include="System\SpanSortHelpers.KeysValues.cs" />
<Compile Include="System\SpanSortHelpers.KeysValues.IComparable.cs" />
<Compile Include="System\SpanSortHelpers.KeysValues.Specialized.cs" />
<Compile Include="System\SpanSortHelpers.KeysValues.TComparer.cs" />
<Compile Include="System\ThrowHelper.cs" />
<Compile Include="System\Buffers\OperationStatus.cs" />
<Compile Include="System\Buffers\Binary\Reader.cs" />
Expand Down
92 changes: 92 additions & 0 deletions src/System.Memory/src/System/MemoryExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@
using Internal.Runtime.CompilerServices;
#endif

using SHC = System.SpanSortHelpersCommon;
// Consolidated code
//using SHK = System.SpanSortHelpersKeysAndOrValues;
//using SHKV = System.SpanSortHelpersKeysAndOrValues;
// Specialized for either only keys or keys and values and for comparable or not
using SHK = System.SpanSortHelpersKeys;
using SHKV = System.SpanSortHelpersKeysValues;

namespace System
{
/// <summary>
Expand Down Expand Up @@ -922,5 +930,89 @@ public static int BinarySearch<T, TComparer>(
value, comparer);
return BinarySearch(span, comparable);
}


/// <summary>
/// Sorts the elements in the entire <see cref="Span{T}" />
/// using the <see cref="IComparable" /> implementation of each
/// element of the <see cref= "Span{T}" />
/// </summary>
/// <param name="span">The <see cref="Span{T}"/> to sort.</param>
/// <exception cref = "InvalidOperationException">
/// One or more elements do not implement the <see cref="IComparable" /> interface.
/// </exception>
// TODO: Revise exception list, if we do not try/catch
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Sort<T>(this Span<T> span)
Copy link
Member

Choose a reason for hiding this comment

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

nit: use abbreviated syntax, here and elsewhere

public static void Sort<T>(this Span<T> span) => SHK.Sort(span);

{
SHK.Sort(span);
}

/// <summary>
/// Sorts the elements in the entire <see cref="Span{T}" />
/// using the <typeparamref name="TComparer" />.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Sort<T, TComparer>(this Span<T> span, TComparer comparer)
where TComparer : IComparer<T>
{
SHK.Sort(span, comparer);
}

/// <summary>
/// Sorts the elements in the entire <see cref="Span{T}" />
/// using the <see cref="Comparison{T}" />.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Sort<T>(this Span<T> span, Comparison<T> comparison)
{
if (comparison == null)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparison);

SHK.Sort(span, new SHC.ComparisonComparer<T>(comparison));
}

/// <summary>
/// Sorts a pair of spans
/// (one contains the keys <see cref="Span{TKey}"/>
/// and the other contains the corresponding items <see cref="Span{TValue}"/>)
/// based on the keys in the first <see cref= "Span{TKey}" />
/// using the <see cref="IComparable" /> implementation of each
/// element of the <see cref= "Span{TKey}"/>.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Sort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items)
{
SHKV.Sort(keys, items);
}

/// <summary>
/// Sorts a pair of spans
/// (one contains the keys <see cref="Span{TKey}"/>
/// and the other contains the corresponding items <see cref="Span{TValue}"/>)
/// based on the keys in the first <see cref= "Span{TKey}" />
/// using the <typeparamref name="TComparer" />.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Sort<TKey, TValue, TComparer>(this Span<TKey> keys,
Span<TValue> items, TComparer comparer)
where TComparer : IComparer<TKey>
{
SHKV.Sort(keys, items, comparer);
}

/// <summary>
/// Sorts a pair of spans
/// (one contains the keys <see cref="Span{TKey}"/>
/// and the other contains the corresponding items <see cref="Span{TValue}"/>)
/// based on the keys in the first <see cref= "Span{TKey}" />
/// using the <see cref="Comparison{TKey}" />.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Sort<TKey, TValue>(this Span<TKey> keys,
Span<TValue> items, Comparison<TKey> comparison)
{
SHKV.Sort(keys, items, new SHC.ComparisonComparer<TKey>(comparison));
}
}
}
224 changes: 224 additions & 0 deletions src/System.Memory/src/System/SpanSortHelpers.Common.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.CompilerServices;

#if !netstandard
using Internal.Runtime.CompilerServices;
#endif

namespace System
{
// TODO: Rename to SpanSortHelpers before move to corefx
Copy link
Member

Choose a reason for hiding this comment

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

remove TODO?

internal static partial class SpanSortHelpersCommon
{

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove new line

// This is the threshold where Introspective sort switches to Insertion sort.
// Empirically, 16 seems to speed up most cases without slowing down others, at least for integers.
// Large value types may benefit from a smaller number.
internal const int IntrosortSizeThreshold = 16;

internal static int FloorLog2PlusOne(int n)
{
Debug.Assert(n >= 2);
int result = 2;
n >>= 2;
while (n > 0)
{
++result;
n >>= 1;
}
return result;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void Swap<T>(ref T items, int i, int j)
{
Debug.Assert(i != j);
Swap(ref Unsafe.Add(ref items, i), ref Unsafe.Add(ref items, j));
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void Swap<T>(ref T a, ref T b)
{
T temp = a;
a = b;
b = temp;
}

// This started out with just LessThan.
// However, due to bogus comparers, comparables etc.
// we need preserve semantics completely to get same result.
internal interface IDirectComparer<in T>
{
bool GreaterThan(T x, T y);
bool LessThan(T x, T y);
bool LessThanEqual(T x, T y); // TODO: Delete if we are not doing specialize Sort3
}
//
Copy link
Member

Choose a reason for hiding this comment

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

nit: add new line

// Type specific DirectComparer(s) to ensure optimal code-gen
Copy link
Member

@ahsonkhan ahsonkhan Mar 19, 2018

Choose a reason for hiding this comment

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

Are these type-specific structs really necessary for optimal code-gen?

cc @AndyAyersMS

Copy link
Member

Choose a reason for hiding this comment

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

@nietras can you compare perf with and without these extra comparers, and if it's quite different, show the key bits of generated code?

Also there still seems to be a healthy dose of AggressiveInlining. I realize it is time consuming to evaluate each of these on its own merits but we do need to be careful with framework code not to use this too broadly. I'm not asking you to remove them wholesale, but I would think they'd be less necessary at the outer edges of this and really only important in the innermost parts of sorting where we expect overhead costs to be magnified. So please think critically about where this attribute is needed. If I'm wrong and there is some benefit to having them higher up it would be good to see examples.

Also (not necessarily critiquing this change, just wondering aloud) it seems like the same algorithmic patterns are duplicated across different comparison contexts, and I wonder if there is some way to avoid this, or whether we are missing language features and/or concepts that would allow there to be one master version that gets appropriately specialized and optimized, or whether this is just how things are done for now. I know we haven't put the effort into optimizing general comparers the way we have for equality comparers -- maybe we should?

Copy link
Author

Choose a reason for hiding this comment

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

@AndyAyersMS perf is significantly lower if using IComparer compared to the direct comparers. Array.Sort does the same for this, but via C++ template. The problem of course is IComparer will do more than one compare. I saw differences up to 2x for some scenarios. In fact, many scenarios could be improved if direct comparer was used for say float keys and int items/values. But due to the inconsistencies in sorting in Array.Sort, this would mean differences in sorting output.

So yes, these are necessary, and I would really like for perf to be good on existing runtimes.

//
internal struct SByteDirectComparer : IDirectComparer<sbyte>
{
[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.

Are all these AggressiveInlining attributes necessary? I think we should remove them and only use them sparingly where it really matters and we know that the JIT can't do it. At least, that is the guideline I have been given.

cc @jkotas, @AndyAyersMS

Copy link
Author

Choose a reason for hiding this comment

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

@ahsonkhan I'll await feedback before deleting them, but deleting LessThanEqual since not needed.

Copy link
Member

Choose a reason for hiding this comment

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

My guess would be that most of these attributes are not needed -- if a method simply invokes another method it is quite likely the jit will inline it without the attribute.

Overall guidance is that you should consider adding the AggressiveInlining attribute only if both of the following are true:

  • use of the attribute leads to large performance improvements in important scenarios
  • use of the attribute does not create large or unbounded amounts of code growth

In particular you need to be careful when using it in combination with generics as the runtime can create large numbers of generic instantiations and the benefits of inlining are often type specific.

public bool GreaterThan(sbyte x, sbyte y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(sbyte x, sbyte y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(sbyte x, sbyte y) => x <= y;
}
internal struct ByteDirectComparer : IDirectComparer<byte>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(byte x, byte y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(byte x, byte y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(byte x, byte y) => x <= y;
}
internal struct Int16DirectComparer : IDirectComparer<short>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(short x, short y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(short x, short y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(short x, short y) => x <= y;
}
internal struct UInt16DirectComparer : IDirectComparer<ushort>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(ushort x, ushort y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(ushort x, ushort y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(ushort x, ushort y) => x <= y;
}
internal struct Int32DirectComparer : IDirectComparer<int>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(int x, int y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(int x, int y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(int x, int y) => x <= y;
}
internal struct UInt32DirectComparer : IDirectComparer<uint>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(uint x, uint y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(uint x, uint y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(uint x, uint y) => x <= y;
Copy link
Member

Choose a reason for hiding this comment

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

nit: re-use GreaterThan, i.e. !(x > y)

Copy link
Author

Choose a reason for hiding this comment

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

deleting this since not needed, but in general I would have kept this as is for optimal code-gen

}
internal struct Int64DirectComparer : IDirectComparer<long>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(long x, long y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(long x, long y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(long x, long y) => x <= y;
}
internal struct UInt64DirectComparer : IDirectComparer<ulong>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(ulong x, ulong y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(ulong x, ulong y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(ulong x, ulong y) => x <= y;
}
internal struct SingleDirectComparer : IDirectComparer<float>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(float x, float y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(float x, float y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(float x, float y) => x <= y;
}
internal struct DoubleDirectComparer : IDirectComparer<double>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(double x, double y) => x > y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(double x, double y) => x < y;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(double x, double y) => x <= y;
}
// TODO: Revise whether this is needed
internal struct StringDirectComparer : IDirectComparer<string>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(string x, string y) => x.CompareTo(y) > 0;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(string x, string y) => x.CompareTo(y) < 0;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(string x, string y) => x.CompareTo(y) <= 0;
}

// Helper to allow sharing code
// Does not work well for reference types
internal struct ComparerDirectComparer<T, TComparer> : IDirectComparer<T>
where TComparer : IComparer<T>
{
readonly TComparer _comparer;

public ComparerDirectComparer(TComparer comparer)
{
_comparer = comparer;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(T x, T y) => _comparer.Compare(x, y) > 0;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(T x, T y) => _comparer.Compare(x, y) < 0;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(T x, T y) => _comparer.Compare(x, y) <= 0;
}
// Helper to allow sharing code
// Does not work well for reference types
internal struct ComparableDirectComparer<T> : IDirectComparer<T>
where T : IComparable<T>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool GreaterThan(T x, T y) => x.CompareTo(y) > 0;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThan(T x, T y) => x.CompareTo(y) < 0;
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool LessThanEqual(T x, T y) => x.CompareTo(y) <= 0;
}

// Helper to allow sharing code (TODO: This probably has issues for reference types...)
internal struct ComparisonComparer<T> : IComparer<T>
{
readonly Comparison<T> m_comparison;

public ComparisonComparer(Comparison<T> comparison)
{
m_comparison = comparison;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int Compare(T x, T y) => m_comparison(x, y);
}


internal interface IIsNaN<T>
{
bool IsNaN(T value);
}
internal struct SingleIsNaN : IIsNaN<float>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool IsNaN(float value) => float.IsNaN(value);
}
internal struct DoubleIsNaN : IIsNaN<double>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool IsNaN(double value) => double.IsNaN(value);
}
}
}
Loading