-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add Span.Sort, and make Array.Sort span-based #27700
Conversation
Shares existing Array-based implementation by changing that implementation to be span based.
- Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome!
* Add Span<T>.Sort Shares existing Array-based implementation by changing that implementation to be span based. * Additional work to enable span-based sorts - Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add Span<T>.Sort Shares existing Array-based implementation by changing that implementation to be span based. * Additional work to enable span-based sorts - Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add Span<T>.Sort Shares existing Array-based implementation by changing that implementation to be span based. * Additional work to enable span-based sorts - Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@stephentoub great! 👍 |
case TypeCode.Single: | ||
if (TryGenericSort(keys as float[], items, index, length)) return; | ||
break; | ||
case TypeCode.String: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it intentional to include String here?
I believe that there are subtle differences in how the current culture is handled between the non-generic and generic default comparer, so using this path for string may change the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just remove this case then. Thanks.
It is possible that I wonder if there is a behavior change here if previously the native code used the runtime type of the array and now the C# code uses the type of the generic argument. |
All these cases should work fine. The switch is done on TypeCode. It that prevents |
* Add Span<T>.Sort Shares existing Array-based implementation by changing that implementation to be span based. * Additional work to enable span-based sorts - Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add Span<T>.Sort Shares existing Array-based implementation by changing that implementation to be span based. * Additional work to enable span-based sorts - Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@nietras @stephentoub great work! /cc @damageboy who is working on a vectorized version of Array.Sort ;) |
* Add Span<T>.Sort Shares existing Array-based implementation by changing that implementation to be span based. * Additional work to enable span-based sorts - Cleans up changes from the previous commit, e.g. corrects nullable annotations, removes TODOs, using GetRawSzArrayData instead of GetRawArrayData, etc. - Passes spans around rather than spans+lo+hi. - Deletes the native TrySZSort, preferring to use the managed implementation in all cases. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Definitely nice to have this back it managed land, this will make special casing my stuff much more friendly :) |
Replaces #24419 from @nietras.
The first commit is @nietras', rebased on top of master.
The second commit cleans up a few things, including removing the native TrySZSort entirely in favor of just using the managed implementation, and passing around only spans rather than spans + lo index + hi index.
Fixes https://github.com/dotnet/coreclr/issues/27683
Contributes to https://github.com/dotnet/corefx/issues/15329
cc: @jkotas, @nietras
I ran the dotnet/performance Array.Sort benchmarks (old is master, new is this PR):
I also ran a few additional benchmarks to try to measure impact on small arrays where other overheads might show up:
Non-generic takes a small hit in overhead, but the numbers here are also miniscule, and the non-generic methods are generally not used by anyone who cares about this level of perf.