From d4f644a23fe9213149aefe6da863ea068ac05661 Mon Sep 17 00:00:00 2001 From: ntr Date: Wed, 29 Nov 2017 13:26:57 +0100 Subject: [PATCH 01/31] S.M: Add initial BinarySearch span extensions method definitions. --- src/System.Memory/src/System.Memory.csproj | 7 +-- .../src/System/MemoryExtensions.cs | 56 ++++++++++++++++++- .../src/System/SpanHelpers.BinarySearch.cs | 37 ++++++++++++ 3 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 src/System.Memory/src/System/SpanHelpers.BinarySearch.cs diff --git a/src/System.Memory/src/System.Memory.csproj b/src/System.Memory/src/System.Memory.csproj index 8cad246ebebf..43700ba4cfa4 100644 --- a/src/System.Memory/src/System.Memory.csproj +++ b/src/System.Memory/src/System.Memory.csproj @@ -23,6 +23,7 @@ + @@ -36,7 +37,6 @@ - @@ -88,7 +88,6 @@ - @@ -96,11 +95,9 @@ - - @@ -145,4 +142,4 @@ - + \ No newline at end of file diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index f7c42ea3a8db..bd987c8b274a 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices; @@ -291,5 +292,58 @@ public static void CopyTo(this T[] array, Memory destination) { array.CopyTo(destination.Span); } + + // TODO: XML doc + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int BinarySearch( + this Span span, IComparable comparable) + { + return BinarySearch>(span, comparable); + } + // TODO: XML doc + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int BinarySearch( + this Span span, TComparable comparable) + where TComparable : IComparable + { + return BinarySearch((ReadOnlySpan)span, comparable); + } + + // TODO: XML doc + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int BinarySearch( + this Span span, T value, TComparer comparer) + where TComparer : IComparer + { + return BinarySearch((ReadOnlySpan)span, value, comparer); + } + + // TODO: XML doc + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int BinarySearch( + this ReadOnlySpan span, IComparable comparable) + { + return BinarySearch>(span, comparable); + } + + // TODO: XML doc + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int BinarySearch( + this ReadOnlySpan span, TComparable comparable) + where TComparable : IComparable + { + return SpanHelpers.BinarySearch(span, comparable); + } + + // TODO: XML doc + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int BinarySearch( + this ReadOnlySpan span, T value, TComparer comparer) + where TComparer : IComparer + { + var comparable = new SpanHelpers.ComparerComparable( + value, comparer); + return BinarySearch(span, comparable); + } } -} \ No newline at end of file +} diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs new file mode 100644 index 000000000000..ed8991ea3cda --- /dev/null +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -0,0 +1,37 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using System.Runtime.InteropServices; +using System.Runtime.CompilerServices; + +namespace System +{ + internal static partial class SpanHelpers + { + public static int BinarySearch( + this ReadOnlySpan span, TComparable comparable) + where TComparable : IComparable + { + throw new NotImplementedException(); + } + + // Helper to allow sharing all code via IComparable inlineable + internal struct ComparerComparable : IComparable + where TComparer : IComparer + { + readonly T _value; + readonly TComparer _comparer; + + public ComparerComparable(T value, TComparer comparer) + { + _value = value; + _comparer = comparer; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public int CompareTo(T other) => _comparer.Compare(_value, other); + } + } +} From 0d76e799a257f882ef3208df306a2f1ed57a3244 Mon Sep 17 00:00:00 2001 From: ntr Date: Mon, 4 Dec 2017 13:20:19 +0100 Subject: [PATCH 02/31] Add dummy xml comments to allow compiling. Add empty test files. --- .../src/System/MemoryExtensions.cs | 105 ++++++++++++++++++ .../Performance/Perf.Span.BinarySearch.cs | 17 +++ .../System.Memory.Performance.Tests.csproj | 3 +- .../tests/ReadOnlySpan/BinarySearch.cs | 30 +++++ .../tests/System.Memory.Tests.csproj | 3 +- 5 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs create mode 100644 src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index bd987c8b274a..64446a569254 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -294,13 +294,48 @@ public static void CopyTo(this T[] array, Memory destination) } // TODO: XML doc + /// + /// Searches the entire sorted for an element + /// using the default comparer and returns the zero-based index of the element. + /// + /// The sorted span to search in. + /// The object to locate as an Comparable. + /// + /// The zero-based index of in the sorted , + /// if is found; otherwise, a negative number that is the bitwise complement + /// of the index of the next element that is larger than or, if there is + /// no larger element, the bitwise complement of . + /// + /// + /// The default comparer cannot + /// find an implementation of the generic interface or + /// the interface for type . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, IComparable comparable) { return BinarySearch>(span, comparable); } + // TODO: XML doc + /// + /// Searches the entire sorted for an element + /// using the default comparer and returns the zero-based index of the element. + /// + /// The sorted span to search in. + /// The object to locate. The value can be null for reference types. + /// + /// The zero-based index of in the sorted , + /// if is found; otherwise, a negative number that is the bitwise complement + /// of the index of the next element that is larger than or, if there is + /// no larger element, the bitwise complement of . + /// + /// + /// The default comparer cannot + /// find an implementation of the generic interface or + /// the interface for type . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, TComparable comparable) @@ -310,6 +345,24 @@ public static int BinarySearch( } // TODO: XML doc + /// + /// Searches the entire sorted for an element + /// using the default comparer and returns the zero-based index of the element. + /// + /// The sorted span to search in. + /// The object to locate. The value can be null for reference types. + /// The comparer to use for searching. + /// + /// The zero-based index of in the sorted , + /// if is found; otherwise, a negative number that is the bitwise complement + /// of the index of the next element that is larger than or, if there is + /// no larger element, the bitwise complement of . + /// + /// + /// The default comparer cannot + /// find an implementation of the generic interface or + /// the interface for type . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, T value, TComparer comparer) @@ -319,6 +372,23 @@ public static int BinarySearch( } // TODO: XML doc + /// + /// Searches the entire sorted for an element + /// using the default comparer and returns the zero-based index of the element. + /// + /// The sorted span to search in. + /// The object to locate. The value can be null for reference types. + /// + /// The zero-based index of in the sorted , + /// if is found; otherwise, a negative number that is the bitwise complement + /// of the index of the next element that is larger than or, if there is + /// no larger element, the bitwise complement of . + /// + /// + /// The default comparer cannot + /// find an implementation of the generic interface or + /// the interface for type . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, IComparable comparable) @@ -327,6 +397,23 @@ public static int BinarySearch( } // TODO: XML doc + /// + /// Searches the entire sorted for an element + /// using the default comparer and returns the zero-based index of the element. + /// + /// The sorted span to search in. + /// The object to locate. The value can be null for reference types. + /// + /// The zero-based index of in the sorted , + /// if is found; otherwise, a negative number that is the bitwise complement + /// of the index of the next element that is larger than or, if there is + /// no larger element, the bitwise complement of . + /// + /// + /// The default comparer cannot + /// find an implementation of the generic interface or + /// the interface for type . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, TComparable comparable) @@ -336,6 +423,24 @@ public static int BinarySearch( } // TODO: XML doc + /// + /// Searches the entire sorted for an element + /// using the default comparer and returns the zero-based index of the element. + /// + /// The sorted span to search in. + /// The object to locate. The value can be null for reference types. + /// The comparer to use for searching. + /// + /// The zero-based index of in the sorted , + /// if is found; otherwise, a negative number that is the bitwise complement + /// of the index of the next element that is larger than or, if there is + /// no larger element, the bitwise complement of . + /// + /// + /// The default comparer cannot + /// find an implementation of the generic interface or + /// the interface for type . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, T value, TComparer comparer) diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs new file mode 100644 index 000000000000..0571e460645d --- /dev/null +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -0,0 +1,17 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Microsoft.Xunit.Performance; +using Xunit; + +namespace System.Memory.Tests +{ + public class Perf_Span_BinarySearch + { + // Existing. + // List: + // https://github.com/dotnet/corefx/blob/157fff35c2427dd0ce5e79557d506e8e7947c8d4/src/System.Collections/tests/Performance/Perf.List.cs#L550 + // https://github.com/dotnet/corefx/blob/157fff35c2427dd0ce5e79557d506e8e7947c8d4/src/System.Collections/tests/Performance/Perf.List.cs#L577 + } +} diff --git a/src/System.Memory/tests/Performance/System.Memory.Performance.Tests.csproj b/src/System.Memory/tests/Performance/System.Memory.Performance.Tests.csproj index a45918d6d5d8..4c1dd8f9d1cf 100644 --- a/src/System.Memory/tests/Performance/System.Memory.Performance.Tests.csproj +++ b/src/System.Memory/tests/Performance/System.Memory.Performance.Tests.csproj @@ -10,6 +10,7 @@ + @@ -30,4 +31,4 @@ - + \ No newline at end of file diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs new file mode 100644 index 000000000000..664aed3aebfd --- /dev/null +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -0,0 +1,30 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Xunit; +using System.Runtime.CompilerServices; + +namespace System.SpanTests +{ + public static partial class ReadOnlySpanTests + { + [Fact] + public static void BinarySearch() + { + uint[] a = { 1, 2, 3, 4 }; + var span = new ReadOnlySpan(a); + + var index = span.BinarySearch(2); + + Assert.Equal(1, index); + } + + //[Fact] + //public static void AsBytesContainsReferences() + //{ + // ReadOnlySpan span = new ReadOnlySpan(Array.Empty()); + // TestHelpers.AssertThrows(span, (_span) => _span.AsBytes().DontBox()); + //} + } +} diff --git a/src/System.Memory/tests/System.Memory.Tests.csproj b/src/System.Memory/tests/System.Memory.Tests.csproj index b2f7fa656ef3..59f228d779ab 100644 --- a/src/System.Memory/tests/System.Memory.Tests.csproj +++ b/src/System.Memory/tests/System.Memory.Tests.csproj @@ -9,6 +9,7 @@ + @@ -149,4 +150,4 @@ - + \ No newline at end of file From 9fb189f5de8db0ca58810176a1909646a49b98e3 Mon Sep 17 00:00:00 2001 From: ntr Date: Mon, 4 Dec 2017 13:41:01 +0100 Subject: [PATCH 03/31] Add to ref. --- src/System.Memory/ref/System.Memory.cs | 9 +++++++++ src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/System.Memory/ref/System.Memory.cs b/src/System.Memory/ref/System.Memory.cs index 0bdb52ba8e57..717c4c2b5454 100644 --- a/src/System.Memory/ref/System.Memory.cs +++ b/src/System.Memory/ref/System.Memory.cs @@ -5,6 +5,8 @@ // Changes to this file must follow the http://aka.ms/api-review process. // ------------------------------------------------------------------------------ +using System.Collections.Generic; + namespace System { public readonly ref struct ReadOnlySpan @@ -130,6 +132,13 @@ public static class MemoryExtensions public static ReadOnlySpan NonPortableCast(this ReadOnlySpan source) where TFrom : struct where TTo : struct { throw null; } public static bool TryGetString(this ReadOnlyMemory readOnlyMemory, out string text, out int start, out int length) { throw null; } + + public static int BinarySearch(this ReadOnlySpan span, IComparable comparable) { throw null; } + public static int BinarySearch(this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { throw null; } + public static int BinarySearch(this ReadOnlySpan span, T value, TComparer comparer) where TComparer : IComparer { throw null; } + public static int BinarySearch(this Span span, IComparable comparable) { throw null; } + public static int BinarySearch(this Span span, TComparable comparable) where TComparable : IComparable { throw null; } + public static int BinarySearch(this Span span, T value, TComparer comparer) where TComparer : IComparer { throw null; } } public readonly struct ReadOnlyMemory diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 664aed3aebfd..344732546165 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -13,9 +13,9 @@ public static partial class ReadOnlySpanTests public static void BinarySearch() { uint[] a = { 1, 2, 3, 4 }; - var span = new ReadOnlySpan(a); + ReadOnlySpan span = new ReadOnlySpan(a); - var index = span.BinarySearch(2); + var index = span.BinarySearch(2u); Assert.Equal(1, index); } From 9b5ad5b0c629b7bb4baa5714244f6f5cb43d5392 Mon Sep 17 00:00:00 2001 From: ntr Date: Mon, 4 Dec 2017 13:45:26 +0100 Subject: [PATCH 04/31] Uncomment in test since extension method not found. --- src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 344732546165..47d0054067fb 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -15,9 +15,9 @@ public static void BinarySearch() uint[] a = { 1, 2, 3, 4 }; ReadOnlySpan span = new ReadOnlySpan(a); - var index = span.BinarySearch(2u); + //var index = span.BinarySearch(2u); - Assert.Equal(1, index); + //Assert.Equal(1, index); } //[Fact] From 8b4a8ba9ea32d1ca15eedaa996cfa71cb52047bd Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 11:35:21 +0100 Subject: [PATCH 05/31] Add initial implementation. --- .../src/System/SpanHelpers.BinarySearch.cs | 56 ++++++++++++++++++- src/System.Memory/src/System/ThrowHelper.cs | 4 ++ .../Performance/Perf.Span.BinarySearch.cs | 1 + .../tests/ReadOnlySpan/BinarySearch.cs | 4 +- 4 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index ed8991ea3cda..cbc661a35762 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -5,16 +5,68 @@ using System.Collections.Generic; using System.Runtime.InteropServices; using System.Runtime.CompilerServices; +using System.Diagnostics; namespace System { internal static partial class SpanHelpers { - public static int BinarySearch( + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static int BinarySearch( this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { - throw new NotImplementedException(); + if (span.IsEmpty) + { return -1; } + + // TODO: Make `readonly ref` when language permits + return BinarySearch(ref span.DangerousGetPinnableReference(), span.Length, comparable); + } + + // TODO: Make `readonly ref` when language permits + // TODO: Make `in TComparable` to allow pass by ref without forcing ref + internal static int BinarySearch( + ref T s, int length, TComparable comparable) + where TComparable : IComparable + { + // Array.BinarySearch implementation: + // https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Array.cs#L802 + int lo = 0; + int hi = length - 1; + while (lo <= hi) + { + // lo or hi will never be negative inside the loop + // TODO: Test/investigate if below is faster (gives better asm), perhaps via Unsafe.As to avoid unnecessary conversions + // This is safe since we know span.Length < int.MaxValue, and indeces are >= 0 + // int i = (int)(((uint)hi + (uint)lo) >> 1) + int i = lo + ((hi - lo) >> 1); + + int c = 0; + try + { + //c = comparable.Compare(Unsafe.Add(ref s, i), value); + // Note this is reversed, in that `value` is before, not after as above + c = comparable.CompareTo(Unsafe.Add(ref s, i)); + } + catch (Exception e) + { + // TODO: Is this correct? Don't like the try/catch, why should we have this?? + ThrowHelper.ThrowInvalidOperationException_IComparableFailed(e); + } + if (c == 0) + { + return i; + } + else if (c > 0) // if (c < 0) old + { + lo = i + 1; + } + else + { + hi = i - 1; + } + } + return ~lo; } // Helper to allow sharing all code via IComparable inlineable diff --git a/src/System.Memory/src/System/ThrowHelper.cs b/src/System.Memory/src/System/ThrowHelper.cs index 2efb54209d78..91df30d46728 100644 --- a/src/System.Memory/src/System/ThrowHelper.cs +++ b/src/System.Memory/src/System/ThrowHelper.cs @@ -68,6 +68,10 @@ internal static class ThrowHelper [MethodImpl(MethodImplOptions.NoInlining)] private static Exception CreateFormatException_BadFormatSpecifier() { return new FormatException(SR.Argument_BadFormatSpecifier); } + internal static void ThrowInvalidOperationException_IComparableFailed(Exception e) { throw CreateInvalidOperationException_IComparableFailed(e); } + [MethodImpl(MethodImplOptions.NoInlining)] + private static Exception CreateInvalidOperationException_IComparableFailed(Exception e) { return new InvalidOperationException(SR.InvalidOperation_IComparableFailed, e); } + // // Enable use of ThrowHelper from TryFormat() routines without introducing dozens of non-code-coveraged "bytesWritten = 0; return false" boilerplate. // diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs index 0571e460645d..065d7d2679f0 100644 --- a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -13,5 +13,6 @@ public class Perf_Span_BinarySearch // List: // https://github.com/dotnet/corefx/blob/157fff35c2427dd0ce5e79557d506e8e7947c8d4/src/System.Collections/tests/Performance/Perf.List.cs#L550 // https://github.com/dotnet/corefx/blob/157fff35c2427dd0ce5e79557d506e8e7947c8d4/src/System.Collections/tests/Performance/Perf.List.cs#L577 + // TODO: What tests do we need here? What types (short, int, string ...)? Range? Scenarios? } } diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 47d0054067fb..344732546165 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -15,9 +15,9 @@ public static void BinarySearch() uint[] a = { 1, 2, 3, 4 }; ReadOnlySpan span = new ReadOnlySpan(a); - //var index = span.BinarySearch(2u); + var index = span.BinarySearch(2u); - //Assert.Equal(1, index); + Assert.Equal(1, index); } //[Fact] From cd7e6a798e4567060eba01ab5182e9e1e2b02f86 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 11:47:38 +0100 Subject: [PATCH 06/31] Add more comments and string test. --- .../src/System/SpanHelpers.BinarySearch.cs | 10 +++++++--- .../tests/ReadOnlySpan/BinarySearch.cs | 13 ++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index cbc661a35762..2f2fea44c73b 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -19,12 +19,12 @@ internal static int BinarySearch( if (span.IsEmpty) { return -1; } - // TODO: Make `readonly ref` when language permits + // TODO: Make `ref readonly`/`in` when language permits return BinarySearch(ref span.DangerousGetPinnableReference(), span.Length, comparable); } - // TODO: Make `readonly ref` when language permits - // TODO: Make `in TComparable` to allow pass by ref without forcing ref + // TODO: Make s `ref readonly`/`in` when language permits + // TODO: Make comparable `ref readonly`/`in` to allow pass by ref without forcing ref internal static int BinarySearch( ref T s, int length, TComparable comparable) where TComparable : IComparable @@ -42,10 +42,14 @@ internal static int BinarySearch( int i = lo + ((hi - lo) >> 1); int c = 0; + // TODO: Is the try/catch needed, can this be removed? try { //c = comparable.Compare(Unsafe.Add(ref s, i), value); // Note this is reversed, in that `value` is before, not after as above + // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, + // if this will be available in language + // TODO: Revise all Unsafe APIs for `readonly` applicability... c = comparable.CompareTo(Unsafe.Add(ref s, i)); } catch (Exception e) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 344732546165..07d3308530c9 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -10,7 +10,7 @@ namespace System.SpanTests public static partial class ReadOnlySpanTests { [Fact] - public static void BinarySearch() + public static void BinarySearch_Int_2() { uint[] a = { 1, 2, 3, 4 }; ReadOnlySpan span = new ReadOnlySpan(a); @@ -20,6 +20,17 @@ public static void BinarySearch() Assert.Equal(1, index); } + [Fact] + public static void BinarySearch_String_B() + { + string[] a = { "a", "b", "c", "d" }; + ReadOnlySpan span = new ReadOnlySpan(a); + + var index = span.BinarySearch("b"); + + Assert.Equal(1, index); + } + //[Fact] //public static void AsBytesContainsReferences() //{ From 039349c89d12d2b58ad95fed250815cd8a2ee783 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 12:50:10 +0100 Subject: [PATCH 07/31] Add more tests. --- src/System.Memory/src/Resources/Strings.resx | 5 ++++- .../src/System/SpanHelpers.BinarySearch.cs | 3 ++- .../tests/ReadOnlySpan/BinarySearch.cs | 15 ++++++++++----- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/System.Memory/src/Resources/Strings.resx b/src/System.Memory/src/Resources/Strings.resx index 48e8eb011cfe..89a20878e22e 100644 --- a/src/System.Memory/src/Resources/Strings.resx +++ b/src/System.Memory/src/Resources/Strings.resx @@ -150,4 +150,7 @@ Precision cannot be larger than {0}. - + + Failed to compare value with comparable. + + \ No newline at end of file diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index 2f2fea44c73b..082c7ad7d240 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -36,8 +36,9 @@ internal static int BinarySearch( while (lo <= hi) { // lo or hi will never be negative inside the loop - // TODO: Test/investigate if below is faster (gives better asm), perhaps via Unsafe.As to avoid unnecessary conversions + // TODO: Test/investigate if below is faster (if it gives better asm), perhaps via Unsafe.As to avoid unnecessary conversions // This is safe since we know span.Length < int.MaxValue, and indeces are >= 0 + // and thus cannot overflow an uint. Saves on subtraction per loop. // int i = (int)(((uint)hi + (uint)lo) >> 1) int i = lo + ((hi - lo) >> 1); diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 07d3308530c9..c97711d0bd38 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -9,15 +9,20 @@ namespace System.SpanTests { public static partial class ReadOnlySpanTests { - [Fact] - public static void BinarySearch_Int_2() + [Theory] + [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 0u, -1)] + [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 1u, 0)] + [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 2u, 1)] + [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 3u, 2)] + [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 4u, 3)] + [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 5u, -5)] + public static void BinarySearch_UInt(uint[] a, uint value, int expectedIndex) { - uint[] a = { 1, 2, 3, 4 }; ReadOnlySpan span = new ReadOnlySpan(a); - var index = span.BinarySearch(2u); + var index = span.BinarySearch(value); - Assert.Equal(1, index); + Assert.Equal(expectedIndex, index); } [Fact] From 3d0ac2076408086b1906296cdb7696056cb638ca Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 16:12:10 +0100 Subject: [PATCH 08/31] Fix string test, add double tests. --- .../tests/ReadOnlySpan/BinarySearch.cs | 49 ++++++++++++++----- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index c97711d0bd38..b2f3dcb53bfe 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -10,30 +10,53 @@ namespace System.SpanTests public static partial class ReadOnlySpanTests { [Theory] - [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 0u, -1)] - [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 1u, 0)] - [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 2u, 1)] - [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 3u, 2)] - [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 4u, 3)] - [InlineData(new uint[] { 1u, 2u, 3u, 4u }, 5u, -5)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 0u, -1)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 1u, 0)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 2u, 1)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 3u, -3)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 4u, 2)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 5u, 3)] + [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 6u, -5)] public static void BinarySearch_UInt(uint[] a, uint value, int expectedIndex) { - ReadOnlySpan span = new ReadOnlySpan(a); + var span = new ReadOnlySpan(a); var index = span.BinarySearch(value); Assert.Equal(expectedIndex, index); } - [Fact] - public static void BinarySearch_String_B() + [Theory] + [InlineData(new double[] { 1, 2, 4, 5 }, 0, -1)] + [InlineData(new double[] { 1, 2, 4, 5 }, 1, 0)] + [InlineData(new double[] { 1, 2, 4, 5 }, 2, 1)] + [InlineData(new double[] { 1, 2, 4, 5 }, 3, -3)] + [InlineData(new double[] { 1, 2, 4, 5 }, 4, 2)] + [InlineData(new double[] { 1, 2, 4, 5 }, 5, 3)] + [InlineData(new double[] { 1, 2, 4, 5 }, 6, -5)] + public static void BinarySearch_Double(double[] a, double value, int expectedIndex) { - string[] a = { "a", "b", "c", "d" }; - ReadOnlySpan span = new ReadOnlySpan(a); + var span = new ReadOnlySpan(a); + + var index = span.BinarySearch(value); - var index = span.BinarySearch("b"); + Assert.Equal(expectedIndex, index); + } - Assert.Equal(1, index); + [Theory] + [InlineData(new string[] { "b", "c", "d", "e" }, "a", -1)] + [InlineData(new string[] { "b", "c", "d", "e" }, "b", 0)] + [InlineData(new string[] { "b", "c", "d", "e" }, "c", 1)] + [InlineData(new string[] { "b", "c", "d", "e" }, "d", 2)] + [InlineData(new string[] { "b", "c", "d", "e" }, "e", 3)] + [InlineData(new string[] { "b", "c", "d", "e" }, "f", -5)] + public static void BinarySearch_String(string[] a, string value, int expectedIndex) + { + var span = new ReadOnlySpan(a); + + var index = span.BinarySearch(value); + + Assert.Equal(expectedIndex, index); } //[Fact] From b84fad2ca04835888238c21680a5ca9ed6424f92 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 16:14:41 +0100 Subject: [PATCH 09/31] cleanup --- .../tests/ReadOnlySpan/BinarySearch.cs | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index b2f3dcb53bfe..7b83f4567568 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using Xunit; -using System.Runtime.CompilerServices; namespace System.SpanTests { @@ -44,12 +43,13 @@ public static void BinarySearch_Double(double[] a, double value, int expectedInd } [Theory] - [InlineData(new string[] { "b", "c", "d", "e" }, "a", -1)] - [InlineData(new string[] { "b", "c", "d", "e" }, "b", 0)] - [InlineData(new string[] { "b", "c", "d", "e" }, "c", 1)] - [InlineData(new string[] { "b", "c", "d", "e" }, "d", 2)] - [InlineData(new string[] { "b", "c", "d", "e" }, "e", 3)] - [InlineData(new string[] { "b", "c", "d", "e" }, "f", -5)] + [InlineData(new string[] { "b", "c", "e", "f" }, "a", -1)] + [InlineData(new string[] { "b", "c", "e", "f" }, "b", 0)] + [InlineData(new string[] { "b", "c", "e", "f" }, "c", 1)] + [InlineData(new string[] { "b", "c", "e", "f" }, "d", -3)] + [InlineData(new string[] { "b", "c", "e", "f" }, "e", 2)] + [InlineData(new string[] { "b", "c", "e", "f" }, "f", 3)] + [InlineData(new string[] { "b", "c", "e", "f" }, "g", -5)] public static void BinarySearch_String(string[] a, string value, int expectedIndex) { var span = new ReadOnlySpan(a); @@ -58,12 +58,5 @@ public static void BinarySearch_String(string[] a, string value, int expectedInd Assert.Equal(expectedIndex, index); } - - //[Fact] - //public static void AsBytesContainsReferences() - //{ - // ReadOnlySpan span = new ReadOnlySpan(Array.Empty()); - // TestHelpers.AssertThrows(span, (_span) => _span.AsBytes().DontBox()); - //} } } From dc79fadca6e70f5c1ba813b14eb1ff0b8ef64525 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 16:28:52 +0100 Subject: [PATCH 10/31] Remove empty check, and add empty tests. --- src/System.Memory/src/System/SpanHelpers.BinarySearch.cs | 3 --- src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index 082c7ad7d240..a7f0a8200d96 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -16,9 +16,6 @@ internal static int BinarySearch( this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { - if (span.IsEmpty) - { return -1; } - // TODO: Make `ref readonly`/`in` when language permits return BinarySearch(ref span.DangerousGetPinnableReference(), span.Length, comparable); } diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 7b83f4567568..105dcf9ee041 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -9,6 +9,7 @@ namespace System.SpanTests public static partial class ReadOnlySpanTests { [Theory] + [InlineData(new uint[] { }, 0u, -1)] [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 0u, -1)] [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 1u, 0)] [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 2u, 1)] @@ -26,6 +27,7 @@ public static void BinarySearch_UInt(uint[] a, uint value, int expectedIndex) } [Theory] + [InlineData(new double[] { }, 0, -1)] [InlineData(new double[] { 1, 2, 4, 5 }, 0, -1)] [InlineData(new double[] { 1, 2, 4, 5 }, 1, 0)] [InlineData(new double[] { 1, 2, 4, 5 }, 2, 1)] @@ -43,6 +45,7 @@ public static void BinarySearch_Double(double[] a, double value, int expectedInd } [Theory] + [InlineData(new string[] { }, null, -1)] [InlineData(new string[] { "b", "c", "e", "f" }, "a", -1)] [InlineData(new string[] { "b", "c", "e", "f" }, "b", 0)] [InlineData(new string[] { "b", "c", "e", "f" }, "c", 1)] From 88c8e0b4e0b8394e40715c3a38106aac31cac7bc Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 16:31:17 +0100 Subject: [PATCH 11/31] Remove xml doc on exception, since that case is not applicable here. --- .../src/System/MemoryExtensions.cs | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index 64446a569254..3834f29533e8 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -306,11 +306,6 @@ public static void CopyTo(this T[] array, Memory destination) /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// - /// - /// The default comparer cannot - /// find an implementation of the generic interface or - /// the interface for type . - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, IComparable comparable) @@ -331,11 +326,6 @@ public static int BinarySearch( /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// - /// - /// The default comparer cannot - /// find an implementation of the generic interface or - /// the interface for type . - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, TComparable comparable) @@ -358,11 +348,6 @@ public static int BinarySearch( /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// - /// - /// The default comparer cannot - /// find an implementation of the generic interface or - /// the interface for type . - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, T value, TComparer comparer) @@ -384,11 +369,6 @@ public static int BinarySearch( /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// - /// - /// The default comparer cannot - /// find an implementation of the generic interface or - /// the interface for type . - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, IComparable comparable) @@ -409,11 +389,6 @@ public static int BinarySearch( /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// - /// - /// The default comparer cannot - /// find an implementation of the generic interface or - /// the interface for type . - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, TComparable comparable) @@ -436,11 +411,6 @@ public static int BinarySearch( /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// - /// - /// The default comparer cannot - /// find an implementation of the generic interface or - /// the interface for type . - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, T value, TComparer comparer) From d86b2480d268fc7e517bfc908706a41ff7130566 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 16:35:45 +0100 Subject: [PATCH 12/31] Remove try/catch, and SR text etc. --- src/System.Memory/src/Resources/Strings.resx | 3 --- .../src/System/SpanHelpers.BinarySearch.cs | 22 +++++-------------- src/System.Memory/src/System/ThrowHelper.cs | 4 ---- 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/src/System.Memory/src/Resources/Strings.resx b/src/System.Memory/src/Resources/Strings.resx index 89a20878e22e..975f4599a8db 100644 --- a/src/System.Memory/src/Resources/Strings.resx +++ b/src/System.Memory/src/Resources/Strings.resx @@ -150,7 +150,4 @@ Precision cannot be larger than {0}. - - Failed to compare value with comparable. - \ No newline at end of file diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index a7f0a8200d96..e374f855c1cd 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -39,22 +39,12 @@ internal static int BinarySearch( // int i = (int)(((uint)hi + (uint)lo) >> 1) int i = lo + ((hi - lo) >> 1); - int c = 0; - // TODO: Is the try/catch needed, can this be removed? - try - { - //c = comparable.Compare(Unsafe.Add(ref s, i), value); - // Note this is reversed, in that `value` is before, not after as above - // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, - // if this will be available in language - // TODO: Revise all Unsafe APIs for `readonly` applicability... - c = comparable.CompareTo(Unsafe.Add(ref s, i)); - } - catch (Exception e) - { - // TODO: Is this correct? Don't like the try/catch, why should we have this?? - ThrowHelper.ThrowInvalidOperationException_IComparableFailed(e); - } + //c = comparable.Compare(Unsafe.Add(ref s, i), value); + // Note this is reversed, in that `value` is before, not after as above + // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, + // if this will be available in language + // TODO: Revise all Unsafe APIs for `readonly` applicability... + var c = comparable.CompareTo(Unsafe.Add(ref s, i)); if (c == 0) { return i; diff --git a/src/System.Memory/src/System/ThrowHelper.cs b/src/System.Memory/src/System/ThrowHelper.cs index 91df30d46728..2efb54209d78 100644 --- a/src/System.Memory/src/System/ThrowHelper.cs +++ b/src/System.Memory/src/System/ThrowHelper.cs @@ -68,10 +68,6 @@ internal static class ThrowHelper [MethodImpl(MethodImplOptions.NoInlining)] private static Exception CreateFormatException_BadFormatSpecifier() { return new FormatException(SR.Argument_BadFormatSpecifier); } - internal static void ThrowInvalidOperationException_IComparableFailed(Exception e) { throw CreateInvalidOperationException_IComparableFailed(e); } - [MethodImpl(MethodImplOptions.NoInlining)] - private static Exception CreateInvalidOperationException_IComparableFailed(Exception e) { return new InvalidOperationException(SR.InvalidOperation_IComparableFailed, e); } - // // Enable use of ThrowHelper from TryFormat() routines without introducing dozens of non-code-coveraged "bytesWritten = 0; return false" boilerplate. // From 0a8a9c2250380d0e23b0e883f54b139d94d3187e Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 16:40:01 +0100 Subject: [PATCH 13/31] minor cleanup --- src/System.Memory/src/System/SpanHelpers.BinarySearch.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index e374f855c1cd..c858ac3536e9 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -3,9 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Collections.Generic; -using System.Runtime.InteropServices; using System.Runtime.CompilerServices; -using System.Diagnostics; namespace System { @@ -39,17 +37,15 @@ internal static int BinarySearch( // int i = (int)(((uint)hi + (uint)lo) >> 1) int i = lo + ((hi - lo) >> 1); - //c = comparable.Compare(Unsafe.Add(ref s, i), value); - // Note this is reversed, in that `value` is before, not after as above // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, // if this will be available in language // TODO: Revise all Unsafe APIs for `readonly` applicability... - var c = comparable.CompareTo(Unsafe.Add(ref s, i)); + int c = comparable.CompareTo(Unsafe.Add(ref s, i)); if (c == 0) { return i; } - else if (c > 0) // if (c < 0) old + else if (c > 0) { lo = i + 1; } From 471f1f71c41c42eab03a98eb97ad1562b6c5efe6 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 18:59:37 +0100 Subject: [PATCH 14/31] refactor tests. --- .../tests/ReadOnlySpan/BinarySearch.cs | 69 ++++++++++++++----- 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 105dcf9ee041..914f081d71fd 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -8,22 +8,57 @@ namespace System.SpanTests { public static partial class ReadOnlySpanTests { - [Theory] - [InlineData(new uint[] { }, 0u, -1)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 0u, -1)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 1u, 0)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 2u, 1)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 3u, -3)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 4u, 2)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 5u, 3)] - [InlineData(new uint[] { 1u, 2u, 4u, 5u }, 6u, -5)] - public static void BinarySearch_UInt(uint[] a, uint value, int expectedIndex) - { - var span = new ReadOnlySpan(a); + public static TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> UIntCases => + new TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> { + (new uint[] { }, 0u, -1), + (new uint[] { 1u, 2u, 4u, 5u }, 0u, -1), + (new uint[] { 1u, 2u, 4u, 5u }, 1u, 0), + (new uint[] { 1u, 2u, 4u, 5u }, 2u, 1), + (new uint[] { 1u, 2u, 4u, 5u }, 3u, -3), + (new uint[] { 1u, 2u, 4u, 5u }, 4u, 2), + (new uint[] { 1u, 2u, 4u, 5u }, 5u, 3), + (new uint[] { 1u, 2u, 4u, 5u }, 6u, -5), + }; + public static TheoryData<(double[] Array, double Value, int ExpectedIndex)> DoubleCases => + new TheoryData<(double[] Array, double Value, int ExpectedIndex)> { + (new double[] { }, 0u, -1), + (new double[] { 1u, 2u, 4u, 5u }, 0u, -1), + (new double[] { 1u, 2u, 4u, 5u }, 1u, 0), + (new double[] { 1u, 2u, 4u, 5u }, 2u, 1), + (new double[] { 1u, 2u, 4u, 5u }, 3u, -3), + (new double[] { 1u, 2u, 4u, 5u }, 4u, 2), + (new double[] { 1u, 2u, 4u, 5u }, 5u, 3), + (new double[] { 1u, 2u, 4u, 5u }, 6u, -5), + }; - var index = span.BinarySearch(value); + [Theory, MemberData(nameof(UIntCases))] + public static void BinarySearch_UInt_Span( + (uint[] Array, uint Value, int ExpectedIndex) c) + { + var index = new Span(c.Array).BinarySearch(c.Value); + Assert.Equal(c.ExpectedIndex, index); + } + [Theory, MemberData(nameof(UIntCases))] + public static void BinarySearch_UInt_ReadOnlySpan( + (uint[] Array, uint Value, int ExpectedIndex) c) + { + var index = new ReadOnlySpan(c.Array).BinarySearch(c.Value); + Assert.Equal(c.ExpectedIndex, index); + } - Assert.Equal(expectedIndex, index); + [Theory, MemberData(nameof(DoubleCases))] + public static void BinarySearch_Double_Span( + (double[] Array, double Value, int ExpectedIndex) c) + { + var index = new Span(c.Array).BinarySearch(c.Value); + Assert.Equal(c.ExpectedIndex, index); + } + [Theory, MemberData(nameof(DoubleCases))] + public static void BinarySearch_Double_ReadOnlySpan( + (double[] Array, double Value, int ExpectedIndex) c) + { + var index = new ReadOnlySpan(c.Array).BinarySearch(c.Value); + Assert.Equal(c.ExpectedIndex, index); } [Theory] @@ -37,7 +72,8 @@ public static void BinarySearch_UInt(uint[] a, uint value, int expectedIndex) [InlineData(new double[] { 1, 2, 4, 5 }, 6, -5)] public static void BinarySearch_Double(double[] a, double value, int expectedIndex) { - var span = new ReadOnlySpan(a); + // Implicitly tests ReadOnlySpan + var span = new Span(a); var index = span.BinarySearch(value); @@ -55,7 +91,8 @@ public static void BinarySearch_Double(double[] a, double value, int expectedInd [InlineData(new string[] { "b", "c", "e", "f" }, "g", -5)] public static void BinarySearch_String(string[] a, string value, int expectedIndex) { - var span = new ReadOnlySpan(a); + // Implicitly tests ReadOnlySpan + var span = new Span(a); var index = span.BinarySearch(value); From d953a2c53022dc49b284936114d9a825254c2ba7 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 19:00:10 +0100 Subject: [PATCH 15/31] remove redundant. --- .../tests/ReadOnlySpan/BinarySearch.cs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 914f081d71fd..7d59408ded76 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -61,25 +61,6 @@ public static void BinarySearch_Double_ReadOnlySpan( Assert.Equal(c.ExpectedIndex, index); } - [Theory] - [InlineData(new double[] { }, 0, -1)] - [InlineData(new double[] { 1, 2, 4, 5 }, 0, -1)] - [InlineData(new double[] { 1, 2, 4, 5 }, 1, 0)] - [InlineData(new double[] { 1, 2, 4, 5 }, 2, 1)] - [InlineData(new double[] { 1, 2, 4, 5 }, 3, -3)] - [InlineData(new double[] { 1, 2, 4, 5 }, 4, 2)] - [InlineData(new double[] { 1, 2, 4, 5 }, 5, 3)] - [InlineData(new double[] { 1, 2, 4, 5 }, 6, -5)] - public static void BinarySearch_Double(double[] a, double value, int expectedIndex) - { - // Implicitly tests ReadOnlySpan - var span = new Span(a); - - var index = span.BinarySearch(value); - - Assert.Equal(expectedIndex, index); - } - [Theory] [InlineData(new string[] { }, null, -1)] [InlineData(new string[] { "b", "c", "e", "f" }, "a", -1)] From 4327a5fe2254d950f154bd3c5241f7d27eaddee9 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 20:39:07 +0100 Subject: [PATCH 16/31] refactor string tests. --- .../tests/ReadOnlySpan/BinarySearch.cs | 66 +++++++++++-------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 7d59408ded76..a601cc84071c 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -22,27 +22,39 @@ public static partial class ReadOnlySpanTests public static TheoryData<(double[] Array, double Value, int ExpectedIndex)> DoubleCases => new TheoryData<(double[] Array, double Value, int ExpectedIndex)> { (new double[] { }, 0u, -1), - (new double[] { 1u, 2u, 4u, 5u }, 0u, -1), - (new double[] { 1u, 2u, 4u, 5u }, 1u, 0), - (new double[] { 1u, 2u, 4u, 5u }, 2u, 1), - (new double[] { 1u, 2u, 4u, 5u }, 3u, -3), - (new double[] { 1u, 2u, 4u, 5u }, 4u, 2), - (new double[] { 1u, 2u, 4u, 5u }, 5u, 3), - (new double[] { 1u, 2u, 4u, 5u }, 6u, -5), + (new double[] { 1.0, 2.0, 4.0, 5u }, 0.0, -1), + (new double[] { 1.0, 2.0, 4.0, 5u }, 1.0, 0), + (new double[] { 1.0, 2.0, 4.0, 5u }, 2.0, 1), + (new double[] { 1.0, 2.0, 4.0, 5u }, 3.0, -3), + (new double[] { 1.0, 2.0, 4.0, 5u }, 4.0, 2), + (new double[] { 1.0, 2.0, 4.0, 5u }, 5.0, 3), + (new double[] { 1.0, 2.0, 4.0, 5u }, 6.0, -5), + }; + public static TheoryData<(string[] Array, string Value, int ExpectedIndex)> StringCases => + new TheoryData<(string[] Array, string Value, int ExpectedIndex)> { + (new string[] { "b", "c", "e", "f" }, "a", -1), + (new string[] { "b", "c", "e", "f" }, "b", 0), + (new string[] { "b", "c", "e", "f" }, "c", 1), + (new string[] { "b", "c", "e", "f" }, "d", -3), + (new string[] { "b", "c", "e", "f" }, "e", 2), + (new string[] { "b", "c", "e", "f" }, "f", 3), + (new string[] { "b", "c", "e", "f" }, "g", -5), }; [Theory, MemberData(nameof(UIntCases))] public static void BinarySearch_UInt_Span( (uint[] Array, uint Value, int ExpectedIndex) c) { - var index = new Span(c.Array).BinarySearch(c.Value); + var span = new Span(c.Array); + var index = span.BinarySearch(c.Value); Assert.Equal(c.ExpectedIndex, index); } [Theory, MemberData(nameof(UIntCases))] public static void BinarySearch_UInt_ReadOnlySpan( (uint[] Array, uint Value, int ExpectedIndex) c) { - var index = new ReadOnlySpan(c.Array).BinarySearch(c.Value); + var span = new ReadOnlySpan(c.Array); + var index = span.BinarySearch(c.Value); Assert.Equal(c.ExpectedIndex, index); } @@ -50,34 +62,34 @@ public static void BinarySearch_UInt_ReadOnlySpan( public static void BinarySearch_Double_Span( (double[] Array, double Value, int ExpectedIndex) c) { - var index = new Span(c.Array).BinarySearch(c.Value); + var span = new Span(c.Array); + var index = span.BinarySearch(c.Value); Assert.Equal(c.ExpectedIndex, index); } [Theory, MemberData(nameof(DoubleCases))] public static void BinarySearch_Double_ReadOnlySpan( (double[] Array, double Value, int ExpectedIndex) c) { - var index = new ReadOnlySpan(c.Array).BinarySearch(c.Value); + var span = new ReadOnlySpan(c.Array); + var index = span.BinarySearch(c.Value); Assert.Equal(c.ExpectedIndex, index); } - [Theory] - [InlineData(new string[] { }, null, -1)] - [InlineData(new string[] { "b", "c", "e", "f" }, "a", -1)] - [InlineData(new string[] { "b", "c", "e", "f" }, "b", 0)] - [InlineData(new string[] { "b", "c", "e", "f" }, "c", 1)] - [InlineData(new string[] { "b", "c", "e", "f" }, "d", -3)] - [InlineData(new string[] { "b", "c", "e", "f" }, "e", 2)] - [InlineData(new string[] { "b", "c", "e", "f" }, "f", 3)] - [InlineData(new string[] { "b", "c", "e", "f" }, "g", -5)] - public static void BinarySearch_String(string[] a, string value, int expectedIndex) + [Theory, MemberData(nameof(StringCases))] + public static void BinarySearch_String_Span( + (string[] Array, string Value, int ExpectedIndex) c) { - // Implicitly tests ReadOnlySpan - var span = new Span(a); - - var index = span.BinarySearch(value); - - Assert.Equal(expectedIndex, index); + var span = new Span(c.Array); + var index = span.BinarySearch(c.Value); + Assert.Equal(c.ExpectedIndex, index); + } + [Theory, MemberData(nameof(StringCases))] + public static void BinarySearch_String_ReadOnlySpan( + (string[] Array, string Value, int ExpectedIndex) c) + { + var span = new ReadOnlySpan(c.Array); + var index = span.BinarySearch(c.Value); + Assert.Equal(c.ExpectedIndex, index); } } } From 9a196588b629e5693e580c296b3e1234c25edcb8 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 20:54:57 +0100 Subject: [PATCH 17/31] refactor tests, add comparer tests. --- .../tests/ReadOnlySpan/BinarySearch.cs | 82 +++++++++++-------- 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index a601cc84071c..3725e320678c 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Collections.Generic; using Xunit; namespace System.SpanTests @@ -42,54 +43,67 @@ public static partial class ReadOnlySpanTests }; [Theory, MemberData(nameof(UIntCases))] - public static void BinarySearch_UInt_Span( + public static void BinarySearch_UInt( (uint[] Array, uint Value, int ExpectedIndex) c) { - var span = new Span(c.Array); - var index = span.BinarySearch(c.Value); - Assert.Equal(c.ExpectedIndex, index); - } - [Theory, MemberData(nameof(UIntCases))] - public static void BinarySearch_UInt_ReadOnlySpan( - (uint[] Array, uint Value, int ExpectedIndex) c) - { - var span = new ReadOnlySpan(c.Array); - var index = span.BinarySearch(c.Value); - Assert.Equal(c.ExpectedIndex, index); + TestOverloads(c.Array, c.Value, c.ExpectedIndex); } [Theory, MemberData(nameof(DoubleCases))] - public static void BinarySearch_Double_Span( - (double[] Array, double Value, int ExpectedIndex) c) - { - var span = new Span(c.Array); - var index = span.BinarySearch(c.Value); - Assert.Equal(c.ExpectedIndex, index); - } - [Theory, MemberData(nameof(DoubleCases))] - public static void BinarySearch_Double_ReadOnlySpan( + public static void BinarySearch_Double( (double[] Array, double Value, int ExpectedIndex) c) { - var span = new ReadOnlySpan(c.Array); - var index = span.BinarySearch(c.Value); - Assert.Equal(c.ExpectedIndex, index); + TestOverloads(c.Array, c.Value, c.ExpectedIndex); } [Theory, MemberData(nameof(StringCases))] - public static void BinarySearch_String_Span( + public static void BinarySearch_String( (string[] Array, string Value, int ExpectedIndex) c) { - var span = new Span(c.Array); - var index = span.BinarySearch(c.Value); - Assert.Equal(c.ExpectedIndex, index); + TestOverloads(c.Array, c.Value, c.ExpectedIndex); } - [Theory, MemberData(nameof(StringCases))] - public static void BinarySearch_String_ReadOnlySpan( - (string[] Array, string Value, int ExpectedIndex) c) + + private static void TestOverloads( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable, T + { + TestSpan(array, value, expectedIndex); + TestReadOnlySpan(array, value, expectedIndex); + TestComparerSpan(array, value, expectedIndex); + TestComparerReadOnlySpan(array, value, expectedIndex); + } + + private static void TestSpan( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable + { + var span = new Span(array); + var index = span.BinarySearch(value); + Assert.Equal(expectedIndex, index); + } + private static void TestReadOnlySpan( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable + { + var span = new ReadOnlySpan(array); + var index = span.BinarySearch(value); + Assert.Equal(expectedIndex, index); + } + private static void TestComparerSpan( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable, T + { + var span = new Span(array); + var index = span.BinarySearch(value, Comparer.Default); + Assert.Equal(expectedIndex, index); + } + private static void TestComparerReadOnlySpan( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable, T { - var span = new ReadOnlySpan(c.Array); - var index = span.BinarySearch(c.Value); - Assert.Equal(c.ExpectedIndex, index); + var span = new ReadOnlySpan(array); + var index = span.BinarySearch(value, Comparer.Default); + Assert.Equal(expectedIndex, index); } } } From c68312ea5f449c455f86ecd3225d7bca7d9bda61 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 20:57:38 +0100 Subject: [PATCH 18/31] add icomparable overload tests. --- .../tests/ReadOnlySpan/BinarySearch.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 3725e320678c..f7d10ca9385f 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -69,6 +69,8 @@ private static void TestOverloads( { TestSpan(array, value, expectedIndex); TestReadOnlySpan(array, value, expectedIndex); + TestIComparableSpan(array, value, expectedIndex); + TestIComparableReadOnlySpan(array, value, expectedIndex); TestComparerSpan(array, value, expectedIndex); TestComparerReadOnlySpan(array, value, expectedIndex); } @@ -89,6 +91,22 @@ private static void TestReadOnlySpan( var index = span.BinarySearch(value); Assert.Equal(expectedIndex, index); } + private static void TestIComparableSpan( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable, T + { + var span = new Span(array); + var index = span.BinarySearch((IComparable)value); + Assert.Equal(expectedIndex, index); + } + private static void TestIComparableReadOnlySpan( + T[] array, TComparable value, int expectedIndex) + where TComparable : IComparable, T + { + var span = new ReadOnlySpan(array); + var index = span.BinarySearch((IComparable)value); + Assert.Equal(expectedIndex, index); + } private static void TestComparerSpan( T[] array, TComparable value, int expectedIndex) where TComparable : IComparable, T From 41ebc1ca1f9bbff9a66fe720cb37519cb622f00a Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 21:16:30 +0100 Subject: [PATCH 19/31] Add test for int.MaxValue size span and possible overflow. --- .../tests/ReadOnlySpan/BinarySearch.cs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index f7d10ca9385f..8878893a9355 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -62,6 +62,46 @@ public static void BinarySearch_String( { TestOverloads(c.Array, c.Value, c.ExpectedIndex); } + + [Fact] + // TODO: Does it need to be OuterLoop, it's pretty fast, compared to total time + //[OuterLoop] + // TODO: Do we need to be platform specific? + //[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX)] + public unsafe static void BinarySearch_MaxLength_NoOverflow() + { + if (sizeof(IntPtr) == sizeof(long)) + { + // Allocate max size span memory + var length = int.MaxValue; + IntPtr memory; + if (!AllocationHelper.TryAllocNative(new IntPtr(length), out memory)) + { + Console.WriteLine($"Span.BinarySearch test {nameof(BinarySearch_MaxLength_NoOverflow)} skipped (could not alloc memory)."); + return; + } + try + { + var span = new Span(memory.ToPointer(), length); + span.Fill(0); + // Fill end of span, so we can search for a value there, just at the end. + // But only to 254 so we can search for 255 just after end. + for (int i = 0; i < byte.MaxValue; i++) + { + var index = span.Length - (byte.MaxValue - i); + span[index] = (byte)i; + } + + Assert.Equal(int.MaxValue - 2, span.BinarySearch((byte)(byte.MaxValue - 2))); + Assert.Equal(int.MaxValue - 1, span.BinarySearch((byte)(byte.MaxValue - 1))); + Assert.Equal(int.MinValue, span.BinarySearch(byte.MaxValue)); + } + finally + { + AllocationHelper.ReleaseNative(ref memory); + } + } + } private static void TestOverloads( T[] array, TComparable value, int expectedIndex) From 42c5deca307b6d8b2f312694c94b79b8fc319a93 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 21:19:14 +0100 Subject: [PATCH 20/31] compute median without subtract --- src/System.Memory/src/System/SpanHelpers.BinarySearch.cs | 5 +++-- src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index c858ac3536e9..68b472e066e1 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -34,8 +34,9 @@ internal static int BinarySearch( // TODO: Test/investigate if below is faster (if it gives better asm), perhaps via Unsafe.As to avoid unnecessary conversions // This is safe since we know span.Length < int.MaxValue, and indeces are >= 0 // and thus cannot overflow an uint. Saves on subtraction per loop. - // int i = (int)(((uint)hi + (uint)lo) >> 1) - int i = lo + ((hi - lo) >> 1); + int i = (int)(((uint)hi + (uint)lo) >> 1); + // Below was intended to avoid overflows, but this cannot happen if we do the computation in uint + //int i = lo + ((hi - lo) >> 1); // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, // if this will be available in language diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 8878893a9355..1cc67ffd5485 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -74,8 +74,7 @@ public unsafe static void BinarySearch_MaxLength_NoOverflow() { // Allocate max size span memory var length = int.MaxValue; - IntPtr memory; - if (!AllocationHelper.TryAllocNative(new IntPtr(length), out memory)) + if (!AllocationHelper.TryAllocNative(new IntPtr(length), out IntPtr memory)) { Console.WriteLine($"Span.BinarySearch test {nameof(BinarySearch_MaxLength_NoOverflow)} skipped (could not alloc memory)."); return; From 1e3b9691beebcc00d7751992f46efa5aecc3ce66 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 7 Dec 2017 21:41:48 +0100 Subject: [PATCH 21/31] make generic params 'in' --- src/System.Memory/ref/System.Memory.cs | 8 ++++---- src/System.Memory/src/System/MemoryExtensions.cs | 8 ++++---- src/System.Memory/src/System/SpanHelpers.BinarySearch.cs | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/System.Memory/ref/System.Memory.cs b/src/System.Memory/ref/System.Memory.cs index fa30bdee44d4..53d41d6d74cb 100644 --- a/src/System.Memory/ref/System.Memory.cs +++ b/src/System.Memory/ref/System.Memory.cs @@ -152,11 +152,11 @@ public static class MemoryExtensions public static bool Overlaps(this ReadOnlySpan first, ReadOnlySpan second, out int elementOffset) { throw null; } public static int BinarySearch(this ReadOnlySpan span, IComparable comparable) { throw null; } - public static int BinarySearch(this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { throw null; } - public static int BinarySearch(this ReadOnlySpan span, T value, TComparer comparer) where TComparer : IComparer { throw null; } + public static int BinarySearch(this ReadOnlySpan span, in TComparable comparable) where TComparable : IComparable { throw null; } + public static int BinarySearch(this ReadOnlySpan span, in T value, in TComparer comparer) where TComparer : IComparer { throw null; } public static int BinarySearch(this Span span, IComparable comparable) { throw null; } - public static int BinarySearch(this Span span, TComparable comparable) where TComparable : IComparable { throw null; } - public static int BinarySearch(this Span span, T value, TComparer comparer) where TComparer : IComparer { throw null; } + public static int BinarySearch(this Span span, in TComparable comparable) where TComparable : IComparable { throw null; } + public static int BinarySearch(this Span span, in T value, in TComparer comparer) where TComparer : IComparer { throw null; } } public readonly struct ReadOnlyMemory diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index e7d4ef4e8b01..c39f75c57ed9 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -675,7 +675,7 @@ public static int BinarySearch( /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this Span span, TComparable comparable) + this Span span, in TComparable comparable) where TComparable : IComparable { return BinarySearch((ReadOnlySpan)span, comparable); @@ -697,7 +697,7 @@ public static int BinarySearch( /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this Span span, T value, TComparer comparer) + this Span span, in T value, in TComparer comparer) where TComparer : IComparer { return BinarySearch((ReadOnlySpan)span, value, comparer); @@ -738,7 +738,7 @@ public static int BinarySearch( /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this ReadOnlySpan span, TComparable comparable) + this ReadOnlySpan span, in TComparable comparable) where TComparable : IComparable { return SpanHelpers.BinarySearch(span, comparable); @@ -760,7 +760,7 @@ public static int BinarySearch( /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this ReadOnlySpan span, T value, TComparer comparer) + this ReadOnlySpan span, in T value, in TComparer comparer) where TComparer : IComparer { var comparable = new SpanHelpers.ComparerComparable( diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index 68b472e066e1..45f75dd71926 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -11,7 +11,7 @@ internal static partial class SpanHelpers { [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static int BinarySearch( - this ReadOnlySpan span, TComparable comparable) + this ReadOnlySpan span, in TComparable comparable) where TComparable : IComparable { // TODO: Make `ref readonly`/`in` when language permits @@ -21,7 +21,7 @@ internal static int BinarySearch( // TODO: Make s `ref readonly`/`in` when language permits // TODO: Make comparable `ref readonly`/`in` to allow pass by ref without forcing ref internal static int BinarySearch( - ref T s, int length, TComparable comparable) + ref T s, int length, in TComparable comparable) where TComparable : IComparable { // Array.BinarySearch implementation: From 74f0496324a7f3776760cf1bbaed1922a69e2c38 Mon Sep 17 00:00:00 2001 From: ntr Date: Fri, 8 Dec 2017 11:45:22 +0100 Subject: [PATCH 22/31] Remove 'in' and update xml comments. --- src/System.Memory/ref/System.Memory.cs | 8 +- .../src/System/MemoryExtensions.cs | 124 +++++++++++------- .../src/System/SpanHelpers.BinarySearch.cs | 15 ++- src/System.Memory/src/System/ThrowHelper.cs | 3 +- 4 files changed, 93 insertions(+), 57 deletions(-) diff --git a/src/System.Memory/ref/System.Memory.cs b/src/System.Memory/ref/System.Memory.cs index 53d41d6d74cb..fa30bdee44d4 100644 --- a/src/System.Memory/ref/System.Memory.cs +++ b/src/System.Memory/ref/System.Memory.cs @@ -152,11 +152,11 @@ public static class MemoryExtensions public static bool Overlaps(this ReadOnlySpan first, ReadOnlySpan second, out int elementOffset) { throw null; } public static int BinarySearch(this ReadOnlySpan span, IComparable comparable) { throw null; } - public static int BinarySearch(this ReadOnlySpan span, in TComparable comparable) where TComparable : IComparable { throw null; } - public static int BinarySearch(this ReadOnlySpan span, in T value, in TComparer comparer) where TComparer : IComparer { throw null; } + public static int BinarySearch(this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { throw null; } + public static int BinarySearch(this ReadOnlySpan span, T value, TComparer comparer) where TComparer : IComparer { throw null; } public static int BinarySearch(this Span span, IComparable comparable) { throw null; } - public static int BinarySearch(this Span span, in TComparable comparable) where TComparable : IComparable { throw null; } - public static int BinarySearch(this Span span, in T value, in TComparer comparer) where TComparer : IComparer { throw null; } + public static int BinarySearch(this Span span, TComparable comparable) where TComparable : IComparable { throw null; } + public static int BinarySearch(this Span span, T value, TComparer comparer) where TComparer : IComparer { throw null; } } public readonly struct ReadOnlyMemory diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index c39f75c57ed9..a1a0c774c2b0 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -640,19 +640,22 @@ ref first.DangerousGetPinnableReference(), } } - // TODO: XML doc /// - /// Searches the entire sorted for an element - /// using the default comparer and returns the zero-based index of the element. + /// Searches an entire sorted for a value + /// using the specified generic interface. /// - /// The sorted span to search in. - /// The object to locate as an Comparable. + /// The element type of the span. + /// The sorted to search. + /// The to use when comparing. /// - /// The zero-based index of in the sorted , + /// The zero-based index of in the sorted , /// if is found; otherwise, a negative number that is the bitwise complement /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// + /// + /// is . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this Span span, IComparable comparable) @@ -660,62 +663,79 @@ public static int BinarySearch( return BinarySearch>(span, comparable); } - // TODO: XML doc /// - /// Searches the entire sorted for an element - /// using the default comparer and returns the zero-based index of the element. + /// Searches an entire sorted for a value + /// using the specified generic type. /// - /// The sorted span to search in. - /// The object to locate. The value can be null for reference types. + /// The element type of the span. + /// The specific type of . + /// The sorted to search. + /// The to use when comparing. /// - /// The zero-based index of in the sorted , + /// The zero-based index of in the sorted , /// if is found; otherwise, a negative number that is the bitwise complement /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// + /// + /// is . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this Span span, in TComparable comparable) + this Span span, TComparable comparable) where TComparable : IComparable { return BinarySearch((ReadOnlySpan)span, comparable); } - // TODO: XML doc /// - /// Searches the entire sorted for an element - /// using the default comparer and returns the zero-based index of the element. + /// Searches an entire sorted for the specified + /// using the specified generic type. /// - /// The sorted span to search in. + /// The element type of the span. + /// The specific type of . + /// The sorted to search. /// The object to locate. The value can be null for reference types. - /// The comparer to use for searching. - /// - /// The zero-based index of in the sorted , + /// The to use when comparing. + /// /// + /// The zero-based index of in the sorted , /// if is found; otherwise, a negative number that is the bitwise complement /// of the index of the next element that is larger than or, if there is /// no larger element, the bitwise complement of . /// + /// + /// is . + /// + // TODO: Do we accept a null comparer and then revert to T as IComparable if possible?? + // T:System.ArgumentException: + // comparer is null, and value is of a type that is not compatible with the elements + // of array. + // T:System.InvalidOperationException: + // comparer is null, and T does not implement the System.IComparable`1 generic interface [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this Span span, in T value, in TComparer comparer) + this Span span, T value, TComparer comparer) where TComparer : IComparer { return BinarySearch((ReadOnlySpan)span, value, comparer); } - // TODO: XML doc /// - /// Searches the entire sorted for an element - /// using the default comparer and returns the zero-based index of the element. + /// Searches an entire sorted for a value + /// using the specified generic interface. /// - /// The sorted span to search in. - /// The object to locate. The value can be null for reference types. + /// The element type of the span. + /// The sorted to search. + /// The to use when comparing. /// - /// The zero-based index of in the sorted , + /// The zero-based index of in the sorted , /// if is found; otherwise, a negative number that is the bitwise complement /// of the index of the next element that is larger than or, if there is - /// no larger element, the bitwise complement of . + /// no larger element, the bitwise complement of . /// + /// + /// is . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( this ReadOnlySpan span, IComparable comparable) @@ -723,46 +743,60 @@ public static int BinarySearch( return BinarySearch>(span, comparable); } - // TODO: XML doc /// - /// Searches the entire sorted for an element - /// using the default comparer and returns the zero-based index of the element. + /// Searches an entire sorted for a value + /// using the specified generic type. /// - /// The sorted span to search in. - /// The object to locate. The value can be null for reference types. + /// The element type of the span. + /// The specific type of . + /// The sorted to search. + /// The to use when comparing. /// - /// The zero-based index of in the sorted , + /// The zero-based index of in the sorted , /// if is found; otherwise, a negative number that is the bitwise complement /// of the index of the next element that is larger than or, if there is - /// no larger element, the bitwise complement of . + /// no larger element, the bitwise complement of . /// + /// + /// is . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this ReadOnlySpan span, in TComparable comparable) + this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { return SpanHelpers.BinarySearch(span, comparable); } - // TODO: XML doc /// - /// Searches the entire sorted for an element - /// using the default comparer and returns the zero-based index of the element. + /// Searches an entire sorted for the specified + /// using the specified generic type. /// - /// The sorted span to search in. + /// The element type of the span. + /// The specific type of . + /// The sorted to search. /// The object to locate. The value can be null for reference types. - /// The comparer to use for searching. - /// - /// The zero-based index of in the sorted , + /// The to use when comparing. + /// /// + /// The zero-based index of in the sorted , /// if is found; otherwise, a negative number that is the bitwise complement /// of the index of the next element that is larger than or, if there is - /// no larger element, the bitwise complement of . + /// no larger element, the bitwise complement of . /// + /// + /// is . + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public static int BinarySearch( - this ReadOnlySpan span, in T value, in TComparer comparer) + this ReadOnlySpan span, T value, TComparer comparer) where TComparer : IComparer { + // TODO: Do we accept a null comparer and then revert to T as IComparable if possible?? + // T:System.ArgumentException: + // comparer is null, and value is of a type that is not compatible with the elements + // of array. + // T:System.InvalidOperationException: + // comparer is null, and T does not implement the System.IComparable`1 generic interface var comparable = new SpanHelpers.ComparerComparable( value, comparer); return BinarySearch(span, comparable); diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index 45f75dd71926..775fcbf32895 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -11,17 +11,18 @@ internal static partial class SpanHelpers { [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static int BinarySearch( - this ReadOnlySpan span, in TComparable comparable) + this ReadOnlySpan span, TComparable comparable) where TComparable : IComparable { - // TODO: Make `ref readonly`/`in` when language permits + if (comparable == null) + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparable); + // TODO: Make `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it return BinarySearch(ref span.DangerousGetPinnableReference(), span.Length, comparable); } - // TODO: Make s `ref readonly`/`in` when language permits - // TODO: Make comparable `ref readonly`/`in` to allow pass by ref without forcing ref + // TODO: Make s `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it internal static int BinarySearch( - ref T s, int length, in TComparable comparable) + ref T s, int length, TComparable comparable) where TComparable : IComparable { // Array.BinarySearch implementation: @@ -33,14 +34,14 @@ internal static int BinarySearch( // lo or hi will never be negative inside the loop // TODO: Test/investigate if below is faster (if it gives better asm), perhaps via Unsafe.As to avoid unnecessary conversions // This is safe since we know span.Length < int.MaxValue, and indeces are >= 0 - // and thus cannot overflow an uint. Saves on subtraction per loop. + // and thus cannot overflow an uint. Saves one subtraction per loop. int i = (int)(((uint)hi + (uint)lo) >> 1); // Below was intended to avoid overflows, but this cannot happen if we do the computation in uint //int i = lo + ((hi - lo) >> 1); // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, // if this will be available in language - // TODO: Revise all Unsafe APIs for `readonly` applicability... + // TODO: Revise all Unsafe APIs for `ref readonly` applicability... int c = comparable.CompareTo(Unsafe.Add(ref s, i)); if (c == 0) { diff --git a/src/System.Memory/src/System/ThrowHelper.cs b/src/System.Memory/src/System/ThrowHelper.cs index 868c616a5d18..d893fa9e21f4 100644 --- a/src/System.Memory/src/System/ThrowHelper.cs +++ b/src/System.Memory/src/System/ThrowHelper.cs @@ -102,6 +102,7 @@ internal enum ExceptionArgument text, obj, ownedMemory, - pointer + pointer, + comparable } } From 5ce195e537b91fc7c904b971a1cec1af15e5f36e Mon Sep 17 00:00:00 2001 From: ntr Date: Fri, 8 Dec 2017 11:52:10 +0100 Subject: [PATCH 23/31] add a few more test --- .../tests/ReadOnlySpan/BinarySearch.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 1cc67ffd5485..d909c72030d3 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -12,6 +12,9 @@ public static partial class ReadOnlySpanTests public static TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> UIntCases => new TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> { (new uint[] { }, 0u, -1), + (new uint[] { 1u }, 0u, -1), + (new uint[] { 1u }, 1u, 0), + (new uint[] { 1u }, 2u, -2), (new uint[] { 1u, 2u, 4u, 5u }, 0u, -1), (new uint[] { 1u, 2u, 4u, 5u }, 1u, 0), (new uint[] { 1u, 2u, 4u, 5u }, 2u, 1), @@ -22,7 +25,10 @@ public static partial class ReadOnlySpanTests }; public static TheoryData<(double[] Array, double Value, int ExpectedIndex)> DoubleCases => new TheoryData<(double[] Array, double Value, int ExpectedIndex)> { - (new double[] { }, 0u, -1), + (new double[] { }, 0.0, -1), + (new double[] { 1.0 }, 0.0, -1), + (new double[] { 1.0 }, 1.0, 0), + (new double[] { 1.0 }, 2.0, -2), (new double[] { 1.0, 2.0, 4.0, 5u }, 0.0, -1), (new double[] { 1.0, 2.0, 4.0, 5u }, 1.0, 0), (new double[] { 1.0, 2.0, 4.0, 5u }, 2.0, 1), @@ -33,6 +39,10 @@ public static partial class ReadOnlySpanTests }; public static TheoryData<(string[] Array, string Value, int ExpectedIndex)> StringCases => new TheoryData<(string[] Array, string Value, int ExpectedIndex)> { + (new string[] { }, "a", -1), + (new string[] { "b" }, "a", -1), + (new string[] { "b" }, "b", 0), + (new string[] { "b" }, "c", -2), (new string[] { "b", "c", "e", "f" }, "a", -1), (new string[] { "b", "c", "e", "f" }, "b", 0), (new string[] { "b", "c", "e", "f" }, "c", 1), @@ -72,7 +82,7 @@ public unsafe static void BinarySearch_MaxLength_NoOverflow() { if (sizeof(IntPtr) == sizeof(long)) { - // Allocate max size span memory + // Allocate maximum length span native memory var length = int.MaxValue; if (!AllocationHelper.TryAllocNative(new IntPtr(length), out IntPtr memory)) { From 6d75e2a642dd3ea620a70d9e9daf5233ef477bbb Mon Sep 17 00:00:00 2001 From: ntr Date: Fri, 8 Dec 2017 17:15:25 +0100 Subject: [PATCH 24/31] add null throw tests --- .../src/System/MemoryExtensions.cs | 2 ++ .../src/System/SpanHelpers.BinarySearch.cs | 4 +-- src/System.Memory/src/System/ThrowHelper.cs | 3 ++- .../tests/ReadOnlySpan/BinarySearch.cs | 25 ++++++++++++++++--- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index a1a0c774c2b0..cda9c86d020e 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -791,6 +791,8 @@ public static int BinarySearch( this ReadOnlySpan span, T value, TComparer comparer) where TComparer : IComparer { + if (comparer == null) + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparer); // TODO: Do we accept a null comparer and then revert to T as IComparable if possible?? // T:System.ArgumentException: // comparer is null, and value is of a type that is not compatible with the elements diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index 775fcbf32895..e1298228a6d5 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -39,9 +39,7 @@ internal static int BinarySearch( // Below was intended to avoid overflows, but this cannot happen if we do the computation in uint //int i = lo + ((hi - lo) >> 1); - // TODO: We probably need to add `ref readonly`/`in` overloads or `AddReadOnly`to unsafe, - // if this will be available in language - // TODO: Revise all Unsafe APIs for `ref readonly` applicability... + // TODO: We probably need to add `ref readonly`/`in` methods e.g. `AddReadOnly` to unsafe, int c = comparable.CompareTo(Unsafe.Add(ref s, i)); if (c == 0) { diff --git a/src/System.Memory/src/System/ThrowHelper.cs b/src/System.Memory/src/System/ThrowHelper.cs index d893fa9e21f4..1bc66984a0b9 100644 --- a/src/System.Memory/src/System/ThrowHelper.cs +++ b/src/System.Memory/src/System/ThrowHelper.cs @@ -103,6 +103,7 @@ internal enum ExceptionArgument obj, ownedMemory, pointer, - comparable + comparable, + comparer } } diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index d909c72030d3..0f37d64cfb74 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -9,7 +9,7 @@ namespace System.SpanTests { public static partial class ReadOnlySpanTests { - public static TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> UIntCases => + public static readonly TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> UIntCases = new TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> { (new uint[] { }, 0u, -1), (new uint[] { 1u }, 0u, -1), @@ -23,7 +23,7 @@ public static partial class ReadOnlySpanTests (new uint[] { 1u, 2u, 4u, 5u }, 5u, 3), (new uint[] { 1u, 2u, 4u, 5u }, 6u, -5), }; - public static TheoryData<(double[] Array, double Value, int ExpectedIndex)> DoubleCases => + public static readonly TheoryData<(double[] Array, double Value, int ExpectedIndex)> DoubleCases = new TheoryData<(double[] Array, double Value, int ExpectedIndex)> { (new double[] { }, 0.0, -1), (new double[] { 1.0 }, 0.0, -1), @@ -37,7 +37,7 @@ public static partial class ReadOnlySpanTests (new double[] { 1.0, 2.0, 4.0, 5u }, 5.0, 3), (new double[] { 1.0, 2.0, 4.0, 5u }, 6.0, -5), }; - public static TheoryData<(string[] Array, string Value, int ExpectedIndex)> StringCases => + public static readonly TheoryData<(string[] Array, string Value, int ExpectedIndex)> StringCases = new TheoryData<(string[] Array, string Value, int ExpectedIndex)> { (new string[] { }, "a", -1), (new string[] { "b" }, "a", -1), @@ -72,7 +72,24 @@ public static void BinarySearch_String( { TestOverloads(c.Array, c.Value, c.ExpectedIndex); } - + + [Fact] + public static void BinarySearch_NullComparableThrows() + { + Assert.Throws(() => new Span(new int[] { }).BinarySearch(null)); + Assert.Throws(() => new ReadOnlySpan(new int[] { }).BinarySearch(null)); + Assert.Throws(() => new Span(new int[] { }).BinarySearch>(null)); + Assert.Throws(() => new ReadOnlySpan(new int[] { }).BinarySearch>(null)); + } + + // TODO: Revise whether this should actually throw + [Fact] + public static void BinarySearch_NullComparerThrows() + { + Assert.Throws(() => new Span(new int[] { }).BinarySearch>(0, null)); + Assert.Throws(() => new ReadOnlySpan(new int[] { }).BinarySearch>(0, null)); + } + [Fact] // TODO: Does it need to be OuterLoop, it's pretty fast, compared to total time //[OuterLoop] From 90f3525151310da1d1204db910eeed7fd999aecd Mon Sep 17 00:00:00 2001 From: ntr Date: Fri, 8 Dec 2017 17:18:37 +0100 Subject: [PATCH 25/31] simplify max length no overflow test --- .../tests/ReadOnlySpan/BinarySearch.cs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index 0f37d64cfb74..a8621f18e895 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -110,17 +110,14 @@ public unsafe static void BinarySearch_MaxLength_NoOverflow() { var span = new Span(memory.ToPointer(), length); span.Fill(0); - // Fill end of span, so we can search for a value there, just at the end. - // But only to 254 so we can search for 255 just after end. - for (int i = 0; i < byte.MaxValue; i++) - { - var index = span.Length - (byte.MaxValue - i); - span[index] = (byte)i; - } - - Assert.Equal(int.MaxValue - 2, span.BinarySearch((byte)(byte.MaxValue - 2))); - Assert.Equal(int.MaxValue - 1, span.BinarySearch((byte)(byte.MaxValue - 1))); - Assert.Equal(int.MinValue, span.BinarySearch(byte.MaxValue)); + // Fill last two elements + span[int.MaxValue - 2] = 2; + span[int.MaxValue - 1] = 3; + // Search at end and assert no overflow + Assert.Equal(~(int.MaxValue - 2), span.BinarySearch((byte)1)); + Assert.Equal(int.MaxValue - 2, span.BinarySearch((byte)2)); + Assert.Equal(int.MaxValue - 1, span.BinarySearch((byte)3)); + Assert.Equal(int.MinValue, span.BinarySearch((byte)4)); } finally { From 955bc4b37704688ea578e0a4fd58c5ee332678bc Mon Sep 17 00:00:00 2001 From: ntr Date: Sat, 9 Dec 2017 11:17:16 +0100 Subject: [PATCH 26/31] add basic perf tests. --- .../src/System/MemoryExtensions.cs | 1 - .../src/System/SpanHelpers.BinarySearch.cs | 27 ++-- .../Performance/Perf.Span.BinarySearch.cs | 145 +++++++++++++++++- 3 files changed, 155 insertions(+), 18 deletions(-) diff --git a/src/System.Memory/src/System/MemoryExtensions.cs b/src/System.Memory/src/System/MemoryExtensions.cs index cda9c86d020e..125fe252b30f 100644 --- a/src/System.Memory/src/System/MemoryExtensions.cs +++ b/src/System.Memory/src/System/MemoryExtensions.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System.Collections.Generic; -using System.Diagnostics; using System.Runtime.CompilerServices; namespace System diff --git a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs index e1298228a6d5..c2428490837d 100644 --- a/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs +++ b/src/System.Memory/src/System/SpanHelpers.BinarySearch.cs @@ -20,27 +20,26 @@ internal static int BinarySearch( return BinarySearch(ref span.DangerousGetPinnableReference(), span.Length, comparable); } - // TODO: Make s `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it + // TODO: Make spanStart `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it internal static int BinarySearch( - ref T s, int length, TComparable comparable) + ref T spanStart, int length, TComparable comparable) where TComparable : IComparable { - // Array.BinarySearch implementation: - // https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Array.cs#L802 int lo = 0; int hi = length - 1; + // If length == 0, hi == -1, and loop will not be entered while (lo <= hi) { - // lo or hi will never be negative inside the loop - // TODO: Test/investigate if below is faster (if it gives better asm), perhaps via Unsafe.As to avoid unnecessary conversions - // This is safe since we know span.Length < int.MaxValue, and indeces are >= 0 - // and thus cannot overflow an uint. Saves one subtraction per loop. + // PERF: `lo` or `hi` will never be negative inside the loop, + // so computing median using uints is safe since we know + // `length <= int.MaxValue`, and indices are >= 0 + // and thus cannot overflow an uint. + // Saves one subtraction per loop compared to + // `int i = lo + ((hi - lo) >> 1);` int i = (int)(((uint)hi + (uint)lo) >> 1); - // Below was intended to avoid overflows, but this cannot happen if we do the computation in uint - //int i = lo + ((hi - lo) >> 1); - // TODO: We probably need to add `ref readonly`/`in` methods e.g. `AddReadOnly` to unsafe, - int c = comparable.CompareTo(Unsafe.Add(ref s, i)); + // TODO: We probably need to add `ref readonly`/`in` methods e.g. `AddReadOnly` to unsafe + int c = comparable.CompareTo(Unsafe.Add(ref spanStart, i)); if (c == 0) { return i; @@ -54,6 +53,10 @@ internal static int BinarySearch( hi = i - 1; } } + // If none found, then a negative number that is the bitwise complement + // of the index of the next element that is larger than or, if there is + // no larger element, the bitwise complement of `length`, which + // is `lo` at this point. return ~lo; } diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs index 065d7d2679f0..b7c73fda9cbd 100644 --- a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -9,10 +9,145 @@ namespace System.Memory.Tests { public class Perf_Span_BinarySearch { - // Existing. - // List: - // https://github.com/dotnet/corefx/blob/157fff35c2427dd0ce5e79557d506e8e7947c8d4/src/System.Collections/tests/Performance/Perf.List.cs#L550 - // https://github.com/dotnet/corefx/blob/157fff35c2427dd0ce5e79557d506e8e7947c8d4/src/System.Collections/tests/Performance/Perf.List.cs#L577 - // TODO: What tests do we need here? What types (short, int, string ...)? Range? Scenarios? + private const int InnerCount = 100000; + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_Int_FirstIndex(int size) + { + BenchmarkAndAssert(size, 0, 0); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_Int_MiddleIndex(int size) + { + BenchmarkAndAssert(size, size / 2, size / 2); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_Int_LastIndex(int size) + { + BenchmarkAndAssert(size, size - 1, size - 1); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_Int_NotFoundBefore(int size) + { + BenchmarkAndAssert(size, -1, -1); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_Int_NotFoundAfter(int size) + { + BenchmarkAndAssert(size, size, ~size); + } + + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_String_FirstIndex(int size) + { + BenchmarkAndAssert(size, 0.ToString(), 0); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_String_MiddleIndex(int size) + { + BenchmarkAndAssert(size, (size / 2).ToString(), size / 2); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_String_LastIndex(int size) + { + BenchmarkAndAssert(size, (size - 1).ToString(), size - 1); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_String_NotFoundBefore(int size) + { + BenchmarkAndAssert(size, (-1).ToString(), -1); + } + + [Benchmark(InnerIterationCount = InnerCount)] + [InlineData(1)] + [InlineData(10)] + [InlineData(100)] + [InlineData(1000)] + public void SpanBinarySearch_String_NotFoundAfter(int size) + { + BenchmarkAndAssert(size, (size).ToString(), ~size); + } + + private static void BenchmarkAndAssert(int size, int value, int expectedIndex) + { + Span span = new int[size]; + for (int i = 0; i < span.Length; i++) + { + span[i] = i; + } + + int index = 0; + foreach (BenchmarkIteration iteration in Benchmark.Iterations) + { + using (iteration.StartMeasurement()) + { + index |= span.BinarySearch(value); + } + } + Assert.Equal(expectedIndex, index); + } + + private static void BenchmarkAndAssert(int size, string value, int expectedIndex) + { + Span span = new string[size]; + for (int i = 0; i < span.Length; i++) + { + span[i] = i.ToString(); + } + + int index = 0; + foreach (BenchmarkIteration iteration in Benchmark.Iterations) + { + using (iteration.StartMeasurement()) + { + index |= span.BinarySearch(value); + } + } + Assert.Equal(expectedIndex, index); + } } } From ee9830fabd6f901d4505153e95586fe725826562 Mon Sep 17 00:00:00 2001 From: ntr Date: Sat, 9 Dec 2017 11:29:24 +0100 Subject: [PATCH 27/31] fix remaining review issues. --- .../tests/ReadOnlySpan/BinarySearch.cs | 66 ++++++++++++------- .../tests/System.Memory.Tests.csproj | 4 +- 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs index a8621f18e895..5e066133e6d1 100644 --- a/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs +++ b/src/System.Memory/tests/ReadOnlySpan/BinarySearch.cs @@ -9,7 +9,7 @@ namespace System.SpanTests { public static partial class ReadOnlySpanTests { - public static readonly TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> UIntCases = + public static readonly TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> s_casesUInt = new TheoryData<(uint[] Array, uint Value, int ExpectedIndex)> { (new uint[] { }, 0u, -1), (new uint[] { 1u }, 0u, -1), @@ -22,22 +22,24 @@ public static partial class ReadOnlySpanTests (new uint[] { 1u, 2u, 4u, 5u }, 4u, 2), (new uint[] { 1u, 2u, 4u, 5u }, 5u, 3), (new uint[] { 1u, 2u, 4u, 5u }, 6u, -5), + (new uint[] { 1u, 2u, 2u, 2u }, 2u, 1), }; - public static readonly TheoryData<(double[] Array, double Value, int ExpectedIndex)> DoubleCases = + public static readonly TheoryData<(double[] Array, double Value, int ExpectedIndex)> s_casesDouble = new TheoryData<(double[] Array, double Value, int ExpectedIndex)> { (new double[] { }, 0.0, -1), (new double[] { 1.0 }, 0.0, -1), (new double[] { 1.0 }, 1.0, 0), (new double[] { 1.0 }, 2.0, -2), - (new double[] { 1.0, 2.0, 4.0, 5u }, 0.0, -1), - (new double[] { 1.0, 2.0, 4.0, 5u }, 1.0, 0), - (new double[] { 1.0, 2.0, 4.0, 5u }, 2.0, 1), - (new double[] { 1.0, 2.0, 4.0, 5u }, 3.0, -3), - (new double[] { 1.0, 2.0, 4.0, 5u }, 4.0, 2), - (new double[] { 1.0, 2.0, 4.0, 5u }, 5.0, 3), - (new double[] { 1.0, 2.0, 4.0, 5u }, 6.0, -5), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 0.0, -1), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 1.0, 0), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 2.0, 1), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 3.0, -3), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 4.0, 2), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 5.0, 3), + (new double[] { 1.0, 2.0, 4.0, 5.0 }, 6.0, -5), + (new double[] { 2.0, 2.0, 2.0, 1.0 }, 2.0, 1), }; - public static readonly TheoryData<(string[] Array, string Value, int ExpectedIndex)> StringCases = + public static readonly TheoryData<(string[] Array, string Value, int ExpectedIndex)> s_casesString = new TheoryData<(string[] Array, string Value, int ExpectedIndex)> { (new string[] { }, "a", -1), (new string[] { "b" }, "a", -1), @@ -50,27 +52,41 @@ public static partial class ReadOnlySpanTests (new string[] { "b", "c", "e", "f" }, "e", 2), (new string[] { "b", "c", "e", "f" }, "f", 3), (new string[] { "b", "c", "e", "f" }, "g", -5), + (new string[] { "b", "b", "c", "c" }, "c", 2), }; - [Theory, MemberData(nameof(UIntCases))] + [Theory, MemberData(nameof(s_casesUInt))] public static void BinarySearch_UInt( - (uint[] Array, uint Value, int ExpectedIndex) c) + (uint[] Array, uint Value, int ExpectedIndex) testCase) { - TestOverloads(c.Array, c.Value, c.ExpectedIndex); + TestOverloads(testCase.Array, testCase.Value, testCase.ExpectedIndex); } - [Theory, MemberData(nameof(DoubleCases))] + [Theory, MemberData(nameof(s_casesDouble))] public static void BinarySearch_Double( - (double[] Array, double Value, int ExpectedIndex) c) + (double[] Array, double Value, int ExpectedIndex) testCase) { - TestOverloads(c.Array, c.Value, c.ExpectedIndex); + TestOverloads(testCase.Array, testCase.Value, testCase.ExpectedIndex); } - [Theory, MemberData(nameof(StringCases))] + [Theory, MemberData(nameof(s_casesString))] public static void BinarySearch_String( - (string[] Array, string Value, int ExpectedIndex) c) + (string[] Array, string Value, int ExpectedIndex) testCase) { - TestOverloads(c.Array, c.Value, c.ExpectedIndex); + TestOverloads(testCase.Array, testCase.Value, testCase.ExpectedIndex); + } + + [Fact] + public static void BinarySearch_Slice() + { + var array = new int[] { 1, 2, 3, 4, 5, 6, 7, 8, 9 }; + var span = new ReadOnlySpan(array, 1, array.Length - 2); + + Assert.Equal(-1, span.BinarySearch(1)); + Assert.Equal(0, span.BinarySearch(2)); + Assert.Equal(3, span.BinarySearch(5)); + Assert.Equal(6, span.BinarySearch(8)); + Assert.Equal(-8, span.BinarySearch(9)); } [Fact] @@ -90,11 +106,13 @@ public static void BinarySearch_NullComparerThrows() Assert.Throws(() => new ReadOnlySpan(new int[] { }).BinarySearch>(0, null)); } + // NOTE: BinarySearch_MaxLength_NoOverflow test is constrained to run on Windows and MacOSX because it causes + // problems on Linux due to the way deferred memory allocation works. On Linux, the allocation can + // succeed even if there is not enough memory but then the test may get killed by the OOM killer at the + // time the memory is accessed which triggers the full memory allocation. [Fact] - // TODO: Does it need to be OuterLoop, it's pretty fast, compared to total time - //[OuterLoop] - // TODO: Do we need to be platform specific? - //[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX)] + [OuterLoop] + [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX)] public unsafe static void BinarySearch_MaxLength_NoOverflow() { if (sizeof(IntPtr) == sizeof(long)) @@ -113,6 +131,8 @@ public unsafe static void BinarySearch_MaxLength_NoOverflow() // Fill last two elements span[int.MaxValue - 2] = 2; span[int.MaxValue - 1] = 3; + + Assert.Equal(int.MaxValue / 2, span.BinarySearch((byte)0)); // Search at end and assert no overflow Assert.Equal(~(int.MaxValue - 2), span.BinarySearch((byte)1)); Assert.Equal(int.MaxValue - 2, span.BinarySearch((byte)2)); diff --git a/src/System.Memory/tests/System.Memory.Tests.csproj b/src/System.Memory/tests/System.Memory.Tests.csproj index f754cba96ab3..a936afd7f768 100644 --- a/src/System.Memory/tests/System.Memory.Tests.csproj +++ b/src/System.Memory/tests/System.Memory.Tests.csproj @@ -9,7 +9,6 @@ - @@ -58,6 +57,7 @@ + @@ -170,4 +170,4 @@ - \ No newline at end of file + From cedb211c6b8ad48740caddd559f08a40e191651c Mon Sep 17 00:00:00 2001 From: ntr Date: Mon, 11 Dec 2017 21:09:56 +0100 Subject: [PATCH 28/31] use InnerCount --- .../tests/Performance/Perf.Span.BinarySearch.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs index b7c73fda9cbd..bb4b3ba95d48 100644 --- a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -125,7 +125,10 @@ private static void BenchmarkAndAssert(int size, int value, int expectedIndex) { using (iteration.StartMeasurement()) { - index |= span.BinarySearch(value); + for (int i = 0; i < Benchmark.InnerIterationCount; i++) + { + index |= span.BinarySearch(value); + } } } Assert.Equal(expectedIndex, index); From 0b3cceca8cbce90f03a475c489b3d78da607ec71 Mon Sep 17 00:00:00 2001 From: ntr Date: Mon, 11 Dec 2017 21:14:27 +0100 Subject: [PATCH 29/31] use InnerCount --- .../tests/Performance/Perf.Span.BinarySearch.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs index bb4b3ba95d48..3cf2e7e88a7e 100644 --- a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -147,7 +147,10 @@ private static void BenchmarkAndAssert(int size, string value, int expectedIndex { using (iteration.StartMeasurement()) { - index |= span.BinarySearch(value); + for (int i = 0; i < Benchmark.InnerIterationCount; i++) + { + index |= span.BinarySearch(value); + } } } Assert.Equal(expectedIndex, index); From dc609083ba7d5a93145a752ef1f386d100570d92 Mon Sep 17 00:00:00 2001 From: ntr Date: Wed, 13 Dec 2017 11:12:29 +0100 Subject: [PATCH 30/31] fix string perf test --- .../tests/Performance/Perf.Span.BinarySearch.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs index 3cf2e7e88a7e..7e91816949f8 100644 --- a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -10,6 +10,7 @@ namespace System.Memory.Tests public class Perf_Span_BinarySearch { private const int InnerCount = 100000; + private const string NumberFormat = "D9"; [Benchmark(InnerIterationCount = InnerCount)] [InlineData(1)] @@ -69,7 +70,7 @@ public void SpanBinarySearch_Int_NotFoundAfter(int size) [InlineData(1000)] public void SpanBinarySearch_String_FirstIndex(int size) { - BenchmarkAndAssert(size, 0.ToString(), 0); + BenchmarkAndAssert(size, 0.ToString(NumberFormat), 0); } [Benchmark(InnerIterationCount = InnerCount)] @@ -79,7 +80,7 @@ public void SpanBinarySearch_String_FirstIndex(int size) [InlineData(1000)] public void SpanBinarySearch_String_MiddleIndex(int size) { - BenchmarkAndAssert(size, (size / 2).ToString(), size / 2); + BenchmarkAndAssert(size, (size / 2).ToString(NumberFormat), size / 2); } [Benchmark(InnerIterationCount = InnerCount)] @@ -89,7 +90,7 @@ public void SpanBinarySearch_String_MiddleIndex(int size) [InlineData(1000)] public void SpanBinarySearch_String_LastIndex(int size) { - BenchmarkAndAssert(size, (size - 1).ToString(), size - 1); + BenchmarkAndAssert(size, (size - 1).ToString(NumberFormat), size - 1); } [Benchmark(InnerIterationCount = InnerCount)] @@ -99,7 +100,8 @@ public void SpanBinarySearch_String_LastIndex(int size) [InlineData(1000)] public void SpanBinarySearch_String_NotFoundBefore(int size) { - BenchmarkAndAssert(size, (-1).ToString(), -1); + // "/" is just before zero in character table + BenchmarkAndAssert(size, "/", -1); } [Benchmark(InnerIterationCount = InnerCount)] @@ -109,7 +111,7 @@ public void SpanBinarySearch_String_NotFoundBefore(int size) [InlineData(1000)] public void SpanBinarySearch_String_NotFoundAfter(int size) { - BenchmarkAndAssert(size, (size).ToString(), ~size); + BenchmarkAndAssert(size, (size).ToString(NumberFormat), ~size); } private static void BenchmarkAndAssert(int size, int value, int expectedIndex) @@ -139,7 +141,7 @@ private static void BenchmarkAndAssert(int size, string value, int expectedIndex Span span = new string[size]; for (int i = 0; i < span.Length; i++) { - span[i] = i.ToString(); + span[i] = i.ToString(NumberFormat); } int index = 0; From 67a5965e0efea4b122bd4c4f8c31e38b41c94eb0 Mon Sep 17 00:00:00 2001 From: ntr Date: Wed, 13 Dec 2017 11:29:36 +0100 Subject: [PATCH 31/31] fix MiddleIndex perf tests --- src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs index 7e91816949f8..4700f3e8fc6f 100644 --- a/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs +++ b/src/System.Memory/tests/Performance/Perf.Span.BinarySearch.cs @@ -29,7 +29,7 @@ public void SpanBinarySearch_Int_FirstIndex(int size) [InlineData(1000)] public void SpanBinarySearch_Int_MiddleIndex(int size) { - BenchmarkAndAssert(size, size / 2, size / 2); + BenchmarkAndAssert(size, (size - 1) / 2, (size - 1) / 2); } [Benchmark(InnerIterationCount = InnerCount)] @@ -80,7 +80,7 @@ public void SpanBinarySearch_String_FirstIndex(int size) [InlineData(1000)] public void SpanBinarySearch_String_MiddleIndex(int size) { - BenchmarkAndAssert(size, (size / 2).ToString(NumberFormat), size / 2); + BenchmarkAndAssert(size, ((size - 1) / 2).ToString(NumberFormat), (size - 1) / 2); } [Benchmark(InnerIterationCount = InnerCount)]