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.

Sign up for GitHub

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

Jump to bottom

Convert SemanticRange to a struct + add RazorRange #9184

Closed
wants to merge 7 commits into from
Closed

Convert SemanticRange to a struct + add RazorRange #9184

wants to merge 7 commits into from

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Aug 28, 2023 •
edited
Loading

Contributes to #9092

  • WIP

I noticed even by just making SemanticRange struct from the original class actually increases allocation size for our existing benchmarks.

At first I thought it has to do with copy allocations but even making readonly struct from struct does not change the allocation size.

This could be due to having List operations on SemanticRange in TagHelperSemanticRangeVisitor and RazorSemanticTokensInfoService.

  • when a class
|                                 Method | WithMultiLineComment |     Mean |    Error |   StdDev |   Median |      Min |      Max | Allocated |
|--------------------------------------- |--------------------- |---------:|---------:|---------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Handling' |                False | 13.59 us | 14.44 us | 0.792 us | 13.39 us | 12.92 us | 14.47 us |   3.66 KB |
| 'Razor Semantic Tokens Range Handling' |                 True | 40.02 us | 20.20 us | 1.107 us | 39.64 us | 39.15 us | 41.27 us |  13.09 KB |

|                                 Method | NumberOfCsSemanticRangesToReturn |      Mean |      Error |    StdDev |    Median |       Min |       Max |   Gen0 | Allocated |
|--------------------------------------- |--------------------------------- |----------:|-----------:|----------:|----------:|----------:|----------:|-------:|----------:|
| 'Razor Semantic Tokens Range Endpoint' |                                0 |  4.362 us |  1.9576 us | 0.1073 us |  4.378 us |  4.247 us |  4.460 us |      - |   3.19 KB |
| 'Razor Semantic Tokens Range Endpoint' |                              100 |  9.209 us |  0.2243 us | 0.0123 us |  9.211 us |  9.195 us |  9.220 us | 0.0153 |   4.48 KB |
| 'Razor Semantic Tokens Range Endpoint' |                             1000 | 83.039 us | 39.3322 us | 2.1559 us | 84.094 us | 80.558 us | 84.464 us |      - |  11.74 KB |

|                                  Method |     Mean |    Error |  StdDev |   Median |      Min |      Max | Allocated |
|---------------------------------------- |---------:|---------:|--------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Scrolling' | 936.4 us | 126.3 us | 6.92 us | 938.9 us | 928.5 us | 941.7 us | 177.92 KB |
  • when a readonly struct
|                                 Method | WithMultiLineComment |     Mean |    Error |   StdDev |   Median |      Min |      Max | Allocated |
|--------------------------------------- |--------------------- |---------:|---------:|---------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Handling' |                False | 31.60 us | 1.444 us | 0.079 us | 31.62 us | 31.51 us | 31.66 us |    3.9 KB |
| 'Razor Semantic Tokens Range Handling' |                 True | 49.31 us | 6.292 us | 0.345 us | 49.51 us | 48.91 us | 49.52 us |  16.02 KB |

|                                 Method | NumberOfCsSemanticRangesToReturn |      Mean |     Error |    StdDev |    Median |       Min |        Max |   Gen0 | Allocated |
|--------------------------------------- |--------------------------------- |----------:|----------:|----------:|----------:|----------:|-----------:|-------:|----------:|
| 'Razor Semantic Tokens Range Endpoint' |                                0 |  4.758 us |  2.579 us | 0.1413 us |  4.733 us |  4.630 us |   4.910 us | 0.0076 |   3.38 KB |
| 'Razor Semantic Tokens Range Endpoint' |                              100 | 11.168 us |  4.366 us | 0.2393 us | 11.168 us | 10.928 us |  11.407 us | 0.0153 |   6.63 KB |
| 'Razor Semantic Tokens Range Endpoint' |                             1000 | 98.349 us | 44.810 us | 2.4562 us | 98.288 us | 95.924 us | 100.836 us | 0.1221 |  31.47 KB |

|                                  Method |     Mean |     Error |    StdDev |   Median |      Min |      Max | Allocated |
|---------------------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Scrolling' | 1.039 ms | 0.0958 ms | 0.0052 ms | 1.038 ms | 1.034 ms | 1.045 ms | 197.91 KB |

