Address Sort<T, TComparer> extensions performance #39543
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
@jkotas I'm struggling with getting a good dev loop here, resorting to running: on the command line. And VS won't open the |
...coreclr/src/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.CoreCLR.cs
Outdated
Show resolved
Hide resolved
It works for me. Do you have the most recent VS update? |
|
Tagging subscribers to this area: @eiriktsarpalis |
Version 16.6.4 which should be latest. But I guess I need a preview of preview.8 it seems:
|
|
Will try latest version from https://github.com/dotnet/installer#installers-and-binaries e.g. Seems to work! 👍 |
…ySortHelper for TKey,TValue scenario, should provide speedup for this.
Yes, it is required to make VS work well. We have it mentioned here https://github.com/dotnet/runtime/blob/master/docs/workflow/requirements/windows-requirements.md#net-sdk |
| { | ||
| // Add a try block here to detect IComparers (or their | ||
| // underlying IComparables, etc) that are bogus. | ||
| try | ||
| { | ||
| comparer ??= Comparer<T>.Default; | ||
| IntrospectiveSort(keys, comparer.Compare); | ||
| if (comparer is null) |
There was a problem hiding this comment.
This will create two instantiations of the sorting code: One on Comparer<T> and second on TComparer.
Can the null check be pushed out to the callers to avoid the duplication where possible?
There was a problem hiding this comment.
@jkotas what if we put !typeof(TComparer).IsValueType && comparer is null?
There was a problem hiding this comment.
comparer is null is JITed into a constant already for structs (that are not Nullable<T>). I do not think this would help.
There was a problem hiding this comment.
yeah I thought so, perhaps I don't fully understand your issue here then given only reference type TComparer should be an issue but that should have a canonical instantiation or? would you mind expanding?
There was a problem hiding this comment.
For example, when this is called from here:
https://github.com/dotnet/runtime/pull/39543/files#diff-d4e4a789c4e124d267dde2cf6505da8eR1760
comparer will be null and so we will always take the first branch (as long as this is the only Sort use for the given T). The JIT or AOT won't be able to figure it out. They will create both generic instantiations of the sorting algorithm.
a canonical instantiation
Yes, instantiations over reference types share code, but there is still duplication of the type system structures.
There was a problem hiding this comment.
there is still duplication of the type system structures.
Right, of course :) Just ball-parking here, but would the following then not ensure we only have a single type system structure for reference type TComparer?
if (typeof(TComparer).IsValueType)
{
ComparerArraySortHelper<TKey, TValue, TComparer>
.IntrospectiveSort(keys, values, comparer);
}
else
{
IComparer<TKey> referenceComparer = comparer ?? Comparer<TKey>.Default;
ComparerArraySortHelper<TKey, TValue, IComparer<TKey>>
.IntrospectiveSort(keys, values, referenceComparer);
}
|
Did you have a chance to get some performance numbers? I am curious what the perf is going to look like. |
Not yet, wanted to finalize impl first. But on that note do you know which Benchmarks Stephen Toub used and where I can find these? Doesn't look like dotnet/performance. We can start with the latter, though, perhaps. |
|
(I used the |
|
@jkotas I can't get: to compile and hence I cannot get a I think I have seen this before, but for the life of me can't remember or find what to do? 😅 |
|
I can build the same code without my changes fine e.g. 2a1595e but for this PR and branch it fails with above errors and: |
|
just taking notes as I'm trying to find a solution. See https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/updating-ref-source.md this say one could: |
🤦♂️ 😁 🤞 |
|
Well first benchmark run is a bust. Something isn't right. 🤔 Int32BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.388 (2004/?/20H1)
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.100-rc.1.20367.2
[Host] : .NET Core 5.0.0 (CoreCLR 5.0.20.36102, CoreFX 5.0.20.36102), X64 RyuJIT
Job-YBDUGJ : .NET Core 5.0 (CoreCLR 42.42.42.42424, CoreFX 42.42.42.42424), X64 RyuJIT
Job-TJRYUK : .NET Core 5.0 (CoreCLR 42.42.42.42424, CoreFX 42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable InvocationCount=5000
IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15
UnrollFactor=1 WarmupCount=1
Look at the IntClassThis appears more inline with expected. Note that
|
|
I did a quick benchmark run with ETWProfiler for both m
pr
Note: Followed https://adamsitnik.com/ETW-Profiler/ for this, since not fresh on my mind. :) |
|
Reran the benchmarks but with
|
|
@jkotas if we focus on the The supposed allocations on |
|
Ha had a file length issue causing me not to be able to load etl files in PerfView. Here is the ComparerStruct. Problem is the private readonly struct ComparableComparerStruct : IComparer<T>
{
public int Compare(T x, T y) => x.CompareTo(y);
}
|
|
@jkotas could the issue above be related to how the instance is created via below? typeof(GenericArraySortHelper<string, Comparer<string>>).TypeHandleScratch that, the isssue is in the benchmark using Array.Sort overloads, not span ones. 🤦♂️ this needs to use span based API of course. Guess should add span based API to benchmarks. |
PR Int32There you go.
|
|
To sum up, running the Anyway, the main issue is the Int32BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.388 (2004/?/20H1)
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.100-rc.1.20367.2
[Host] : .NET Core 5.0.0 (CoreCLR 5.0.20.36102, CoreFX 5.0.20.36102), X64 RyuJIT
Job-SQSCEM : .NET Core 5.0 (CoreCLR 42.42.42.42424, CoreFX 42.42.42.42424), X64 RyuJIT
Job-GAEOPC : .NET Core 5.0 (CoreCLR 42.42.42.42424, CoreFX 42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable InvocationCount=5000
IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15
UnrollFactor=1 WarmupCount=1
|
|
@jkotas latest results via dotnet/performance#1400 Two regressions remain:
Int32Why is the value type comparer scenario faster than simple
BigStruct
Command line |
|
@nietras, are you still working on this? Thanks. |
|
@stephentoub I'm waiting for #39732 to be resolved 😅 |
|
Thanks. @AndyAyersMS, it seems like that's unlikely to be addressed in the foreseeable future? |
|
@CarolEidt is this one of the struct issues we've considered as part of .Net 6 planning? |
No, this is more related to optimization of structs, while the struct work that we've planned for .Net 6 (thus far) is focused primarily on completing the work to ensure that structs passed in registers don't needlessly get forced to the stack. |
|
// Auto-generated message 69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts. To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others. |
|
@AndyAyersMS, recommendations on how to proceed here then? |
|
I've pinged @sandreenko on the linked issue to assess for .Net 6. I'm hoping we can implement some forms of struct copy elimination. |
ghost
closed this
|
Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes. |
ghost
locked as resolved and limited conversation to collaborators
@nietras, now that #39732 was addressed, want to have another go at this? |
@stephentoub hi! Sorry, priorities have shifted since and focusing my spare time on other OSS efforts like https://github.com/nietras/Sep :) I do hope this gets added anyway 🙏 |
ghost
locked as resolved and limited conversation to collaborators
#39466
@jkotas first draft. Please take a look and let me know if this looks like it is on the right path.
TODO:
ArraySortHelper.Mono.cs