Porting CoreClr benchmarks into the repo. by jorive · Pull Request #31 · dotnet/performance

jorive · 2018-05-16T22:35:13Z

Background

The idea is to have a single repository with .NET benchmarks that can be built and tested against different stages of the .NET pipeline/repositories/branches/releases.

What's in this PR?

This is the initial work to bring the benchmarks from CoreClr into this repo.

Additionally, I have converted the benchmarks old MSBuild/C# csproj into the .NET Core C# csproj, configured the projects with a common.props import, set treat warnings as errors, fixed the errors, and made the harness reference all the benchmarks (a single project needs to be built).

Finally, the benchmarks support netcoreapp1.1, netcoreapp2.0, and netcoreapp2.1 target frameworks (next, I might add desktop, thoughts?).

What's missing?

I'm currently finishing the build.py script for this folder.
Enable a CI validation for all projects.

michellemcdaniel

LGTM

AndyAyersMS

Does the global solution get messed up because the projects don't have unique GUIDs?

How are you handling package references for span in netcoreapp 1.1 and 2.0? Is referring to the portable span?

jorive · 2018-05-17T00:06:12Z

@AndyAyersMS I will remove the solution file. It's not needed anymore. It was my initial attempt. The whole GUID stuff was a mess.
Instead, I'm handling the whole netcoreapp[1.1|2.0] on the ./src/coreclr/harness/harness.csproj using the Condition attribute on the ItemGroup element (it is a cleaner solution to the *.sln file).
Now, you can run:

dotnet restore ./src/coreclr/harness/harness.csproj
dotnet publish ./src/coreclr/harness/harness.csproj -c release -f netcoreapp1.1
dotnet publish ./src/coreclr/harness/harness.csproj -c release -f netcoreapp2.0
dotnet publish ./src/coreclr/harness/harness.csproj -c release -f netcoreapp2.1
``` #Resolved

jorive · 2018-05-17T00:11:12Z

+    <ProjectReference Include="..\V8\Richards\Richards.csproj" />
+  </ItemGroup>
+
+  <ItemGroup Condition="'$(TargetFramework)'!='netcoreapp1.1'">


@AndyAyersMS This is how I handle different releases. #Resolved

adamsitnik

LGTM!

adamsitnik · 2018-05-17T13:13:34Z

+
+```log
+Xunit-Performance-Api
+Copyright (c) Microsoft Corporation 2015


nit: 2015 -> 2018? #Resolved

This is the current output of the Api.

In reply to: 188951690 [](ancestors = 188951690)

adamsitnik · 2018-05-17T13:14:25Z

+```cmd
+dotnet restore Harness/Harness.csproj
+dotnet publish Harness/Harness.csproj -c <Configuration> -f <Framework>
+```


is publish required? why not just dotnet run -c Release -f <Framework> ? #Resolved

This is to publish (build) a self contained folder. I do not like dotnet run when running benchmarks. It's slower because it implies: build -> run. dotnet <path to dll> should be the way to run benchmarks.
Below is the step to run benchmarks.

In reply to: 188951947 [](ancestors = 188951947)

adamsitnik · 2018-05-17T13:35:22Z


    <WarningLevel>4</WarningLevel>
-    <TreatWarningsAsErrors>false</TreatWarningsAsErrors>
+    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>


👍 #Resolved

adamsitnik · 2018-05-17T13:38:37Z

btw should we keep the coreclr in the folder structure? #Resolved

jorive · 2018-05-17T14:27:36Z

We do not need to keep it. I was trying to keep the same structure Drew has on his port of CoreFx. We could come up with something better.

In reply to: 389869235 [](ancestors = 389869235)

adamsitnik · 2018-05-17T14:32:13Z

brianrob · 2018-05-17T15:18:52Z

+
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <TargetFrameworks>netcoreapp1.1;netcoreapp2.0;netcoreapp2.1</TargetFrameworks>


Do we still need to build for netcoreapp1.1? Also, should we add net461 here? #Resolved

There is no draw back at the moment on supporting netcoreapp1.1. Once we stop providing support we could remove it.

In reply to: 188997692 [](ancestors = 188997692)

I will add desktop after some testing... Mono is missing too

In reply to: 189015906 [](ancestors = 189015906,188997692)

brianrob · 2018-05-17T15:22:44Z

@jorive, how do these benchmarks stay in sync with CoreCLR? I'm thinking we should work with the corefx folks that own the mirror to set it up to sync these files. This will require that the directory structures remain largely the same (it does not have to be the same from the root directory, but could be the same within the coreclr directory), but will give us the benefit of not having to manually port changes. What do you think? #Resolved

jorive · 2018-05-17T16:07:03Z

The directory is very closed to CoreClr, so you could potentially script a copy *.cs between benchmarks. The idea is that eventually we make CoreClr use these benchmarks :)

