Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and refactor FCSBenchmarks #13419

Merged
merged 20 commits into from
Jul 15, 2022
Merged

Conversation

safesparrow
Copy link
Contributor

@safesparrow safesparrow commented Jun 29, 2022

Major cleanup and fixes of the benchmarks in tests/FCSBenchmarks projects.

Changes in all benchmarks

  • All benchmarks should now build and work
  • Use BDN's console API to make the benchmarks more flexible (end-user can tweak the config without having to change code and build)
  • General cleanup

Changes in historical benchmark (BenchmarkComparison)

Changes in the notebook (runner.ipynb)

  • Fix it
  • Fix the commit-based benchmark which should now work at least with recent versions (it might still fail when building an old codebase)
  • Replace multiple projects + String.Replace with a single project using MSBuild properties
  • Refactor/cleanup
  • Store results in JSON instead of CSV, put artifacts in separate directories (inside .artifacts/, read them at the end

Changes in the BDN benchmark (now renamed to HistoricalBenchmark.fsproj)

  • Refactor
  • Add it to VisualFSharp.sln
  • Add it to the InternalsVisibleTo list in FSharp.Compiler.Service

Changes in FSharp.Compiler.Benchmarks

  • Deduplicate some of the code by reusing code from HistoricalBenchmark
  • Now depends on HistoricalBenchmark mainly to reuse the SingleFileCompiler. This might be somewhat controversial - HistoricalBenchmark needs to support old versions of FCS to allow doing a comparison, which can be limiting. However, currently this isn't a problem - once it is, we could either duplicate the code again or extract the shared bits to a separate project.

* extract all types into separate files
* deduplicate the shared source file
* start creating a script for running all benchmarks
* Refactor all the benchmarks a bit
- use JSON reports instead of CSV
- put results in separate directories so that they can be accessed after the benchmark
- read the results after all benchmarks finish rather than keeping them in memory

* Add `BenchmarkComparison/README.md`
* refactor runner.ipynb further
* Clear notebook outputs
* Rename entrypoint file to `Program.fs`
* Finish the smoke test script
@@ -1,27 +0,0 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced by a single project file with MSBuild properties

@@ -1,25 +0,0 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced by a single project file with MSBuild properties

@@ -1,25 +0,0 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced by a single project file with MSBuild properties

@@ -1,23 +0,0 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for an extra top-level project now

@safesparrow safesparrow marked this pull request as ready for review June 30, 2022 01:17
@safesparrow
Copy link
Contributor Author

@vzarytovskii Would you be able to review? Thanks!

@vzarytovskii
Copy link
Member

vzarytovskii commented Jun 30, 2022

@vzarytovskii Would you be able to review? Thanks!

Thanks for changes, they look good will do more thorough review tomorrow, once at my laptop :)

One thing which comes to mind is to add benchmarks to FSharp.sln too (and maybe FSharp.Compiler.Service.sln, @auduchinok wdyt?). Some people are not using the VisulaFSharp.sln.

@auduchinok
Copy link
Member

auduchinok commented Jun 30, 2022

One thing which comes to mind is to add benchmarks to FSharp.sln too (and maybe FSharp.Compiler.Service.sln, @auduchinok wdyt?). Some people are not using the VisulaFSharp.sln`.

Yes, thanks. We only use the FSharp.Compiler.Service solution, so it would be useful.

@safesparrow
Copy link
Contributor Author

@vzarytovskii Let me know if this is something worth merging and any changes you'd like to see :)

… F# project `HistoricalBenchmark.Runner`, call that logic from `runner.ipynb`

* Add a few customisations (mostly directories)

* Reuse existing checkouts/builds/benchmark reports

* Provide simple `Runner.runAll` method for doing everything

* Include sample results with three historical comparisons
@safesparrow
Copy link
Contributor Author

safesparrow commented Jul 13, 2022

I did a few changes:

  • moved all the logic from BenchmarkComparison/runner.ipynb into a new F# project HistoricalBenchmark.Runner
  • added a few customisations
  • made the benchmarking more robust and easier to run (one line of code in the notebook)
  • added three sample results

@vzarytovskii @KevinRansom Would it be possible to get the changes reviewed? If this is not a change you are willing to consider at this time that's fine - but I would be keen to know whether there is any point spending more time on this👍

@vzarytovskii
Copy link
Member

I did a few changes:

  • moved all the logic from BenchmarkComparison/runner.ipynb into a new F# project HistoricalBenchmark.Runner
  • added a few customisations
  • made the benchmarking more robust and easier to run (one line of code in the notebook)
  • added three sample results

@vzarytovskii Vlad Zarytovskii FTE @KevinRansom Kevin Ransom FTE Would it be possible to get the changes reviewed? If this is not a change you are willing to consider at this time that's fine - but I would be keen to know whether there is any point spending more time on this👍

It looks great, sorry it was taking long time, we were a bit preoccupied with some other stuff :)

@vzarytovskii vzarytovskii merged commit c19ebd5 into dotnet:main Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants