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

Jump to bottom

System.Linq Orderby performance optimization #46414

Closed
wants to merge 1 commit into from
Closed

System.Linq Orderby performance optimization #46414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 17 additions & 8 deletions src/libraries/System.Linq/src/System/Linq/OrderedEnumerable.SpeedOpt.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ public TElement[] ToArray()
{
return buffer._items;
}

if (CanInPlaceSort())
{
SortElements(buffer);
return buffer._items;
}
TElement[] array = new TElement[count];
int[] map = SortedMap(buffer);
for (int i = 0; i != array.Length; i++)
Expand All @@ -33,16 +37,21 @@ public List<TElement> ToList()
{
Buffer<TElement> buffer = new Buffer<TElement>(_source);
int count = buffer._count;
if (count == 0)
{
return new List<TElement>(count);
}
if (CanInPlaceSort())
{
SortElements(buffer);
return new List<TElement>(buffer._items);
}
List<TElement> list = new List<TElement>(count);
if (count > 0)
int[] map = SortedMap(buffer);
for (int i = 0; i != count; i++)
{
int[] map = SortedMap(buffer);
for (int i = 0; i != count; i++)
{
list.Add(buffer._items[map[i]]);
}
list.Add(buffer._items[map[i]]);
}

return list;
}

Expand Down
42 changes: 39 additions & 3 deletions src/libraries/System.Linq/src/System/Linq/OrderedEnumerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ internal abstract partial class OrderedEnumerable<TElement> : IOrderedEnumerable

private int[] SortedMap(Buffer<TElement> buffer) => GetEnumerableSorter().Sort(buffer._items, buffer._count);

private void SortElements(Buffer<TElement> buffer) => GetEnumerableSorter().SortElements(buffer._items, buffer._count);
Copy link
Member

Choose a reason for hiding this comment

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

I would use a name that emphasizes that this is an in-place sort, i.e. buffer is going to get mutated.


private int[] SortedMap(Buffer<TElement> buffer, int minIdx, int maxIdx) =>
GetEnumerableSorter().Sort(buffer._items, buffer._count, minIdx, maxIdx);

Expand All @@ -24,10 +26,21 @@ public IEnumerator<TElement> GetEnumerator()
Buffer<TElement> buffer = new Buffer<TElement>(_source);
if (buffer._count > 0)
{
int[] map = SortedMap(buffer);
for (int i = 0; i < buffer._count; i++)
if (CanInPlaceSort())
{
yield return buffer._items[map[i]];
SortElements(buffer);
for (int i = 0; i < buffer._count; i++)
{
yield return buffer._items[i];
}
}
else
{
int[] map = SortedMap(buffer);
for (int i = 0; i < buffer._count; i++)
{
yield return buffer._items[map[i]];
}
}
}
}
Expand Down Expand Up @@ -72,6 +85,8 @@ internal IEnumerator<TElement> GetEnumerator(int minIdx, int maxIdx)
IOrderedEnumerable<TElement> IOrderedEnumerable<TElement>.CreateOrderedEnumerable<TKey>(Func<TElement, TKey> keySelector, IComparer<TKey>? comparer, bool descending) =>
new OrderedEnumerable<TElement, TKey>(_source, keySelector, comparer, @descending, this);

protected abstract bool CanInPlaceSort();

