Use DocumentKey to avoid holding Razor project/document snapshots when the most recent will be used. #11644
Conversation
This file contains hidden or 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
- Move DocumentKey to Microsoft.AspNetCore.Razor.ProjectEngineHost project alongside ProjectKey.
- Move DocumentKey to the Microsoft.AspNetCore.Razor.ProjectSystem namespace
- Rename DocumentFilePath to FilePath. ("Document" is a bit redundant in a "DocumentKey").
- Implement IComparable<ProjectKey> on ProjectKey.
- Implement IComparable<DocumentKey> on DocumentKey.
- Move ProjectKeyTests to Microsoft.AspNetCore.Razor.ProjectEngineHost.Test project - Delete TestProjectKey. This is a pretty old helper and creates a pretty dubious ProjectKey. The few tests that still used this have been updated and fixed where necessary. - Add tests for comparing ProjectKeys. - Add tests for comparing DocumentKeys.
This adds two overloads to GetMostRecentUniqueItems. The first overload takes a HashSet<T> instead of an IEqualityComparer<T>. This allows a caller to avoid an allocation by passing an empty set. The second overload doesn't take an additional argument. Instead, it acquires a set from HashSetPool<T>.
- Add ProjectSnapshotManager.ContainsDocument(DocumentKey) extension method - Add ProjectSnapshotManager.TryGetDocument(DocumentKey, ...) extension method - Add ProjectSnapshotManager.GetDocument(DocumentKey) extension method - Add ProjectSnapshotManager.GetRequiredDocument(DocumentKey) extension method - Add ProjectSnapshotManager.Updater.ContainsDocument(DocumentKey) extension method - Add ProjectSnapshotManager.Updater.TryGetDocument(DocumentKey, ...) extension method - Add ProjectSnapshotManager.Updater.GetDocument(DocumentKey) extension method - Add ProjectSnapshotManager.Updater.GetRequiredDocument(DocumentKey) extension method
Update ProjectStateChangeDetector to use a dedicated HashSet instance to compute the most recent unique items.
Update AbstractRazorProjectInfoDriver to use a dedicated HashSet instance to compute the most recent unique items.
Update OpenDocumentGenerator to use a dedicated HashSet instance to compute the most recent unique items. In addition, make the following changes: - Add a DocumentSnapshot.Key property that produces a DocumentKey. - Update OpenDocumentGenerator's async batching work queue to track DocumentKeys rather than DocumentSnapshots. - Prefer faster boolean tests first, but checking _options.UpdateBuffersForCLosedDocuments before _projectManager.IsDocumentOpen(...)
Update IFallbackProjectManager.IsFallbackProject(...) to take a ProjectKey rather than a ProjectSnapshot. In addition, this commit includes a couple of changes to reorder some checks to avoid creating a ProjectSnapshot if it's not a fallback project.
Update BackgroundDocumentGenerator to use a dedicated HashSet instance to compute the most recent unique items. This is a larger change that makes BackgroundDocumentGenerator track work by DocumentKey rather than (ProjectSnapshot, DocumentSnapshot): - Update BackgroundDocumentGenerator's async batching work queue to track DocumentKeys rather than (ProjectSnapshot, DocumentSnapshot). - Update document suppression to use DocumentKey.
davidwengier
approved these changes
Mar 19, 2025
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/OpenDocumentGenerator.cs
Outdated
Show resolved
Hide resolved
When iterating over `ProjectSnapshot.DocumentFilePaths`, it is redundant to call `ContainsDocument(...)` for those file paths on the same project. We already know it contains those file paths.
Move a logging message from the point that a document is enqueued to the point that is dequeued and processed. This also avoids instantiating a DocumentSnapshot to get its Version until later.
- Change ProjectSnapshot.GetRelatedDocuments(DocumentSnapshot) to GetRelatedDocumentFIlePaths(string) that takes a document file path and returns document file paths. This avoids creating DocumentSnapshots unnecessarily to call this method or for the result. - Make OpenDocumentGenerator and BackgroundDocumentGenerator ProjectManager_Changed effectively match. - Rename BackgroundDocumentGenerator.Enqueue to EnqueueIfNecessary to match OpenDocumentGeneator. - Merge ProjectAdded and ProjectChanged cases in BackgroundDocumentGenerator since they match. - Add ProjectAdded case in OpenDocumentGenerator. This is probably a no-op, but it matches BackgroundDocumentGenerator. - Add _solutionClosing flag to OpenDocumentGenerator to match BackgroundDocumentGenerator.
|
@davidwengier: Just wanted to note that the most recent commit has more significant changes that you might want to take a look at. |
davidwengier
approved these changes
Mar 20, 2025
This was referenced Mar 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The goal of this change is to move
DocumentKeyclose toProjectKeyand use it various async batching work queues rather than holding onto project or document snapshots. I recommend reviewing commit-by-commit. Each commit describes its changes.CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2668867&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/621273