Span: Add BinarySearch(...) extension methods for ReadOnlySpan<T> (and Span<T>) #25777
Conversation
| // 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)); |
There was a problem hiding this comment.
Does this really need to use unsafe code?
There was a problem hiding this comment.
@jkotas could use indexer, but this way the code is more general (only needs a ref) and bounds checking won't be an issue. Perhaps the jit can elide the bounds checks but not sure if it could given the pattern here.
| } | ||
| catch (Exception e) | ||
| { | ||
| // TODO: Is this correct? Don't like the try/catch, why should we have this?? |
There was a problem hiding this comment.
I do not think that this try/catch is needed.
IIRC, tt was in Sort as well and it was removed there. I think it can be removed in BinarySearch as well (even in the array-based BinarySearch).
There was a problem hiding this comment.
that would be great, it would simplify the PR greatly, I'll remove it.
|
@mmitche my new Alpine 3.6 CI leg has failed here with error: |
|
@janvorli This branch has conflicts so it's building the unmerged code rather than the merged code. |
|
|
||
| [Theory, MemberData(nameof(UIntCases))] | ||
| public static void BinarySearch_UInt( | ||
| (uint[] Array, uint Value, int ExpectedIndex) c) |
There was a problem hiding this comment.
nit: rename c to something more descriptive, maybe testData
| @@ -9,6 +9,7 @@ | |||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netstandard-Release|AnyCPU'" /> | |||
| <ItemGroup> | |||
| <Compile Include="AllocationHelper.cs" /> | |||
| <Compile Include="ReadOnlySpan\BinarySearch.cs" /> | |||
There was a problem hiding this comment.
nit: can you please move this into the ReadOnlySpan test property group?
Also consider splitting the tests into separate files for Span and ReadOnlySpan tests, but up to you.
|
@ahsonkhan thanks for the great feedback. I think I have addressed everything except splitting unit tests into two files since I think this will lead to more code duplication, if you have a way that doesn't let me know. Also did not change value tuple property names, since not sure correct, and not specifically addressed in the coding style (PS: Consider adding to code style.). |
nietras
changed the title
| { | ||
| using (iteration.StartMeasurement()) | ||
| { | ||
| index |= span.BinarySearch(value); |
There was a problem hiding this comment.
You aren't using InnerIterationCount here (or elsewhere). If it isn't needed (i.e. your test results are already > 1 ms), feel free to remove it. Otherwise, I think you need to add a loop within the measured block, using the InnerIterationCount.
See the following for an example - https://github.com/Microsoft/xunit-performance#authoring-benchmarks
As an aside, can you please share the results of the perf tests? I am curious to see them.
There was a problem hiding this comment.
@ahsonkhan fixed to use InnerIterationCount, however, testResults.xml does not contain any results for these, maybe I am not executing it correctly:
<?xml version="1.0" encoding="utf-8"?>
<assemblies>
<assembly name="System.Memory.Performance.Tests.dll" environment="64-bit .NET (unknown version) [collection-per-class, parallel (4 threads)]" test-framework="xUnit.net 2.2.0.3300" run-date="2017-12-11" run-time="21:08:22" total="2" passed="2" failed="0" skipped="0" time="0.221" errors="0">
<errors />
<collection total="2" passed="2" failed="0" skipped="0" name="Test collection for System.Buffers.Text.Tests.Base64TestHelper" time="0.014">
<test name="System.Buffers.Text.Tests.Base64TestHelper.GenerateEncodingMapAndVerify" type="System.Buffers.Text.Tests.Base64TestHelper" method="GenerateEncodingMapAndVerify" time="0.0112869" result="Pass" />
<test name="System.Buffers.Text.Tests.Base64TestHelper.GenerateDecodingMapAndVerify" type="System.Buffers.Text.Tests.Base64TestHelper" method="GenerateDecodingMapAndVerify" time="0.0028464" result="Pass" />
</collection>
</assembly>
</assemblies>There was a problem hiding this comment.
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md does not seem to be correct, at least running:
msbuild /t:BuildAndTest /p:Performance=true /p:ConfigurationGroup=Release /p:TargetOS=Windows_NT
does not work. What am I doing wrong? 🙄
There was a problem hiding this comment.
The performance results should be somewhere in this directory: "corefx\bin\tests\System.Memory.Performance.Tests\netcoreapp-Windows_NT-Release-x64", not in testResults.xml
Go to "\corefx\src\System.Memory\tests\Performance" and run the command you listed above:
msbuild /t:BuildAndTest /p:Performance=true /p:ConfigurationGroup=Release /p:TargetOS=Windows_NT
Are you getting any errors when you do this?
There was a problem hiding this comment.
@ahsonkhan no I do not get any errors when running the msbuild command, but no tests are run.
There was a problem hiding this comment.
@ahsonkhan didn't notice this at first but the msbuild run says:
Skipping target "RunTestsForProject" because all output files are up-to-date with respect to the input files.
I thought that weird, and since the testResults.xml file was incorrect, only had two tests, that actually do not seem to be proper perf tests, I deleted the file and ran the msbuild command again. Then the tests are run and I noted that:
[13-12-2017 10:24:05][WRN] Skipping 2 Xunit [Fact]s because they are not [Benchmark]s
Therefore I think there are two tests that are wrongly marked as Benchmark or something which mess the whole process up.
There is an issue with the string perf tests, so results will have to come later... I think I made a mistake when doing the int to string stuff:
[13-12-2017 10:27:55][ERR] System.Memory.Tests.Perf_Span_BinarySearch.SpanBinarySearch_String_NotFoundBefore(size: 100): Assert.Equal() Failure
Expected: -1
Actual: -3
[13-12-2017 10:27:55][ERR] at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in C:\BuildAgent\work\cb37e9acf085d108\src\xunit.assert\Asserts\EqualityAsserts.cs:line 35
at System.Memory.Tests.Perf_Span_BinarySearch.BenchmarkAndAssert(Int32 size, String value, Int32 expectedIndex) in D:\oss\corefx\src\System.Memory\tests\Performance\Perf.Span.BinarySearch.cs:line 157
at System.Memory.Tests.Perf_Span_BinarySearch.SpanBinarySearch_String_NotFoundBefore(Int32 size) in D:\oss\corefx\src\System.Memory\tests\Performance\Perf.Span.BinarySearch.cs:line 103
There was a problem hiding this comment.
Followup, weird the actual perf test run does not produce a testResults.xml file at the end, maybe an intermediate? Anyway, seems I can run the perf tests after fixing that.
Regarding the possible wrong tests, these are defined as:
[Fact]
public static void GenerateEncodingMapAndVerify()
{
var data = new byte[64]; // Base64
for (int i = 0; i < s_characters.Length; i++)
{
data[i] = (byte)s_characters[i];
}
Assert.True(s_encodingMap.AsSpan().SequenceEqual(data));
}
[Fact]
public static void GenerateDecodingMapAndVerify()
{
var data = new sbyte[256]; // 0 to byte.MaxValue (255)
for (int i = 0; i < data.Length; i++)
{
data[i] = s_invalidByte;
}
for (int i = 0; i < s_characters.Length; i++)
{
data[s_characters[i]] = (sbyte)i;
}
Assert.True(s_decodingMap.AsSpan().SequenceEqual(data));
}these are the ones populating the testResults.xml. These are static methods on a static class, perhaps there is an issue around that here.
|
After fixing the string perf test these are the results I am getting. This shows that I made a mistake there with how I defined the
so I fixed that and reran the perf tests yielding:
|
|
@ahsonkhan except for the value tuple PascalCase or camelCase issue, I think this is ready now. |
|
Ok from what I gather the issue with perf tests is that there have been added to The question is whether there should be normal unit tests in a performance tests project or if this is an issue with the build tools that should figure out that running unit tests in a performance tests project does not mean performance tests have been run etc. Not sure what. And perhaps a different issue should be filed for this. |
| @@ -10,6 +10,7 @@ namespace System.Memory.Tests | |||
| public class Perf_Span_BinarySearch | |||
| { | |||
| private const int InnerCount = 100000; | |||
| private const string NumberFormat = "D9"; | |||
There was a problem hiding this comment.
Can you please explain why this is needed?
I am not exactly sure if that is the issue since I am not able to reproduce the behavior you see yet, but we could consider moving the GenerateEncodingMapAndVerify / GenerateDecodingMapAndVerify tests out of Base64TestHelpers and into Base64DecoderUnitTests.cs / Base64EncoderUnitTests.cs. Can you please try removing those tests locally and see if it fixed the issue of running performance tests you were seeing to confirm that is the cause? If it is the issue, I can file an issue to fix it (feel free to do it yourself as well). |
|
How come SpanBinarySearch_String_NotFoundBefore is faster than SpanBinarySearch_String_NotFoundAfter? |
|
One thing that would be really helpful, if you have time, is to add Array.BinarySearch baseline performance tests so we can get comparison of the performance. Otherwise, looks good to me :) I am curious, why does the SpanBinarySearch_String_MiddleIndex performs so well? Is it because it only runs one iteration of the loop and the CompareTo already returns 0? int c = comparable.CompareTo(Unsafe.Add(ref spanStart, i)); // c = 0 right away, and we return i? |
| { | ||
| if (comparable == null) | ||
| ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparable); | ||
| // TODO: Make `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it |
There was a problem hiding this comment.
@ahsonkhan, @nietras , do we have workitems filed for these Unsafe APIs and to fix the TODOs?
There was a problem hiding this comment.
@KrzysztofCwalina My proposal to change Unsafe APIs to ref readonly was rejected a while ago. A few things have changed since then (such as the rename to in), so maybe it's time the proposal could be revisited?
There was a problem hiding this comment.
There was a problem hiding this comment.
I should probably just remove the TODOs since the Unsafe fix was to "cast" away the readonly and then there is no reason as such to change the impl, if DangerousGetPinnableReference() will still return a ref for ReadOnlySpan<T> otherwise the readonly simply has to be cast away. This will show up if any such change comes to ReadOnlySpan<T> so deleting the TODOs should be fine.
|
Looks good! Thanks for the great PR! |
To reproduce I run: Which then shows: After that I then run: which then says it skips the performance tests as previously mentioned. Removing the two tests aka uncommenting the two yields: And a new <?xml version="1.0" encoding="utf-8"?>
<assemblies>
<assembly>
<errors />
</assembly>
</assemblies>Then running: and it still skips the performance tests, so you are right that disabling/uncommenting the two |
@ahsonkhan the
String_NotFoundBefore uses a single character string
I will try to see if I can add this in a separate PR, perhaps with a few comments for some of the questions raised.
Yes, the MiddleIndex is then first index checked in the binary search so it returns on first compare no matter what. It is the same for the int perf tests.
Thanks I will see if I can make a PR for some of the remaining small nits. Otherwise, next up is |
Span: Add BinarySearch(...) extension methods for ReadOnlySpan<T> (and Span<T>) Commit migrated from dotnet/corefx@c362f30
Implements https://github.com/dotnet/corefx/issues/15818
This is currently an early work-in-progress with the current status:
try/catchin the implementation? Why is it there? REMOVED, per review comment.uintwith single add and shiftinon e.g.TComparer,TComparable. Commented on initial proposal to change this.insince it will always pass byrefwhich will not be good for small types.Array.BinarySearch? couldn't find them, copy from VS F12)coreclrhas a lot of "weird" tests, guidance on tests needed here would be good? Not using these... seem unnecessary)coreclrhas a lot of "weird" tests, guidance on tests needed here would be good? What scenarios? Also see code comments.)cc: @karelz a heads up, since I started preliminary work on this. Hopefully after this I can continue with
Sort.cc: @jkotas if you could review and perhaps answer some questions, thank you.