Add the performance benchmark project for PowerShell performance testing by daxian-dbw · Pull Request #15242 · PowerShell/PowerShell

daxian-dbw · 2021-04-15T22:40:54Z

PR Summary

Add the performance benchmark project for PowerShell performance testing.

The structure of the test\perf directory is

├───benchmarks
└───dotnet-tools
    ├───BenchmarkDotNet.Extensions
    ├───Reporting
    └───ResultsComparer

The benchmarks folder contains the benchmark project, and will contain all the benchmarks we are going to write in future for testing PowerShell performance. It's targeting the .NET runtime net6.0, and is able to target different versions of PS sdk nuget packages. By default, it runs the tests targeting the current code base.

The tools folder contains projects that are copied from dotnet/performance,
the performance testing repository for the .NET runtime and framework libraries.

BenchmarkDotNet.Extensions
- It provides the needed extensions for running benckmarks,
  such as the RecommendedConfig which defines the set of recommended configurations for running the dotnet benckmarks (we use the same config).
Reporting
- It provides additional result reporting support
  which may be useful to us when running our benchmarks in lab.
ResultsComparer
- It's a tool for comparing different benchmark results.
  It's very useful to show the regression of new changes by comparing its benchmark results to the baseline results.

The files that are copied from dotnet/performance are kept as is, without adding the copyright header used in PowerShell repository.

PR Checklist

iSazonov

I wonder - has MSFT PowerShell team already internal perf LAB?

What is Baseline we should use? PowerShell 6.0? 7.0? Latest LTS version? Latest version?

Has MSFT team a roadmap for adding perf tests? I believe we need a perf test set with good code coverage.

Will perf tests be mandatory in new PRs to avoid perf regressions?

iSazonov · 2021-04-16T12:45:08Z

Will DependaBot automatically update the package versions in all csproj files added in the PR?

Please add the csproj here: https://github.com/PowerShell/PowerShell/blob/master/.github/dependabot.yml

iSazonov · 2021-04-16T13:29:38Z

#Addressed.
We have tools folder, test\tools and now test\perf\tools. Maybe rename last one to perftools?

daxian-dbw · 2021-04-16T17:36:53Z

@iSazonov The perf/tools folder was renamed to perf/dotnet-tools, to differentiate it from other tools folders.

I wonder - has MSFT PowerShell team already internal perf LAB?

