-
Notifications
You must be signed in to change notification settings - Fork 5k
[API Proposal]: Stable Sort for Spans #60982
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
Comments
Tagging subscribers to this area: @GrabYourPitchforks, @dotnet/area-system-memory Issue DetailsBackground and motivation@hickford explains the motivation in #15803:
There are workarounds, but they all come at the cost of higher memory and CPU consumption:
API Proposalnamespace System
{
// This is a copy/paste of the span Sort methods, with "stable" prepended to the names.
public static partial class MemoryExtensions
{
public static void StableSort<T>(this Span<T> span);
public static void StableSort<T, TComparer>(this Span<T> span, TComparer comparer) where TComparer : IComparer<T>?;
public static void StableSort<T>(this Span<T> span, Comparison<T> comparison);
public static void StableSort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items);
public static void StableSort<TKey, TValue, TComparer>(this Span<TKey> keys, Span<TValue> items, TComparer comparer) where TComparer : IComparer<TKey>?;
public static void StableSort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items, Comparison<TKey> comparison);
}
} API UsageThis will be used the same way as the existing sort methods are used on arrays and spans. var strings = new string {"B", "A", "b" , "a"};
strings.StableSort(StringComparer.OrdinalIgnoreCase);
// strings is now guaranteed to be in the order {"A", "a", "B", "b"} But how to stably sort a list? Fortunately, even without changing var strings = new List<string>(new[] {"B", "A", "b" , "a"});
strings.AsSpan().StableSort(StringComparer.OrdinalIgnoreCase); Alternative Designs
Risks
|
Repathing to runtime because the algorithm folks tend to monitor that label. |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsBackground and motivation@hickford explains the motivation in #15803:
There are workarounds, but they all come at the cost of higher memory and CPU consumption:
API Proposalnamespace System
{
// This is a copy/paste of the span Sort methods, with "stable" prepended to the names.
public static partial class MemoryExtensions
{
public static void StableSort<T>(this Span<T> span);
public static void StableSort<T, TComparer>(this Span<T> span, TComparer comparer) where TComparer : IComparer<T>?;
public static void StableSort<T>(this Span<T> span, Comparison<T> comparison);
public static void StableSort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items);
public static void StableSort<TKey, TValue, TComparer>(this Span<TKey> keys, Span<TValue> items, TComparer comparer) where TComparer : IComparer<TKey>?;
public static void StableSort<TKey, TValue>(this Span<TKey> keys, Span<TValue> items, Comparison<TKey> comparison);
}
} API UsageThis will be used the same way as the existing sort methods are used on arrays and spans. var strings = new string {"B", "A", "b" , "a"};
strings.StableSort(StringComparer.OrdinalIgnoreCase);
// strings is now guaranteed to be in the order {"A", "a", "B", "b"} But how to stably sort a list? Fortunately, even without changing var strings = new List<string>(new[] {"B", "A", "b" , "a"});
strings.AsSpan().StableSort(StringComparer.OrdinalIgnoreCase); Alternative Designs
Risks
|
Minor thing: Implicit conversions are not considered for extension method resolution. This isn't insurmountable, but it does reduce discoverability more since this new extension group also won't show up in IntelliSense for |
Thanks for pointing that our @Joe4evr, I've updated the proposal to reflect that. |
What do I need to do to get this proposal reviewed? |
This needs some additional consideration/weigh in from folks like @stephentoub. This seems like a fairly scenario specific thing that could be achieved via a NuGet package if desired/necessary. It would likewise be good to explicitly list scenarios where stable sort is necessary. For |
My stance is largely the same as @tannergooding's. There are already multiple available nuget packages that proffer stable sorting routines; are they insufficient? If not, why not? A great sorting implementation is a non-trivial amount of code to tune and maintain, especially one that covers all the variations suggested in the proposal. And I'd expect the cited workarounds along with nuget implementations to cover the majority of the needs such that this wouldn't need to be built-in. I'm not wholly against it, but we'd need really good motivation to undertake it (and the cited motivation of "others do it so you should to" generally doesn't meet the bar ;-). |
@stephentoub i have a use case where winforms needs a stable sort. i don't think relying on a nuget package is a good option. dotnet/winforms#5202 Regarding maintenance, one option would be to port an existing well-tested and well-vetted routine from another runtime, eg Java's timsort or Rust's timsort-esque routine. A java port may make the most sense because the code maps fairly straightforwardly over to c#. And it has been used in production for many years. (for reference, under 1000 lines of code, authored by josh bloch so you know it's good ;-) ) |
I believe that winforms can achieve what it needs just fine even without stable sort provided by core libraries: dotnet/winforms#5202 (comment) . We can continue discussion on the winforms issue.
The code you have linked has GPL license. GPL is not compatible with this project. We would not be able to accept an implementation that was based on it. |
Uh oh!
There was an error while loading. Please reload this page.
Background and motivation
@hickford explains the motivation in #15803:
There are workarounds, but they all come at the cost of higher memory and CPU consumption:
Enumerable.OrderBy()
is stable, but is not in-place.API Proposal
API Usage
This will be used the same way as the existing sort methods are used on spans.
EDIT: @Joe4evr points out that implicit conversions are not considered for extension method resolution. So stably sorting an array requires an
AsSpan()
call:Sorting a list will also require calling
AsSpan()
:Alternative Designs
Sort
methods stable. This would be a slight performance regression for many uses, but would avoid the need to add new APIs.StableSort
method group toArray
and/orList
so users don't have to callAsSpan()
, at the cost of code/metadata bloat on a core type. An extension method group could improve discoverability without adding weight to trimmed builds, but it's not necessary to unlock this functionality.Risks
StableSort
, because "stable" is "better", and lose performance for no good reason.The text was updated successfully, but these errors were encountered: