-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Avoid sort allocations #116109
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
Avoid sort allocations #116109
Conversation
|
@EgorBot -intel using BenchmarkDotNet.Attributes;
public class Bench
{
int[] a = new int[10000];
Comparison<int> c = (int x, int y) => x - y;
[IterationSetup]
public void IterationSetup() { for (int i = 0; i < a.Length; i++) a[i] = i; }
[Benchmark]
public void Sort() => Array.Sort(a, c);
} |
|
Could you please share perf numbers that demonstrate the performance improvements? The one micro-benchmark that I have kicked off above shows ~10% regression: EgorBot/runtime-utils#370 (comment) . |
|
No, I haven't done any performance testing. |
|
@2A5F You can use the benchmarks from the performance repo (we use them for ensuring there are no regressions): You can both trace files and disassembler output: https://github.com/dotnet/performance/tree/main/src/benchmarks/micro#quick-start |
c46122b to
1b22a69
Compare
|
@adamsitnik Thanks for the notice Test Code[Benchmark]
public void Span_ComparerStruct() => _arrays[_iterationIndex++].AsSpan(0, Size).Sort(new ComparableComparerStruct());
private readonly struct ComparableComparerStruct : IComparer<T>
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int Compare(T x, T y) => x.CompareTo(y);
}All test on Int32
BigStruct
IntClass
IntStruct
String
|
|
Ideas and todo:
|
|
To avoid duplicate code, could you do something like this: struct DelegateWrappedComparer<T> : IComparer<T>
{
private Comparer<T> _comparer;
public int Compare(T? x, T? y) => _comparer.Compare(x, y);
}And point the "old" version of the method to the new one, with this wrapper? Hope this is clear. |
|
@andrewjsaid For Delegate is This test has expired/is invalid. I will retest the new code later when I have time. (benchmarks are really slow) |
|
My reasoning was that by using the struct wrapper, methods like the below would be specialized for that struct and the JIT would be able to inline the call to private static void SwapIfGreater<TComparer>(Span<T> keys, TComparer comparer, int i, int j)
where TComparer : IComparer<T>, allows ref structHowever on further reflection I note that currently since |
|
|
Oops already suggested 🤦 Thanks for the link |
6c02cb3 to
25b1914
Compare
|
I accidentally pushed to the wrong branch; the tags and review may need to be adjusted. |
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs
Outdated
Show resolved
Hide resolved
The throughput should be fixable by introducing even more code duplication. (A separate question is whether the code duplication is worth it.) |
|
I assume this is related? https://github.com/dotnet/runtime/pull/116109/checks?check_run_id=51639673842 |
|
Looks like a problem with #116109 (comment) . The special casing is needed to substitute null comparer with a default one. |
|
@EgorBot -intel using BenchmarkDotNet.Attributes;
public class Bench
{
int[] a = new int[10000];
int[] b = new int[10000];
MyComparer c = new MyComparer();
[IterationSetup]
public void IterationSetup() { for (int i = 0; i < a.Length; i++) a[i] = i; }
[Benchmark]
public void Sort() => Array.Sort(a, b, c);
class MyComparer : IComparer<int>
{
public int Compare(int x, int y) => x-y;
}
} |
|
Looks the benchmark numbers shared by @EgorBo not showing any regression. @jkotas what do you think? |
|
@EgorBot -intel -arm |
|
LGTM (with the caveats listed in #116109 (comment)). |
|
@EgorBot -intel -arm -commit 1c955dd vs previous |
|
@EgorBot -intel -arm -commit 1c955dd vs previous |
@tarekgh The benchmark had mistake and it was not actually testing the code changed in this PR. The updated results show up to ~3x regressions for comparers that are larger structs: EgorBot/runtime-utils#549 (comment) . I think it is still fine to keep this change. Larger structs are frowned upon, so it should be ok to regress comparers that are larger structs and improve comparers that are small structs. |
Modified
ArraySortHelperand addedArraySortHelper<T, TComparer>to allow generic struct TComparer without memory allocationSpan.Sort<T, TComparer>will not box now#39466
#39543