maryamariyan
maryamariyan commented Aug 28, 2023
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticRange.cs
@@ -29,20 +39,27 @@ public SemanticRange(int kind, Range range, int modifier, bool fromRazor)
/// </summary>
public bool FromRazor { get; }

public int CompareTo(SemanticRange? other)
public int CompareTo(SemanticRange other)
Copy link
Member Author

@maryamariyan maryamariyan Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • Improve CompareTo
  • Get new benchmarks
  • Confirm tests pass

@maryamariyan
Copy link
Member Author

maryamariyan commented Aug 29, 2023 •
edited
Loading

I noticed even by just making SemanticRange struct from the original class actually increases allocation size for our existing benchmarks.

At first I thought it has to do with copy allocations but even making readonly struct from struct does not change the allocation size.

This could be due to having List operations on SemanticRange in TagHelperSemanticRangeVisitor and RazorSemanticTokensInfoService.

  • when a class
|                                 Method | WithMultiLineComment |     Mean |    Error |   StdDev |   Median |      Min |      Max | Allocated |
|--------------------------------------- |--------------------- |---------:|---------:|---------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Handling' |                False | 13.59 us | 14.44 us | 0.792 us | 13.39 us | 12.92 us | 14.47 us |   3.66 KB |
| 'Razor Semantic Tokens Range Handling' |                 True | 40.02 us | 20.20 us | 1.107 us | 39.64 us | 39.15 us | 41.27 us |  13.09 KB |

|                                 Method | NumberOfCsSemanticRangesToReturn |      Mean |      Error |    StdDev |    Median |       Min |       Max |   Gen0 | Allocated |
|--------------------------------------- |--------------------------------- |----------:|-----------:|----------:|----------:|----------:|----------:|-------:|----------:|
| 'Razor Semantic Tokens Range Endpoint' |                                0 |  4.362 us |  1.9576 us | 0.1073 us |  4.378 us |  4.247 us |  4.460 us |      - |   3.19 KB |
| 'Razor Semantic Tokens Range Endpoint' |                              100 |  9.209 us |  0.2243 us | 0.0123 us |  9.211 us |  9.195 us |  9.220 us | 0.0153 |   4.48 KB |
| 'Razor Semantic Tokens Range Endpoint' |                             1000 | 83.039 us | 39.3322 us | 2.1559 us | 84.094 us | 80.558 us | 84.464 us |      - |  11.74 KB |

|                                  Method |     Mean |    Error |  StdDev |   Median |      Min |      Max | Allocated |
|---------------------------------------- |---------:|---------:|--------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Scrolling' | 936.4 us | 126.3 us | 6.92 us | 938.9 us | 928.5 us | 941.7 us | 177.92 KB |
  • when a readonly struct
|                                 Method | WithMultiLineComment |     Mean |    Error |   StdDev |   Median |      Min |      Max | Allocated |
|--------------------------------------- |--------------------- |---------:|---------:|---------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Handling' |                False | 31.60 us | 1.444 us | 0.079 us | 31.62 us | 31.51 us | 31.66 us |    3.9 KB |
| 'Razor Semantic Tokens Range Handling' |                 True | 49.31 us | 6.292 us | 0.345 us | 49.51 us | 48.91 us | 49.52 us |  16.02 KB |

|                                 Method | NumberOfCsSemanticRangesToReturn |      Mean |     Error |    StdDev |    Median |       Min |        Max |   Gen0 | Allocated |
|--------------------------------------- |--------------------------------- |----------:|----------:|----------:|----------:|----------:|-----------:|-------:|----------:|
| 'Razor Semantic Tokens Range Endpoint' |                                0 |  4.758 us |  2.579 us | 0.1413 us |  4.733 us |  4.630 us |   4.910 us | 0.0076 |   3.38 KB |
| 'Razor Semantic Tokens Range Endpoint' |                              100 | 11.168 us |  4.366 us | 0.2393 us | 11.168 us | 10.928 us |  11.407 us | 0.0153 |   6.63 KB |
| 'Razor Semantic Tokens Range Endpoint' |                             1000 | 98.349 us | 44.810 us | 2.4562 us | 98.288 us | 95.924 us | 100.836 us | 0.1221 |  31.47 KB |

|                                  Method |     Mean |     Error |    StdDev |   Median |      Min |      Max | Allocated |
|---------------------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Scrolling' | 1.039 ms | 0.0958 ms | 0.0052 ms | 1.038 ms | 1.034 ms | 1.045 ms | 197.91 KB |

