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

Use message pack instead of Json.NET #9214

Merged
merged 19 commits into from
Sep 11, 2023

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Aug 31, 2023

This PR should improve the performance of communication with the Roslyn OOP and significantly reduce allocations. It does this by making the following changes:

  • Communication with Roslyn OOP now uses MessagePack rather than Json.NET for serialization. Once MessagePack was enabled, I found that the MessagePack formatters weren't working correctly and needed to be reworked.
  • The Razor MessagePack formatters expend some effort to dedupe strings and MetadataCollection instances. More work can be done here in the future if needed.
  • Tag helpers are now cached in tooling by checksum and are weakly referenced by the cache. The cache implements a simple mechanism to remove dead references over time.
  • When a request is made to Roslyn OOP for the delta of tag helpers since the last request, the changes are returned as checksum. This avoids loads of extra work to serialize all the tag helpers, which is unnecessary if they've already been seen and cached.
  • In the case that a tag helper has not been seen yet, a new API (FetchTagHelpersAsync) has been added to request tag helpers from Roslyn OOP by checksum. The same caching mechanism is used in Roslyn OOP to ensure that this method doesn't have to re-compute tag helpers again.

In addition, I've done work in this change to clean up code that is no longer needed. In particular, all of the JsonConverters (except for one in Microsoft.VisualStudio.LiveShare.Razor) have been deleted.

@dotnet/razor-compiler: The only compiler-level change is an additional HashCodeCombiner.Add(...) overload that takes an ImmutableArray<T>.

@davidwengier: This changes the project.razor.json format by replacing the old hash codes with checksums. So, this will require an insertion for VS Code.

@adrianwright109
Copy link
Contributor

Out of interest does this move from JSON.net to MessagePack include the RazorCodeDocument that's mentioned in #8832

@DustinCampbell
Copy link
Member Author

Out of interest does this move from JSON.net to MessagePack include the RazorCodeDocument that's mentioned in #8832

No, but that's because that code isn't merged yet.

The message pack formatters were implemented incorrectly and did
not work correctly in serialization scenarios, such as StreamJsonRpc.
This change reworks them to fit within that framework.
This change updates TagHelperDeltaResult to return Checksums for
tag helpers that were added and introduces a new OOP API for fetching
tag heleprs by checksum. These are used to only fetch tag helpers from
Roslyn OOP that aren't already cached by Razor tooling.
@DustinCampbell DustinCampbell marked this pull request as ready for review September 7, 2023 00:06
@DustinCampbell DustinCampbell requested review from a team as code owners September 7, 2023 00:06
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 This benchmark was unhelpful because it just serialized/deserialized a very small type.

@ToddGrun
Copy link
Contributor

Very nice! Excited for this change to get in.

@DustinCampbell DustinCampbell merged commit 2b0d9f7 into dotnet:main Sep 11, 2023
@DustinCampbell DustinCampbell deleted the use-message-pack branch September 11, 2023 19:26
@ghost ghost added this to the Next milestone Sep 11, 2023
@Mike-E-angelo
Copy link

Thank you so very much for your diligence and effort out there, @DustinCampbell & team! I cannot express enough how greatly appreciated this is to me. 🙏🙏🙏

@ToddGrun
Copy link
Contributor

Should #8371 be closed now, or is it tracking additional work?

@DustinCampbell
Copy link
Member Author

I think we should wait until the changes make their way to @Mike-E-angelo and see whether they're working.

333fred added a commit to 333fred/razor that referenced this pull request Sep 12, 2023
This reverts commit 2b0d9f7, reversing
changes made to a7f2e33.
@LockTar
Copy link

LockTar commented Sep 13, 2023

@DustinCampbell When will this be released? Can I track it somewhere? Thanks.

@davidwengier
Copy link
Contributor

@LockTar the PR will be set to the appropriate milestone when it merged into VS, with the version it merged into, so you can track it right here :)

@LockTar
Copy link

LockTar commented Sep 13, 2023

@davidwengier stupid of me! Of course the milestones. I was looking at Projects (which are empty) and searching on Microsoft docs. Thanks!

@Mike-E-angelo
Copy link

I had the same question too @LockTar so thank you for finding that out for me 😅

@DustinCampbell
Copy link
Member Author

@LockTar / @Mike-E-angelo: I just wanted to let you know that this change has flowed into Visual Studio. So, it should show up in the next VS Preview.

@Mike-E-angelo
Copy link

Mike-E-angelo commented Sep 19, 2023

Thank you for the update @DustinCampbell it is appreciated. If I understand correctly, that might be another 2 weeks or so (maybe more) before trying here to give the yay/nay. One thought I had is that I have provided an older version of my solution for another issue here:
https://developercommunity.visualstudio.com/t/Disco-Colors-Still-Occur-in-Latest-Previ/10394600#T-N10460536

If you are able to confirm on your side with some testing on your side in regard to this issue, it would be further appreciated. 👍

@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
@Mike-E-angelo
Copy link

Hi @DustinCampbell I have tried 17.8 P3 and unfortunately, while this issue may address other problems the problem persists for my solution. You should be able to reproduce it with the solution on your end with the link above. I appreciate any further consideration or efforts to address this.

@DustinCampbell
Copy link
Member Author

Hi @Mike-E-angelo, sorry for not getting back to you here sooner. We were able to reproduce the issue you described on the solution you provided a week or so ago, and I forgot to make note of it here. Sorry about that! The incorrect colors (affectionately known as "disco colors") aren't affected by this change. I was expecting this change to improve other memory and performance concerns you had raised. Very sorry for not reporting back here.

@Mike-E-angelo
Copy link

Hi @DustinCampbell thank you for the update! It is appreciated. I wanted to clarify that the only relation the disco colors issue has is that it happens to host the solution for your review. I was being lazy and didn't want to re-upload it to another issue/location. 😁 I understand that this issue does not impact the reported issue there.

For further clarity, I was hoping that this issue would assist #8371 but that does not appear to be the case. 😭

@DustinCampbell
Copy link
Member Author

I was hoping for the same thing, and thanks very much for filing a new community issue with updated dumps and ETL traces. I'm digging into those now...

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.

7 participants