No, unfortunately :( We used to have perf and stressing tests before open source for Windows PowerShell, but not for the open-sourced PowerShell.

What is Baseline we should use? PowerShell 6.0? 7.0? Latest LTS version? Latest version?

It's based on what tests you are looking at. For CI regression tests, the baseline would be the results from the run without the PR changes. For high level regression tests, the base line would be the results from a previous preview, or a previous stable release.

Has MSFT team a roadmap for adding perf tests? I believe we need a perf test set with good code coverage.

No, we are still figuring things out. I also reached out to @adamsitnik for guidelines on performance testing in general.

Will perf tests be mandatory in new PRs to avoid perf regressions?

Ideally, we will enable the perf test for CI, so that we know what's getting better/worse with the changes. We can leverage what dotnet team has for this, but the first thing for us is to have a good suite of perf tests.

iSazonov · 2021-04-18T10:21:08Z

It's based on what tests you are looking at. For CI regression tests, the baseline would be the results from the run without the PR changes. For high level regression tests, the base line would be the results from a previous preview, or a previous stable release.

It would be great to have support of automatically creating these baselines in the scripts.

Ideally, we will enable the perf test for CI,

It seems it is impossible without a lab since a perf history can not be without the lab.

adamsitnik · 2021-04-26T14:42:52Z

+                CorrelationId = environment.GetEnvironmentVariable("HELIX_CORRELATION_ID"),
+                PerfRepoHash = environment.GetEnvironmentVariable("PERFLAB_PERFHASH"),
+                Name = environment.GetEnvironmentVariable("PERFLAB_RUNNAME"),
+                Queue = environment.GetEnvironmentVariable("PERFLAB_QUEUE"),


all these env vars are specific to current dotnet/performance repo setup & CI. Do you plan to use .NET Performance Team hardware lab? If not, I think that you should not be including anything from
the Reporting folder

We don't have a hardware lab, so it's definitely a preferable option for us if it's possible for us to run our perf tests in the .NET performance team hardware lab 😄. Even if we cannot use the .NET team lab, maybe it's still preferable for us to keep the same format of the exported reports, so that we can leverage the tools from the .NET performance teams easily?

adamsitnik · 2021-04-26T14:56:33Z

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <!-- We are using a single TFM for this project, the drawback is that we cannot run benchmarks
+       targeting other .NET runtime versions, such as net5.0 (PS7.1) and netcoreapp3.1 (PS7.0) -->


this is something that we should discuss at the meeting as I would be very happy to help you ensure that you can benchmark various .NET and PowerShell releases

adamsitnik · 2021-04-26T15:02:30Z

+namespace BenchmarkDotNet.Extensions
+{
+    // a simplified copy of internal BDN type: https://github.com/dotnet/BenchmarkDotNet/blob/0445917bf93059f17cb09e7d48cdb5e27a096c37/src/BenchmarkDotNet/Disassemblers/Exporters/GithubMarkdownDisassemblyExporter.cs#L35-L80
+    internal static class DiffableDisassemblyExporter


this type is a set of hacks that we have temporarily added to the performance repo and never had the time to provide a proper solution. IMO you should not be copying it and we should just make these types public in BenchmarkDotNet if DisassemblyDiagnoser does not meet your needs.

Oh, didn't know that. Yeah, I don't think we would need anything beyond what DisassemblyDiagnoser supports.
I found the DiffableDisassemblyExporter is used in PerfLabExporter, so it's related to lab result reporting. Again, we'd like to use the same format for reporting like in .NET performance, so that it would be easier for us to leverage other tools available in .NET performance repo. From that perspective, maybe we should keep this type? Let's talk more about this in our meeting.

adamsitnik · 2021-04-26T15:10:35Z

+                CommandLineOptions.ParseAndRemoveStringsParameter(argsList, "--category-exclusion-filter", out categoryExclusionFilterValue);
+                CommandLineOptions.ParseAndRemoveBooleanParameter(argsList, "--disasm-diff", out getDiffableDisasm);
+
+                CommandLineOptions.ValidatePartitionParameters(partitionCount, partitionIndex);


to be honest with you I don't think that you are going to need any of these custom arguments in the near future:

we have added --partition-count and --partition-index after our portfolio has reached three thousands of benchmarks and running all of them was taking 5 hours. The partitioning allows for distributing this work amongst many machines.

we have added --exclusion-filter and --category-exclusion-filter after WebAssembly (WASM) supports was added to .NET Runtime and it turned out that we could not run multithreaded benchmarks for WASM because it's single threaded by design.

--disasm-diff this is something that we have added just to simplify the process of exporting generated disassembly to our ReportingSystem. If you would also like to do that we should just add it to BenchmarkDotNet and remove the reflection hacks.

That's good to know. As for the --disasm-diff flag, we'd like to generate the same reporting as .NET performance, so that sounds like something we may need.

and remove the reflection hacks.

where is the reflection hack?

The --partition-count and --partition-index sound useful to keep, though we won't need it for the foreseeable future. I hope we can also grow the number of our perf tests to the point that we need to use those 2 flags 😄

The --exclusion-filter and --category-exclusion-filter are also useful to us. We are planning to have Interal and Public categories, for benchmarks targeting the internal APIs and public APIs respectively. For benchmarks that targets the internal APIs, they won't run with the existing 7.0.x and 7.1.x PowerShell SDK packages because it requires InternalVisible attribute to be declared in the PowerShell assemblies. So we may need those 2 flags when running our benchmarks targeting the 7.0.x and 7.1.x packages.

where is the reflection hack?

https://github.com/dotnet/performance/blob/ba63f00d5a6ace5fcabb4d9ba20eecdee3925b52/src/harness/BenchmarkDotNet.Extensions/DiffableDisassemblyExporter.cs#L62-L88

adamsitnik · 2021-04-28T16:34:48Z

@daxian-dbw @adityapatwardhan I've reopened the bug that you have hit today: dotnet/BenchmarkDotNet#837

iSazonov

It is great we can pickup a baseline by targeting published packages.
Another hot scenario we could think is targeting "before the PR" or master git branch vs current dev branch.

daxian-dbw · 2021-04-29T17:31:52Z

Currently there is one issue when running the benchmarks targeting the current code base (with ProjectReference takes effect). The step that runs dotnet build -c Release on the generated temporary project takes too long and always fail in the end. Then BDN attempts to run dotnet build again with --no-dependencies, and that succeeds very fast:

Actually, when running benchmarks with the ‘ProjectReference’ setting, the very first dotnet run on our powershell-perf.csporj already built the dependency powershell/dotnet-tool projects, so there is no need to build those dependency projects again when running dotnet build on the generated temporary project, and thus directly applying --no-dependencies should be just fine. I already reached out to @adamsitnik for help and we will resolve it offline in a separate PR.

daxian-dbw · 2021-04-29T17:32:56Z

@adityapatwardhan I think this PR is ready to be merged. Improvement can come in separate PRs.

adityapatwardhan · 2021-04-29T18:00:03Z

Will look at it today

adityapatwardhan · 2021-04-29T18:49:18Z

+        [GlobalSetup(Target = nameof(InvokeMethod))]
+        public void GlobalSetup()
+        {
+            SetupRunspace();


Should we make sure that the PowerShell / .NET telemetry is off.

Good question. Maybe we should disable it and I will do that from the command line level.

daxian-dbw · 2021-04-29T20:59:25Z

@adityapatwardhan Files in dotnet-tools are copied from dotnet/performance and we probably will need to sync them from time to time, so let's just keep them as they are.

adityapatwardhan · 2021-04-29T22:27:31Z

@daxian-dbw - just noticed there are 4 CodeFactor issues not under dotnet-tools.. could you fix them please?

adityapatwardhan · 2021-04-29T22:38:44Z

Maybe we can ask @TravisEz13 or @SteveL-MSFT to add the test/perf/dotnet-tools/* path in the ignore list for CodeFactor.

Instructions here: https://support.codefactor.io/i15-ignoring-code-files

Ignoring code files | CodeFactor.io

…ing (PowerShell#15242)

ghost · 2021-05-27T17:31:56Z

🎉v7.2.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

daxian-dbw requested review from TravisEz13, adityapatwardhan and anmenaga as code owners April 15, 2021 22:40

ghost assigned adityapatwardhan Apr 15, 2021

iSazonov reviewed Apr 16, 2021

View reviewed changes

adityapatwardhan reviewed Apr 16, 2021

View reviewed changes

Comment thread test/perf/benchmarks/powershell-perf.csproj Outdated

daxian-dbw added 9 commits April 19, 2021 11:18

Initial check-in for perf tests

424d5cd

Add docs and sample benchmarks

acdfc69

Add references

163c6f1

Exclude markdown files from 'test/perf/tools' folder

f804497

Update the '.spelling' file

9a4283d

Address review comments

4d1dbe1

Update URLs

2308a57

Update benchkmarks

602ec0f

Allow running benchmarks targeting different versions of PS packages

8edf6c2

daxian-dbw force-pushed the perf-tests branch from 02e9b7c to 8edf6c2 Compare April 22, 2021 17:52

Add script for basic benchmark runs

aea7af2

iSazonov reviewed Apr 24, 2021

View reviewed changes

Comment thread test/perf/perf.psm1

Comment thread test/perf/perf.psm1 Outdated

A couple updates

211fd05

iSazonov approved these changes Apr 26, 2021

View reviewed changes

adamsitnik reviewed Apr 26, 2021

View reviewed changes

daxian-dbw added 2 commits April 26, 2021 12:30

Address feedback from Adam

c5b551a

Update the README.md

9799147

adamsitnik mentioned this pull request Apr 28, 2021

Problems with default UnrollFactor in V0.11.0 dotnet/BenchmarkDotNet#837

Closed

daxian-dbw added 2 commits April 28, 2021 16:59

Make benchmarks more stable

3ae74aa

Add nuget.config that includes the 'benchmark-dotnet-prerelease' feed

0d314c2

iSazonov reviewed Apr 29, 2021

View reviewed changes

Comment thread test/perf/benchmarks/Engine.ScriptBlock.cs

Comment thread test/perf/benchmarks/README.md

Minor updates

08811fc

adityapatwardhan requested changes Apr 29, 2021

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Apr 29, 2021

Address Aditya's comments

f6bf604

adityapatwardhan approved these changes Apr 29, 2021

View reviewed changes

Address CodeFactor issues

d35c4d6

adityapatwardhan reviewed Apr 30, 2021

View reviewed changes

Comment thread .vsts-ci/linux.yml Outdated

daxian-dbw and others added 2 commits April 29, 2021 22:15

Fix typos

0b12443

Add trigger filter

bf30d9f

adityapatwardhan merged commit a62c9d9 into PowerShell:master Apr 30, 2021

adityapatwardhan added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Apr 30, 2021

adityapatwardhan added this to the 7.2.0-preview.6 milestone Apr 30, 2021

daxian-dbw deleted the perf-tests branch April 30, 2021 16:36

rkeithhill pushed a commit to rkeithhill/PowerShell that referenced this pull request May 3, 2021

Add the performance benchmark project for PowerShell performance test…

37549e3

…ing (PowerShell#15242)

Conversation

daxian-dbw commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Apr 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Apr 26, 2021 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Apr 26, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Apr 28, 2021

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Apr 29, 2021

Uh oh!

daxian-dbw commented Apr 29, 2021

Uh oh!

adityapatwardhan commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Apr 15, 2021 •

edited

Loading

iSazonov left a comment •

edited

Loading

iSazonov commented Apr 16, 2021 •

edited

Loading

daxian-dbw commented Apr 16, 2021 •

edited

Loading

daxian-dbw Apr 26, 2021 •

edited

Loading

daxian-dbw Apr 26, 2021 •

edited

Loading

adityapatwardhan commented Apr 29, 2021 •

edited by unfurl-links Bot

Loading