Corefxlab: Cleanup and move Utf8Parser tests to corefx

[ahsonkhan]

Move all the Utf8Parser performance tests to corefx https://github.com/dotnet/corefx/blob/8e53c7efea4efd28ba9d111dd4e70e3ca2f987f3/src/System.Memory/tests/Performance/Perf.Utf8Parser.cs#L11 and remove duplicate/unnecessary test cases in the process.

This includes the following performance tests:
https://github.com/dotnet/corefxlab/blob/89ea88c64f2dabe362c0a3df21d07313542c1d80/tests/System.Text.Primitives.Tests/Parsing/PrimitiveParserBoolPerfTests.cs#L11
https://github.com/dotnet/corefxlab/blob/89ea88c64f2dabe362c0a3df21d07313542c1d80/tests/System.Text.Primitives.Tests/Parsing/PrimitiveParserInt16PerfTests.cs#L10
https://github.com/dotnet/corefxlab/blob/89ea88c64f2dabe362c0a3df21d07313542c1d80/tests/System.Text.Primitives.Tests/Parsing/PrimitiveParserInt32PerfTests.cs#L11
https://github.com/dotnet/corefxlab/blob/89ea88c64f2dabe362c0a3df21d07313542c1d80/tests/System.Text.Primitives.Tests/Parsing/PrimitiveParserSBytePerfTests.cs#L11
https://github.com/dotnet/corefxlab/blob/89ea88c64f2dabe362c0a3df21d07313542c1d80/tests/System.Text.Primitives.Tests/Parsing/PrimitiveParserUInt32PerfTests.cs#L12
https://github.com/dotnet/corefxlab/blob/89ea88c64f2dabe362c0a3df21d07313542c1d80/tests/System.Text.Primitives.Tests/Parsing/PrimitiveParserUInt64PerfTests.cs#L12

[L-Dogg]

@ahsonkhan Could I work on this issue? It'd be my first one, so please correct me if I'm wrong, it requires creating one pull request to this repo and another to dotnet/corefx?

[ahsonkhan]

Could I work on this issue? It'd be my first one, so please correct me if I'm wrong, it requires creating one pull request to this repo and another to dotnet/corefx?

@L-Dogg, absolutely! Feel free to ask questions and I will help where needed.

I would start with creating a pull request in dotnet/corefx (where you add the performance tests there, and de-duplicate them, see https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Performance/Perf.Utf8Parser.cs). After that's merged, then yes, you would create a PR here, to remove these tests.

@karelz, can you help @L-Dogg with onboarding.

[L-Dogg]

It'd be great - I've just set up my environment (VS 2017 Community) according to the developer's guide and pulled the forked repo, but I couldn't get it built due to the error below:
```Restoring packages for C:\Users\Szymek\Desktop\corefx\Tools\ilasm\ilasm.depproj...

Retrying 'FindPackagesByIdAsync' for source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-core/nuget/v3/flatcontainer/microsoft.netcore.dotnethostresolver/index.json'.
An error occurred while sending the request.
Unable to read data from the transport connection:
A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
```
Anyway, right now I cannot run the tests - should I create an issue? I get this error while building corefxlab as well as corefx.

Welp, I've just opened corefxlab solution and tests were run properly using R# 👍 But the error still occurs and corefx's System.Memory solution is full of errors like Error CS0246 The type or namespace name 'Microsoft' could not be found (are you missing a using directive or an assembly reference?) System.Memory.Performance.Tests

[ahsonkhan]

Welp, I've just opened corefxlab solution and tests were run properly using R# 👍 But the error still occurs and corefx's System.Memory solution is full of errors like Error CS0246 The type or namespace name 'Microsoft' could not be found (are you missing a using directive or an assembly reference?) System.Memory.Performance.Tests

Try building System.Memory from the command line instead of from within VS and let me know if it succeeds.

[ahsonkhan]

See this guide on how to run perf tests:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md#building-and-running-tests

[L-Dogg]

In the Developer Command Prompt for VS 2017 I typed build System.Memory and got the same error:

Failed to retrieve information about 'Microsoft.NETCore.DotNetHostResolver' from remote source
'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-core/nuget/v3/flatcontainer/microsoft.netcore.dotnethostresolver/index.json'. [C:\Users\Szymek\Desktop\corefx\Tools\ilasm\ilasm.depproj]
Unable to read data from the transport connection: 
A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. [C:\Users\Szymek\Desktop\corefx\Tools\ilasm\ilasm.depproj]

[L-Dogg]

I read the instruction but I am still struggling to get a successful build.

[ahsonkhan]

1) Get a clean clone of the corefx repo.
2) Run build.cmd at the root of the repo (required to do once).
3) Then navigate to the System.Memory src directory and run msbuild (not build).

See the following guide for the proper developer workflow:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md

Let me know if you are still seeing build failures after following those steps because I haven't seen such errors before.

[L-Dogg]

I got a clean clone, then proceeded to step 2 and unfortunately Failed to retrieve information about 'Microsoft.NETCore.DotNetHostResolver' from remote source 'https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-core/nuget/v3/flatcontainer/microsoft.netcore.dotnethostresolver/index.json'. occured.

[dagood]

@L-Dogg Are you able to access

https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-core/nuget/v3/flatcontainer/microsoft.netcore.dotnethostresolver/index.json

directly in your web browser? It should list all the versions of the microsoft.netcore.dotnethostresolver package (like the MyGet Gallery page).