In reply to: 389905514 [](ancestors = 389905514)

jorive · 2018-05-17T16:10:00Z

I just found out that some benchmarks depend on CORE_ROOT being defined... I'll fix it :(

AndyAyersMS · 2018-05-17T16:28:39Z

The reliance on CORE_ROOT is usually a workaround for not being able to locate an assembly's directory, either to reference it (for the roslyn tests) or to find co-located input files. IIRC there were some missing APIs back when we first set up the benchmarks. See for instance dotnet/coreclr#9281.

It would be nice to fix the underlying problem, but I think that's only possible in netstandard1.5 and up.

jorive · 2018-05-17T16:32:20Z

@AndyAyersMS It seems a problem with the build in coreclr instead. With the current build, the resources/input-data are side-by-side with the assemblies, so they will not need to search for them in the source tree :)

AndyAyersMS · 2018-05-17T17:34:10Z

CoreCLR is actually the same way.

The input files are copied into the same directory as the test assembly during build via project file directives. But on older netstandards the app cannot easily figure out where that assembly is located in the file system.

jorive · 2018-05-17T21:56:24Z

OK, finally, all benchmarks build and run/pass.

michellemcdaniel · 2018-05-17T22:03:06Z

Latest change looks good to me.

adamsitnik · 2018-05-17T22:18:51Z

NOTE: CscBench code could be improved/cleaned

We also have two Csc benchmarks in Scenarios (compile hello world and compile roslyn), so maybe this one could be just deleted?

the code looks good to me! great job @jorive 🍻 !

- Updated to .NET Core tooling (Cli). - Reorganized the folders. - Renamed PerfHarness to PerformanceHarness and placed it side-by-side to the benchmarks. - Added a reference of the benchmarks to the harness. - Enabled treat warnings as error. - Fixed warning/errors. - Adams needed to define XUNIT_PERF. - Added logs folder to .gitignore. - Added ms-python.python vscode extension suggestion. - Added initial README.md for the CoreClr benchmarks. - Moved common package reference to common.props - Added a prefix to the benchmark assembly names so it is easy to find/iterate on automation/ad-hoc runs - Removed CORE_ROOT dependency!!! - NOTE: CscBench code could be improved/cleaned (for example, it does not run on .NET Framework) - Bonus: Enable Desktop: net461

First commit (copy from coreclr to performance)

0880b93

jorive requested review from a team and AndyAyersMS May 16, 2018 22:35

michellemcdaniel approved these changes May 16, 2018

View reviewed changes

AndyAyersMS reviewed May 16, 2018

View reviewed changes

jorive commented May 17, 2018

View reviewed changes

adamsitnik approved these changes May 17, 2018

View reviewed changes

brianrob approved these changes May 17, 2018

View reviewed changes

jorive merged commit 6253bdf into dotnet:master May 18, 2018

jorive deleted the dev/CoreClr-CQ branch May 18, 2018 01:47

Conversation

jorive commented May 16, 2018

Background

What's in this PR?

What's missing?

Uh oh!

michellemcdaniel left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

jorive commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorive May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik May 17, 2018 • edited by jorive Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorive May 17, 2018

Choose a reason for hiding this comment

Uh oh!

adamsitnik May 17, 2018 • edited by jorive Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorive May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik May 17, 2018 • edited by jorive Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented May 17, 2018 • edited by jorive Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorive commented May 17, 2018

Uh oh!

adamsitnik commented May 17, 2018

Uh oh!

brianrob May 17, 2018 • edited by jorive Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorive May 17, 2018

Choose a reason for hiding this comment

Uh oh!

jorive May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianrob commented May 17, 2018 • edited by jorive Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorive commented May 17, 2018

Uh oh!

jorive commented May 17, 2018

Uh oh!

AndyAyersMS commented May 17, 2018

Uh oh!

jorive commented May 17, 2018

Uh oh!

AndyAyersMS commented May 17, 2018

Uh oh!

jorive commented May 17, 2018

Uh oh!

michellemcdaniel commented May 17, 2018

Uh oh!

adamsitnik commented May 17, 2018

Uh oh!

Reviewers

Assignees

jorive commented May 17, 2018 •

edited

Loading

jorive May 17, 2018 •

edited

Loading

adamsitnik May 17, 2018 •

edited by jorive

Loading

adamsitnik May 17, 2018 •

edited by jorive

Loading

jorive May 17, 2018 •

edited

Loading

adamsitnik May 17, 2018 •

edited by jorive

Loading

adamsitnik commented May 17, 2018 •

edited by jorive

Loading

brianrob May 17, 2018 •

edited by jorive

Loading

jorive May 17, 2018 •

edited

Loading

brianrob commented May 17, 2018 •

edited by jorive

Loading