Add Sort(...) extension methods for Span<T> #16986
Conversation
src/mscorlib/Resources/Strings.resx
Outdated
| <value>Unable to sort because the IComparable.CompareTo() method returns inconsistent results. Either a value does not compare equal to itself, or one value repeatedly compared to another value yields different results. IComparable: '{0}'.</value> | ||
| </data> | ||
| <data name="Arg_ItemsMustHaveSameLengthAsKeys" xml:space="preserve"> | ||
| <value>Items must have same length as keys</value> |
| @@ -491,6 +491,19 @@ | |||
| <Compile Include="$(MSBuildThisFileDirectory)System\SpanHelpers.cs" /> | |||
| <Compile Include="$(MSBuildThisFileDirectory)System\SpanHelpers.BinarySearch.cs" /> | |||
| <Compile Include="$(MSBuildThisFileDirectory)System\SpanHelpers.Byte.cs" /> | |||
| <Compile Include="$(MSBuildThisFileDirectory)System\SpanSortHelpers.KeysValues.TDirectComparer.cs" /> | |||
| <Compile Include="$(MSBuildThisFileDirectory)System\SpanSortHelpers.Common.cs" /> | |||
There was a problem hiding this comment.
nit: sort order alphabetically
nietras
changed the title
| internal interface IDirectComparer<in T> | ||
| { | ||
| bool GreaterThan(T x, T y); | ||
| bool LessThan(T x, T y); |
There was a problem hiding this comment.
Can we use default implementation syntax? If yes perhaps we could get more short source code.
| ref TKey keys, int length, | ||
| Comparison<TKey> comparison) | ||
| { | ||
| int depthLimit = 2 * FloorLog2PlusOne(length); |
There was a problem hiding this comment.
Argument in FloorLog2PlusOne() should be >= 2 but we haven't check for this. Perhaps it makes sense to return 0 or 1 from the method in the case.
| ref TKey keys, int length) | ||
| { | ||
| // Type unfolding adopted from https://github.com/dotnet/coreclr/blob/master/src/classlibnative/bcltype/arrayhelpers.cpp#L268 | ||
| if (typeof(TKey) == typeof(sbyte)) |
There was a problem hiding this comment.
We could use TypeCode.
Also here it is better to use switch {} instead of many if-else-if.
| if (typeof(TKey) == typeof(sbyte)) | ||
| { | ||
| ref var specificKeys = ref Unsafe.As<TKey, sbyte>(ref keys); | ||
| Sort(ref specificKeys, length, new SByteDirectComparer()); |
There was a problem hiding this comment.
Can we exclude allocations of DirectComparer-s with static?
| return true; | ||
| } | ||
| // TODO: Specialize for string if necessary. What about the == null checks? | ||
| //else if (typeof(TKey) == typeof(string)) |
There was a problem hiding this comment.
I guess perhaps we could get a perf win only for Ordinal and OrdinalIgnoreCase.
| { | ||
| int j = i; | ||
|
|
||
| var t = Unsafe.Add(ref keys, j + 1); |
There was a problem hiding this comment.
Testing my code I discovered that incrementing ref-s by one is more faster:
ref var t = ref keys;
...
t = Unsafe.Add(ref keys, 1);I don't test but we could replace "hi" and "lo" (and others) with ref-s and use methods like Unsafe. IsAddressGreaterThan(). Perhaps it will simplify boundary control.
|
Hello @nietras , do you plan to take this forward? If this is to get into 3.0 we will have to keep this going now. Thanks for what you have done so far! |
@danmosemsft I would love for this to get into 3.0, and could try another stab at it, but as per dotnet/corefx#26859 (comment) and the corefx PR the version of Sort that I implemented has been denied due to security concerns, see the corefx PR. Basically, since I used Anyway, I will try to reach out to @jkotas again to ask what he thinks is the way forward. |
|
Superseded by #24419 |
WIP: Work in progress, see dotnet/corefx#26859
All code review, not specific to coreclr, should be done in the corefx PR.
@ahsonkhan here Span.Sort in coreclr, this builds, but I have not yet had time to test this in corefx yet.