The error generally means NuGet wasn't able to access the URL. I'm wondering if this might be a proxy or georeplication issue.

[L-Dogg]

I forgot to mention that I am able to access this https://dotnetmyget.blob.core.windows.net/artifacts/dotnet-core/nuget/v3/flatcontainer/microsoft.netcore.dotnethostresolver/index.json via curl/Firefox on my PC.

[dagood]

My only other thought is the service disruption listed at http://status.myget.org/, but I'm not 100% sure this applies to dotnet.myget.org.

I'd suggest trying to get a simpler repro case and asking about this on https://github.com/NuGet/Home/issues. My suggestion is a basic dotnet new console project with a PackageReference on the Microsoft.NETCore.DotNetHostResolver package and pointing at the https://dotnet.myget.org/F/dotnet-core/api/v3/index.json NuGet feed.

[L-Dogg]

I've just tried building corefx at work on my work laptop and it finished successfully. Maybe it has something to do with my home ISP - will try to reproduce it using other connection. Thank you guys!

[L-Dogg]

I finally had some time to start working on that issue and have some questions:

1. Do we need to move Thai symbol parsing tests to the corefx repo? They invoke the CustomParser's methods that utilise Utf8Parser under the hood.
2. I'm not sure about how many InlineData attributes should I use? There are numerous in the corefxlab, however this doc: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md states we should limit them to the key examples. Should I leave only the standard parse, max/min value trio? 😄
3. Am I correct to leave "baseline" tests untouched, as they test the [typename].TryParse, not Utf8Parser.TryParse?

[L-Dogg]

Apart from the questions above, here's what I've managed to code:
https://github.com/L-Dogg/corefx/commit/4f9f6ddeaf67da6671e70a7d63db61ec7e4f4c78

[ahsonkhan]

1. Do we need to move Thai symbol parsing tests to the corefx repo? They invoke the CustomParser's methods that utilise Utf8Parser under the hood.

We do not need to move the Thai symbol parsing tests. They are testing experimental APIs that didn't ship and can remain in corefxlab as is.

1. I'm not sure about how many InlineData attributes should I use? There are numerous in the corefxlab, however this doc: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md states we should limit them to the key examples. Should I leave only the standard parse, max/min value trio? 😄

I think we should add some, but not all to get a balance between the two extremes (keeping the trio or adding them all). The objective is to get the minimal set that capture just enough useful scenarios. For example, I would add the single digit case like "0" and at least a special case like leading zeroes ("00000000000000000000123abcdfg"). Here is an example where we have made optimizations for single digit inputs - https://github.com/dotnet/coreclr/pull/18383. We don't need to add the '+' prefixed numbers or all digit length inputs. @adamsitnik, can provide additional guidance on your PR, once it is ready. I would encourage you to submit a PR marked as WIP (work in progress) in corefx and I can provide targeted suggestions there.

1. Am I correct to leave "baseline" tests untouched, as they test the [typename].TryParse, not Utf8Parser.TryParse?

Yes. They provide a good metric to compare against.

Also, I think there are plans to port all performance tests to BenchmarkDotNet soon, so I wouldn't worry too much about getting the right IterationCount/InnerIterationCount/etc. as BDN takes care of that for us. For example, see this recent performance test added by @ViktorHofer: https://github.com/dotnet/corefx/pull/30837

Apart from the questions above, here's what I've managed to code

Great. Submit a PR after making any other necessary changes based on the conversation here and I can review it (once you are ready).

[ViktorHofer]

Also, I think there are plans to port all performance tests to BenchmarkDotNet soon, so I wouldn't worry too much about getting the right IterationCount/InnerIterationCount/etc. as BDN takes care of that for us

Right, please don't set any InnerIterationCount manually as it will make it harder porting the test to BDN.

[fredeil]

I was about to remove these tests after finishing up https://github.com/dotnet/corefx/pull/32026, but I see they have been remove from the master branch already. I guess there is nothing for me to do here then @ahsonkhan

[ahsonkhan]

They were ported over to BenchmarkDotNet (BDN) here: https://github.com/dotnet/corefxlab/pull/2394 by @adamsitnik.

I think we can safely remove the redundant ones.
For instance, this is now unnecessary: https://github.com/dotnet/corefxlab/blob/98ea133b2a3d36d377b59ec44437ae780f8529a7/tests/Benchmarks/System.Text.Primitives/PrimitiveParserBoolPerfTests.cs#L12

I would keep the custom parser benchmarks though:
https://github.com/dotnet/corefxlab/blob/98ea133b2a3d36d377b59ec44437ae780f8529a7/tests/Benchmarks/System.Text.Primitives/PrimitiveParserInt16PerfTests.cs#L77

[fredeil]

Oh ok, they were ported already?

Does it mean that https://github.com/dotnet/corefx/pull/32026 won't get merged?

[ahsonkhan]

Oh ok, they were ported already?

They were ported to BDN in corefxlab. We should still remove them as they are redundant.

Does it mean that dotnet/corefx#32026 won't get merged?

It will. The goal here was to move over the perf tests from corefxlab to corefx since that is where the types/APIs live now.

[fredeil]

Could you assign me to this one? 😄
Do I need a collaborator invite here too?

[ahsonkhan]

Do I need a collaborator invite here too?

Yes, I think so. cc @karelz, @joshfree

[karelz]

@fredeil invite sent. Ping us here once you accept.

[fredeil]

@karelz invite accepted