Don't hold onto Roslyn projects longer than necessary #11458
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Calling Project.Matches(...) rather than Project.ToProjectKey() avoids a string allocation per project.
Both the WorkspaceProjectStateChangeDetector and ProjectWorkspaceStateGenerator capture Roslyn Projects and Razor ProjectSnapshots in an async batching work queue and then just use the last one. This change avoids holding onto all of that extra memory by just capturing the Roslyn ProjectId and Razor ProjectKey. This change includes a fairly substantial update to tests, but they're a bit more comprehensible now, IMHO.
We had taken the time to ensure that the vs-threading analyzers could look for our custom JTF assert helpers, but it stopped working when they were moved to a different namespace. This fixes that problem, which allowed the vs-threading analyzers to find a bit of suspicious code. (The suspicious code is fully intentional and there's a comment explaining it, but it's great to see the analyzer point this out!)
This mechanical change organizes all code related to tag helper discovery in the same place.
This change attempts to tame ProjectStateChangeDetector a bit. - Remove duplicated code - Add cache for assembly path to ProjectKey to avoid extra allocations - Avoid LINQ code - Make it clearer when a project update is enqueued vs. a project remove. - Fix a few race conditions in tests (causing failures now that it's slightly faster)
davidwengier
approved these changes
Feb 6, 2025
|
Looks like some good wins in this change.
|
|
The only failing integration test is the same Code Folding test that has failed since FUSE was enabled by default. I'll call that a pass for this PR. 😄 |
This was referenced Feb 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In general, it's good practice to avoid holding onto Roslyn projects longer than necessary because it results in a lot of memory hanging around for the GC to clean up at once. Unfortunately, that is exactly what Razor's
WorkspaceProjectStateChangeDetectordoes. It holds Roslyn projects and Razor project snapshots in an async batching work queue, only to throw most of them away when the batch is processed. This change avoids that by changing the work queue to hold RazorProjectKeysand RoslynProjectIds. In addition, a lot of refactoring has been done to ensure that the code is more readable, more maintainable, and has far fewer allocations. In particular, there were a lot of extra string allocations coming from file path normalization becauseProjectKeyswere being created over and over again. So, a simple cache has been introduced so that aProjectKeyis only created once per assembly path.As part of this change, I've renamed and organized all of the files related to tag helper discovery in Microsoft.VisualsStudio.LanguageServices.Razor. So, I recommend reviewing commit-by-commit, because the final PR looks like a bunch of new files replacing deleted files.
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2636157&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/608737