Skip to content

Restore perf work. #8995

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
Merged
merged 4 commits into from
Jul 26, 2023
Merged

Restore perf work. #8995

merged 4 commits into from
Jul 26, 2023

Conversation

@chsienki
Copy link
Member

@chsienki chsienki commented Jul 20, 2023

Based on #8212

  • Do an initial parse only once
  • If a document has changed, re-write the tag helpers, recording which ones were used
  • Re-run the tag helper re-write step, but check if the tag helpers have changed, and skip if we can

@chsienki
Restore perf work.
1152b7f 
Based on dotnet#8212

- Do an initial parse only once
- If a document has changed, re-write the tag helpers, recording which ones were used
- Re-run the tag helper re-write step, but check if the tag helpers have changed, and skip if we can
@chsienki chsienki added perf 📊 area-compiler Umbrella for all compiler issues labels Jul 20, 2023
@chsienki
Copy link
Member Author

chsienki commented Jul 20, 2023 •
edited
Loading

Benchmarks show expected improvements, with very small regressions in scenarios that were already fast (becuase we're doing slightly more work to calculate e.g. the tag helpers used).

Edit: Updated so that Baseline reflects main as of 7/20 rather than the previous target from a few months ago.

|                        Method |      Job | BuildConfiguration |       Mean |     Error |    StdDev | Ratio | RatioSD |      Gen0 |      Gen1 | Allocated | Alloc Ratio |
|------------------------------ |--------- |------------------- |-----------:|----------:|----------:|------:|--------:|----------:|----------:|----------:|------------:|
|         Razor_Add_Independent | Baseline |      Release_Nuget | 162.267 ms | 3.2322 ms | 3.3192 ms |  1.00 |    0.00 | 4000.0000 | 1000.0000 |  40.51 MB |        1.00 |
|         Razor_Add_Independent |  Current |            Release |  37.186 ms | 0.6021 ms | 0.5632 ms |  0.23 |    0.01 |  400.0000 |  200.0000 |   4.33 MB |        0.11 |
|                               |          |                    |            |           |           |       |         |           |           |           |             |
|        Razor_Edit_Independent | Baseline |      Release_Nuget |   9.989 ms | 0.0919 ms | 0.0860 ms |  1.00 |    0.00 |  312.5000 |  156.2500 |   3.04 MB |        1.00 |
|        Razor_Edit_Independent |  Current |            Release |  10.549 ms | 0.1785 ms | 0.1669 ms |  1.06 |    0.02 |  328.1250 |  281.2500 |   3.18 MB |        1.05 |
|                               |          |                    |            |           |           |       |         |           |           |           |             |
|      Razor_Remove_Independent | Baseline |      Release_Nuget | 160.349 ms | 3.1932 ms | 4.5796 ms |  1.00 |    0.00 | 4000.0000 | 1000.0000 |   40.1 MB |        1.00 |
|      Razor_Remove_Independent |  Current |            Release |  28.421 ms | 0.1224 ms | 0.1085 ms |  0.17 |    0.01 |  343.7500 |  281.2500 |   3.57 MB |        0.09 |
|                               |          |                    |            |           |           |       |         |           |           |           |             |
| Razor_Edit_DependentIgnorable | Baseline |      Release_Nuget |   2.862 ms | 0.0558 ms | 0.0686 ms |  1.00 |    0.00 |  101.5625 |   46.8750 |   1.01 MB |        1.00 |
| Razor_Edit_DependentIgnorable |  Current |            Release |   3.129 ms | 0.0125 ms | 0.0097 ms |  1.11 |    0.02 |  109.3750 |   54.6875 |   1.12 MB |        1.11 |
|                               |          |                    |            |           |           |       |         |           |           |           |             |
|          Razor_Edit_Dependent | Baseline |      Release_Nuget | 165.732 ms | 3.0128 ms | 2.6708 ms |  1.00 |    0.00 | 4000.0000 | 1000.0000 |  40.57 MB |        1.00 |
|          Razor_Edit_Dependent |  Current |            Release |  34.394 ms | 0.6597 ms | 0.7059 ms |  0.21 |    0.00 |  400.0000 |  200.0000 |   4.41 MB |        0.11 |
|                               |          |                    |            |           |           |       |         |           |           |           |             |
|        Razor_Remove_Dependent | Baseline |      Release_Nuget | 158.481 ms | 2.6138 ms | 2.4450 ms |  1.00 |    0.00 | 4000.0000 | 1000.0000 |  39.18 MB |        1.00 |
|        Razor_Remove_Dependent |  Current |            Release |  30.478 ms | 0.5055 ms | 0.4482 ms |  0.19 |    0.00 |  333.3333 |  166.6667 |   3.58 MB |        0.09 |

@chsienki chsienki marked this pull request as ready for review July 21, 2023 00:06
@chsienki chsienki requested a review from a team as a code owner July 21, 2023 00:06
jjonescz
jjonescz reviewed Jul 21, 2023
View reviewed changes
Copy link
Member

@jjonescz jjonescz left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

@chsienki chsienki mentioned this pull request Jul 21, 2023
Closed
@chsienki
PR Feedback.
2504c2d 
- Sealed, unreachable etc.
- Refactor tag helper static feature aquisition to make it clearer
- Remove unused tag helper methods
@chsienki chsienki force-pushed the source-generator/reenable-perf branch from 6b4f678 to 2504c2d Compare July 21, 2023 18:37
333fred
333fred reviewed Jul 21, 2023
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Chris can you please manually test this compiler with some exemplar projects? ASP.NET, MudBlazor, etc?

chsienki and others added 2 commits July 25, 2023 10:13
@chsienki
Copy link
Member Author

chsienki commented Jul 25, 2023

Chris can you please manually test this compiler with some exemplar projects? ASP.NET, MudBlazor, etc?

@333fred Its been run against MudBlazor and AspNetCore here: https://dev.azure.com/dnceng/internal/_build/results?buildId=2229220&view=logs&j=426a675b-bf70-5b3e-81b7-b93f7cef5a5e&t=7023cd9d-f1a8-5290-89a7-d879caa30bad

@chsienki chsienki changed the base branch from main to release/dev17.8 July 25, 2023 18:30
@chsienki
Copy link
Member Author

chsienki commented Jul 25, 2023

@jjonescz @333fred Feedback addressed, and tests run to confirm things seem to be working as expected. Changing branch target to release/17.8 as we'd like to get this in for 17.8p1

333fred
333fred approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Thanks for the explicit testing!

@chsienki chsienki merged commit 3e86591 into dotnet:release/dev17.8 Jul 26, 2023
@chsienki chsienki mentioned this pull request Jul 27, 2023
Closed
@allisonchou allisonchou mentioned this pull request Jul 28, 2023
Closed
333fred added a commit to 333fred/razor that referenced this pull request Aug 23, 2023
@333fred
Merge remote-tracking branch 'upstream/main' into sourcetext
527044e 
* upstream/main: (188 commits)
  Rename CSHTML file reference. (dotnet#8969)
  Remove Omnisharp logic from main branch (dotnet#9027)
  Update dependencies from https://github.com/dotnet/arcade build 20230726.1
  Fixes CVE-2023-33127 and CVE-2023-33170 (dotnet#9032)
  Remove Async Keyword For Generate Async Method Code Action (dotnet#9030)
  Remove dispatcher from DocumentVersionCache (dotnet#9026)
  Restore perf work. (dotnet#8995)
  Implement priority trigger support
  Change implementations and references
  Rename ProjectSnapshotChangeTrigger and convert to interface
  Updates after merge
  Fix nullability
  Use pattern matching
  Convert to record struct
  Move CloseTextTagOnAutoInsertProvider to FindToken (dotnet#9025)
  Move GenerateMethodCodeActionProvider to FindToken (dotnet#8988)
  Add comment describing when ProjectRazorJson.Version should be incremented
  Some more violations, after the merge
  Remove TryResolveDocument method
  [Infra] 17.8 P1 snap PRs (dotnet#9021)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@jjonescz jjonescz jjonescz approved these changes

Assignees

No one assigned

Labels

area-compiler Umbrella for all compiler issues perf 📊

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chsienki @333fred @jjonescz