public TElement? TryGetLast(Func<TElement, bool> predicate, out bool found)
{
CachingComparer<TElement> comparer = GetComparer();
Expand Down Expand Up @@ -158,6 +173,16 @@ internal override CachingComparer<TElement> GetComparer(CachingComparer<TElement
: new CachingComparerWithChild<TElement, TKey>(_keySelector, _comparer, _descending, childComparer);
return _parent != null ? _parent.GetComparer(cmp) : cmp;
}

protected override bool CanInPlaceSort()
Copy link
Member

Choose a reason for hiding this comment

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

This would be a good place to write a comment detailing the motivations for this optimization.

{
if (_parent != null)
{
return false;
}
Type elementType = typeof(TElement);
return elementType.IsValueType || (elementType.IsClass && !elementType.IsArray);
Copy link
Member

@eiriktsarpalis eiriktsarpalis Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This expression could be optimized away by the JIT if you avoided assigning typeof(TElement) to an intermediate variable:

https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACAygCzACcAHAGTACMA6AJQFcA7YASygFMBuddAbQCkWwAOLtG7IiwDGACmABPEuwD2AMxktmASi0BddHADM2RNgDCAHgAqAUQA27DswB82AN7psX7J+8lJAG5gwOzYypQAVuxSwNgA+iTEYrEAvNiM9HZ23GjePrnevl5G2JTKynbmYIwAkowACnZgUuy4ykTAMlpF7j15LKrYMglJzNgAhGkZWd0FeV4ec/N5cADs2KpgdhBcfd4AvjxLeVaKoewOTsCnSthpCkpqMraXyVo5y8XrF47JN+zUGoQABqW3o7H+2BgMCGPyu/0BEDMTQgEGwADJ0RM4X8zoiAIJEIhgeTvHqHJY9EplCpVWoNJotNodBBdHqLT7YAZDEZEZITKaZOyzTkczneNYbLY7D6fCmcvZfbAPFTqF6/bSI0F2cGQ6FDFVPdVXLSI5GQNGY5VnI32DXAU1AwnE0myrwU9D7IA=

}
}

// A comparer that chains comparisons, and pushes through the last element found to be
Expand Down Expand Up @@ -262,6 +287,12 @@ internal int[] Sort(TElement[] elements, int count)
return map;
}

internal void SortElements(TElement[] elements, int count)
{
ComputeKeys(elements, count);
QuickSortElements(elements, 0, count - 1);
}

internal int[] Sort(TElement[] elements, int count, int minIdx, int maxIdx)
{
int[] map = ComputeMap(elements, count);
Expand All @@ -279,6 +310,8 @@ internal TElement ElementAt(TElement[] elements, int count, int idx)

protected abstract void QuickSort(int[] map, int left, int right);

protected abstract void QuickSortElements(TElement[] map, int left, int right);

// Sorts the k elements between minIdx and maxIdx without sorting all elements
// Time complexity: O(n + k log k) best and average case. O(n^2) worse case.
protected abstract void PartialQuickSort(int[] map, int left, int right, int minIdx, int maxIdx);
Expand Down Expand Up @@ -343,6 +376,9 @@ internal override int CompareAnyKeys(int index1, int index2)
protected override void QuickSort(int[] keys, int lo, int hi) =>
new Span<int>(keys, lo, hi - lo + 1).Sort(CompareAnyKeys);

protected override void QuickSortElements(TElement[] element, int lo, int hi) =>
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, I would call this something like QuickSortInPlace or something to that effect.

new Span<TKey>(_keys, lo, hi - lo + 1).Sort(new Span<TElement>(element, lo, hi-lo +1), _comparer);
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem to be taking descending sorts into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch, I tried using the overload taking Comparison<TKey> and made a function like CompareAnyKeys. This is causing a huge perf degredation, it appears that it is due to the additional method call to the Comparable<TKey> created in Sort and and the call to new new Comparison<TKey>. Since Sort doesn't support descending the only way to maintain this Perf is to restrict this to only when doing ascending which I think is a bad idea. If you don't have any further suggestion I'll abandon this PR.

Method Job Toolchain N Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
LongToList Job-KMOQSK IComparison<TKey> 200 15,966.5 ns 736.64 ns 788.20 ns 15,986.1 ns 14,693.0 ns 17,493.0 ns 1.1421 - - 5104 B
LongToList Job-BJFYTT upstream 200 15,546.1 ns 773.05 ns 859.24 ns 15,410.0 ns 14,292.5 ns 17,462.7 ns 1.3152 - - 5904 B
LongToList Job-BCHDBG Comparable<TKey> 200 3,087.8 ns 177.38 ns 189.79 ns 3,065.8 ns 2,846.0 ns 3,512.2 ns 1.1559 - - 5016 B


// Sorts the k elements between minIdx and maxIdx without sorting all elements
// Time complexity: O(n + k log k) best and average case. O(n^2) worse case.
protected override void PartialQuickSort(int[] map, int left, int right, int minIdx, int maxIdx)
Expand Down