Skip to content

Conversation

@maryamariyan
Copy link
Contributor

@maryamariyan maryamariyan commented Apr 12, 2023

This PR adds benchmarks to compare perf for when there are zero, one or 1000 C# diagnostics in a razor document.
Note: The benchmarks are faking a call to roslyn to get the C# diagnostic.

// * Summary *

BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.18363.2274/1909/November2019Update/19H2)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=8.0.100-preview.2.23157.25
  [Host]     : .NET 8.0.0 (8.0.23.12803), X64 RyuJIT AVX2
  Job-UVKTRU : .NET 7.0.4 (7.0.423.11508), X64 RyuJIT AVX2
  Job-NLOVLU : .NET Framework 4.8 (4.8.4510.0), X64 RyuJIT VectorSize=256

PowerPlanMode=00000000-0000-0000-0000-000000000000

|      Method |        Job |            Toolchain |    N | FileType |           Mean |        Error |       StdDev |         Median |            Min |            Max |    Gen0 |    Gen1 | Allocated |
|------------ |----------- |--------------------- |----- |--------- |---------------:|-------------:|-------------:|---------------:|---------------:|---------------:|--------:|--------:|----------:|
| Diagnostics | Job-UVKTRU |             .NET 7.0 |    0 |    Small |       680.5 ns |     13.25 ns |     14.72 ns |       676.8 ns |       664.1 ns |       714.9 ns |  0.0105 |       - |    1008 B |
| Diagnostics | Job-NLOVLU | .NET Framework 4.7.2 |    0 |    Small |     1,450.9 ns |     13.63 ns |     10.64 ns |     1,449.4 ns |     1,435.6 ns |     1,466.3 ns |  0.2041 |       - |    1292 B |
| Diagnostics | Job-UVKTRU |             .NET 7.0 |    0 |    Large |       676.7 ns |      8.74 ns |      7.75 ns |       675.3 ns |       664.0 ns |       695.2 ns |  0.0105 |       - |    1008 B |
| Diagnostics | Job-NLOVLU | .NET Framework 4.7.2 |    0 |    Large |     1,417.6 ns |     19.14 ns |     16.96 ns |     1,418.1 ns |     1,380.3 ns |     1,436.1 ns |  0.2041 |       - |    1292 B |
| Diagnostics | Job-UVKTRU |             .NET 7.0 |    1 |    Small |     1,518.9 ns |     20.69 ns |     17.28 ns |     1,520.6 ns |     1,491.4 ns |     1,545.6 ns |  0.0191 |       - |    1888 B |
| Diagnostics | Job-NLOVLU | .NET Framework 4.7.2 |    1 |    Small |     2,961.7 ns |     16.12 ns |     14.29 ns |     2,959.5 ns |     2,939.5 ns |     2,989.1 ns |  0.3433 |       - |    2166 B |
| Diagnostics | Job-UVKTRU |             .NET 7.0 |    1 |    Large |     3,998.0 ns |     44.20 ns |     41.34 ns |     4,018.0 ns |     3,928.0 ns |     4,047.9 ns |  0.0153 |       - |    1888 B |
| Diagnostics | Job-NLOVLU | .NET Framework 4.7.2 |    1 |    Large |     6,723.4 ns |     69.88 ns |     61.95 ns |     6,740.4 ns |     6,521.6 ns |     6,777.4 ns |  0.3357 |       - |    2166 B |
| Diagnostics | Job-UVKTRU |             .NET 7.0 | 1000 |    Small |   587,534.5 ns |  6,349.26 ns |  5,628.45 ns |   586,930.5 ns |   580,090.1 ns |   596,473.4 ns |  4.8828 |  0.9766 |  522385 B |
| Diagnostics | Job-NLOVLU | .NET Framework 4.7.2 | 1000 |    Small | 1,088,137.8 ns | 10,777.28 ns |  9,553.78 ns | 1,083,954.7 ns | 1,078,160.4 ns | 1,107,036.9 ns | 89.8438 | 25.3906 |  572962 B |
| Diagnostics | Job-UVKTRU |             .NET 7.0 | 1000 |    Large | 3,006,419.8 ns | 30,803.00 ns | 28,813.15 ns | 2,997,595.7 ns | 2,965,953.1 ns | 3,058,587.1 ns |  3.9063 |       - |  522388 B |
| Diagnostics | Job-NLOVLU | .NET Framework 4.7.2 | 1000 |    Large | 4,606,931.6 ns | 30,717.45 ns | 25,650.46 ns | 4,607,103.9 ns | 4,529,810.9 ns | 4,634,828.9 ns | 85.9375 | 23.4375 |  573006 B |

Related to: #8539

TODO next:

  • Add a benchmark that also includes the real cost of making an LSP call to roslyn (rather than faking the ClientNotifierServiceBase)
  • Add a benchmark that generates an RZ000X diagnostic (razor rather than CS000X diagnostic).

@maryamariyan maryamariyan requested a review from a team as a code owner April 12, 2023 00:30
@maryamariyan maryamariyan self-assigned this Apr 12, 2023
return Task.FromResult((TResponse)result);
}

private static object BuildDiagnostics(int numDiagnostics)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better if this method was called in the setup method, and the array passed directly to the ClientNotifierService. At the moment the benchmark is measuring the memory allocated by creating this array, which is likely to mask any improvements, and make identifying real problems harder.

Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

LGTM. Would be keen to see updated run results, to know if there is actually as much low hanging fruit here as the initial run suggested, or it was just in the diagnostic creation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants