dotnet / roslyn-sdk

Roslyn-SDK templates and Syntax Visualizer
MIT License
498 stars 253 forks source link

Provide helpers for performance testing #1165

Open MattKotsenas opened 1 week ago

MattKotsenas commented 1 week ago

The roslyn-analyzers repo has a set of helpers to make performance testing of analyzers using BenchmarkDotNet easier. See https://github.com/dotnet/roslyn-analyzers/tree/main/src/PerformanceTests/Utilities/Common

It would be nice if we provided:

  1. A set of guidance around how to do write benchmarks for analyzers that don't run afoul of compilation caching nor measure project setup itself
  2. Where appropriate, add the necessary helpers to the Microsoft.CodeAnalysis.Testing.* packages to avoid copy/pasting code.
MattKotsenas commented 1 week ago

Additionally, it appears the Microsoft.CodeAnalysis.Analyzer.Testing libraries are releasing DEBUG versions instead of RELEASE, which trips up BenchmarkDotNet:

// Validating benchmarks:
//    * Assembly MyAssembly which defines benchmarks references non-optimized Microsoft.CodeAnalysis.Analyzer.Testing
        If you own this dependency, please, build it in RELEASE.
        If you don't, you can disable this policy by using 'config.WithOptions(ConfigOptions.DisableOptimizationsValidator)'.
sharwell commented 1 week ago

it appears the Microsoft.CodeAnalysis.Analyzer.Testing libraries are releasing DEBUG versions instead of RELEASE,

This is absolutely by design. It allows users to debug through the test code.

MattKotsenas commented 1 week ago

That's ~fine, just unexpected from my end.

I'm more documenting that if we address this performance testing issue we'd need to revisit that decision, or not leverage the existing testing packages for perf testing, or something else.

sharwell commented 1 week ago

It wouldn't significantly impact benchmarking if it was used. It would really just change the results for sections of code that aren't part of the optimization process.

sharwell commented 1 week ago

I'm not sure there are any guidelines or suggestions related to analyzer performance testing like this. I question the usefulness in practice of the test suite you mentioned, as well as its ability to guide analyzer authors in the direction of better performing analyzers. The reasons for this are super complex so its hard to describe in short form.

sharwell commented 1 week ago

To my knowledge:

  1. There are no documented procedures for testing the impact of changes on the scoped code analysis performed in the IDE
  2. There are no documented procedures for testing the impact of incremental code analysis for the purpose of executing code fixes in the IDE
  3. There are no documented procedures for testing the impact of analyzers on dotnet-format, etc.
  4. There are no documented procedures for testing the impact of analyzers on the build.
  5. There are no documented procedures for analyzing performance results in the context of costs that are shared across multiple analyzers.

The current state of the art that we use daily is: when a scenario involving one or more analyzers is observably slow, use PerfView to collect performance information about that scenario, and fix the low-hanging fruit.

sharwell commented 1 week ago

One example of a difficult scenario: sometimes analyzers are slow in a specific large project. In practice, we frequently observe that the analyzer spends a significant (often majority) on a very small section of code, e.g. one file. It's also often the case that the slow file isn't the largest file in the project, or really notable in any obvious way. BenchmarkDotNet is not very well suited to locating the slow file among all the rest such that it could be evaluated and/or extracted specifically.

MattKotsenas commented 1 week ago

Those are all good points, and in an ideal world we'd cover them all, however today we have approximately nothing to go on, so I'd assert that even a small doc with "considerations" and some best practices is an improvement worth considering.

The point of a benchmarking tool isn't to be a comprehensive regression suite; currently we're asserting that the way to correctly design an analyzer is "just already be an expert". As an author I currently have no way to know which design is better for any scenario, forget all scenarios. I'll assert that the existence of https://github.com/dotnet/roslyn-analyzers/tree/main/src/PerformanceTests is a proof by contradiction that BenchmarkDotNet is a useful tool.

MattKotsenas commented 1 week ago

Here's a small but concrete example: https://github.com/rjmurillo/moq.analyzers/pull/109/files#diff-5b0caf31d695c71b66dc3a6978f20d0da9738667e2627824b198eefdf86b0ed4

Writing a benchmark seems to require copy/pasting:

in order to write a benchmark. I also tried using the full testing harness but substituting my own EmptyVerifier instead of DefaultVerifier (see https://github.com/rjmurillo/moq.analyzers/pull/109/commits/55bb0996956dd06ca92fe22aa38bd04a264d28c7), but that was ~2x slower than this method (for both baseline and test case) which then makes my analyzer look better than it actually is (as a percentage of the baseline).

For instance, one step forward (though I don't know if it's the right one or not) would be to expose the CompilationHelpers as public classes, and to build the Verifier classes on top. I don't know if that's feasible or not given the current design.