@maryamariyan
Copy link
Member Author

Going over other commits I tried for perf improvement, but none seemed to make significant changes

e.g. with diff in 616cb25:

before

|                                 Method | WithMultiLineComment |      Mean |     Error |    StdDev |    Median |       Min |       Max | Allocated |
|--------------------------------------- |--------------------- |----------:|----------:|----------:|----------:|----------:|----------:|----------:|
| 'Razor Semantic Tokens Range Handling' |                False |  7.814 us | 0.4245 us | 0.6354 us |  7.649 us |  7.013 us |  9.273 us |   3.55 KB |
| 'Razor Semantic Tokens Range Handling' |                 True | 27.430 us | 1.4985 us | 2.1965 us | 27.556 us | 24.451 us | 32.794 us |  14.49 KB |

|                                 Method | NumberOfCsSemanticRangesToReturn |       Mean |     Error |    StdDev |     Median |        Min |        Max |   Gen0 | Allocated |
|--------------------------------------- |--------------------------------- |-----------:|----------:|----------:|-----------:|-----------:|-----------:|-------:|----------:|
| 'Razor Semantic Tokens Range Endpoint' |                                0 |   4.630 us | 0.1307 us | 0.1916 us |   4.575 us |   4.374 us |   5.006 us | 0.0076 |   3.09 KB |
| 'Razor Semantic Tokens Range Endpoint' |                              100 |  10.222 us | 0.1710 us | 0.2506 us |  10.182 us |   9.814 us |  10.653 us | 0.0153 |   4.35 KB |
| 'Razor Semantic Tokens Range Endpoint' |                             1000 | 109.107 us | 2.9281 us | 4.3826 us | 108.329 us | 101.251 us | 116.238 us |      - |  11.64 KB |

|                                  Method |     Mean |    Error |   StdDev |   Median |      Min |      Max | Allocated |
|---------------------------------------- |---------:|---------:|---------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Scrolling' | 871.9 us | 12.35 us | 18.48 us | 868.4 us | 834.3 us | 906.1 us | 171.33 KB |

after

|                                 Method | WithMultiLineComment |      Mean |     Error |    StdDev |    Median |       Min |       Max | Allocated |
|--------------------------------------- |--------------------- |----------:|----------:|----------:|----------:|----------:|----------:|----------:|
| 'Razor Semantic Tokens Range Handling' |                False |  8.254 us | 0.4814 us | 0.7056 us |  8.197 us |  7.158 us |  9.255 us |   3.56 KB |
| 'Razor Semantic Tokens Range Handling' |                 True | 26.878 us | 1.1436 us | 1.6763 us | 26.929 us | 24.206 us | 29.040 us |  14.49 KB |

|                                 Method | NumberOfCsSemanticRangesToReturn |       Mean |     Error |    StdDev |     Median |        Min |        Max |   Gen0 | Allocated |
|--------------------------------------- |--------------------------------- |-----------:|----------:|----------:|-----------:|-----------:|-----------:|-------:|----------:|
| 'Razor Semantic Tokens Range Endpoint' |                                0 |   4.484 us | 0.1025 us | 0.1470 us |   4.464 us |   4.288 us |   4.889 us | 0.0076 |   3.09 KB |
| 'Razor Semantic Tokens Range Endpoint' |                              100 |  10.124 us | 0.1344 us | 0.1970 us |  10.148 us |   9.681 us |  10.473 us | 0.0153 |   4.38 KB |
| 'Razor Semantic Tokens Range Endpoint' |                             1000 | 111.612 us | 2.6318 us | 3.9391 us | 110.037 us | 106.880 us | 118.624 us |      - |  11.64 KB |

|                                  Method |     Mean |   Error |   StdDev |   Median |      Min |      Max | Allocated |
|---------------------------------------- |---------:|--------:|---------:|---------:|---------:|---------:|----------:|
| 'Razor Semantic Tokens Range Scrolling' | 853.3 us | 8.24 us | 12.33 us | 853.2 us | 833.3 us | 877.5 us | 171.33 KB |

Closing PR.

@maryamariyan maryamariyan deleted the dev/maryamariyan/semanticrange-struct branch April 1, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DustinCampbell DustinCampbell DustinCampbell left review comments

@davidwengier davidwengier davidwengier left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maryamariyan @DustinCampbell @davidwengier